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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux