Re: [Intel-gfx] [PATCH v4 3/5] drm/i915: Introduce guard pages to i915_vma

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Andi Shyti <andi.shyti@linux.intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	intel-gfx@lists.freedesktop.org,
	"Matthew Auld" <matthew.auld@intel.com>,
	dri-devel@lists.freedesktop.org,
	"Chris Wilson" <chris@chris-wilson.co.uk>
Subject: Re: [Intel-gfx] [PATCH v4 3/5] drm/i915: Introduce guard pages to i915_vma
Date: Thu, 1 Dec 2022 10:50:58 +0000	[thread overview]
Message-ID: <15153e25-c104-003c-0bdc-47581a162c93@linux.intel.com> (raw)
In-Reply-To: <Y4iFuizKSmS2YURK@ashyti-mobl2.lan>

On 01/12/2022 10:45, Andi Shyti wrote:
> Hi Tvrtko,
> 
> [...]
> 
>>> @@ -768,8 +773,17 @@ 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 */
>>> +	if (flags & PIN_OFFSET_GUARD) {
>>> +		GEM_BUG_ON(overflows_type(flags & PIN_OFFSET_MASK, u32));
>>> +		guard = max_t(u32, guard, flags & PIN_OFFSET_MASK);
>>> +	}
>>> +	roundup(guard, BIT(vma->vm->scratch_order + PAGE_SHIFT));
>>
>> roundup = ?
> 
> ehehe... yes, please ignore, that's some copy/paste error during
> the rebase...
> 
>> Lets have a comment here as well.
>>
>> /*
>>   * Be efficient with PTE use by using the native size for the guard.
>>   */
>>
>> Would that be accurate?
> 
> and I also forgot the update of my previous comment... yours is
> quite accurate.
> 
>>> +
>>>    	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>>>    	GEM_BUG_ON(!IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
>>> +	/* We need to be sure we do not ecceed the va area */
>>> +	GEM_BUG_ON(2 * guard > end);
>>
>> "exceed" but haven't we said this is not needed?
> 
> I wrote it in the cover letter. I had an offline chat with Chris
> and he was keen to have this check not only for overflow
> protection but also for a documentation purpose so that the
> reader knows better about the size and usage of the guard.
> 
> Does it make sense?

Not to me really, I have no idea how could anyone ever end up with guard 
size of half+ of GGTT. And the total 2 * guard + size is checked and 
rejected already. So I have no idea what it is supposed to be 
documenting. GEM_BUG_ON suggests really bad things would happen if that 
was passed in, like something would be incorrectly calculated. If that 
is not the case and things would just safely fail then it is just 
confusing to have it.

Regards,

Tvrtko