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
