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?
[...]
> > @@ -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.
[...]
> > @@ -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.
[...]
> > @@ -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.
> > /**
> > * Count of the number of times this vma has been opened by different
>
> Regards,
Thanks,
Andi
> Tvrtko
