On 23/11/2022 18:54, Andi Shyti wrote:
Hi Tvrtko,
[...]
@@ -768,6 +768,9 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
GEM_BUG_ON(!IS_ALIGNED(alignment, I915_GTT_MIN_ALIGNMENT));
GEM_BUG_ON(!is_power_of_2(alignment));
+ guard = vma->guard; /* retain guard across rebinds */
+ guard = ALIGN(guard, alignment);
Why does guard area needs the same alignment as the requested mapping? What about the fact on 32-bit builds guard is 32-bit and alignment u64?
I guess this just to round up/down guard to something, not
necessarily to that alignment.
Shall I remove it?
Don't know, initially I thought it maybe needs a comment on what's it
doing and why. If it is about aligning to "something" then should it be
I915_GTT_MIN_ALIGNMENT?
@@ -777,6 +780,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
if (flags & PIN_ZONE_4G)
end = min_t(u64, end, (1ULL << 32) - I915_GTT_PAGE_SIZE);
GEM_BUG_ON(!IS_ALIGNED(end, I915_GTT_PAGE_SIZE));
+ GEM_BUG_ON(2 * guard > end);
End is the size of relevant VA area at this point so what and why is this checking?
I think because we want to make sure the padding is at least not
bigger that the size. What is actually wrong with this.
Same as above - if there is subtle special meaning please add a comment.
Otherwise, for the whole object and not just the guards, it is covered by:
+ if (size > end - 2 * guard) {
I don't follow what is the point on only checking the guards.
[...]
@@ -855,6 +869,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
GEM_BUG_ON(!i915_gem_valid_gtt_space(vma, color));
list_move_tail(&vma->vm_link, &vma->vm->bound_list);
+ vma->guard = guard;
unsigned long into u32 - what guarantees no truncation?
we are missing here this part above:
guard = vma->guard; /* retain guard across rebinds */
if (flags & PIN_OFFSET_GUARD) {
GEM_BUG_ON(overflows_type(flags & PIN_OFFSET_MASK, u32));
guard = max_t(u32, guard, flags & PIN_OFFSET_MASK);
}
that should make sure that we fit into 32 bits.
Hm okay. I guess the u64 alignment and that "guard = ALIGN(guard,
alignment);" is what is bothering me to begin with. In other words with
that there is a chance to overflow vma->guard with a small guard and
large alignment.
[...]
@@ -197,14 +197,15 @@ struct i915_vma {
struct i915_fence_reg *fence;
u64 size;
- u64 display_alignment;
struct i915_page_sizes page_sizes;
/* mmap-offset associated with fencing for this vma */
struct i915_mmap_offset *mmo;
+ u32 guard; /* padding allocated around vma->pages within the node */
u32 fence_size;
u32 fence_alignment;
+ u32 display_alignment;
u64 -> u32 for display_alignment looks unrelated change.
./display/intel_fb_pin.c: vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
./gem/i915_gem_domain.c: vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
These two sites need to be changed not to use u64.
Do this part in a separate patch?
Right! will remove it.
Okay, to be clear, refactoring of vma->display_alignemnt to be u32 as a
separate patch in the series. Thanks!
Regards,
Tvrtko
/**
* Count of the number of times this vma has been opened by different
Regards,
Thanks,
Andi
Tvrtko
