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

[Andi Shyti <andi.shyti@linux.intel.com>]

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?

Thanks a lot!

Andi