Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: Introduce guard pages to i915_vma

From: Andi Shyti <andi.shyti@linux.intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	"Chris Wilson" <chris@chris-wilson.co.uk>,
	"Matthew Auld" <matthew.auld@intel.com>,
	"Andi Shyti" <andi.shyti@linux.intel.com>
Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: Introduce guard pages to i915_vma
Date: Wed, 23 Nov 2022 19:54:21 +0100	[thread overview]
Message-ID: <Y35sXXLiAmwulDRU@ashyti-mobl2.lan> (raw)
In-Reply-To: <a579e9a5-0bd4-d439-3193-64dc52e05997@linux.intel.com>

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