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

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

 




On 23/11/2022 18:54, Andi Shyti wrote:
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?

Don't know, initially I thought it maybe needs a comment on what's it doing and why. If it is about aligning to "something" then should it be I915_GTT_MIN_ALIGNMENT?

@@ -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.

Same as above - if there is subtle special meaning please add a comment. Otherwise, for the whole object and not just the guards, it is covered by:

+	if (size > end - 2 * guard) {

I don't follow what is the point on only checking the guards.


[...]

@@ -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.

Hm okay. I guess the u64 alignment and that "guard = ALIGN(guard, alignment);" is what is bothering me to begin with. In other words with that there is a chance to overflow vma->guard with a small guard and large alignment.


[...]

@@ -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.

Okay, to be clear, refactoring of vma->display_alignemnt to be u32 as a separate patch in the series. Thanks!

Regards,

Tvrtko


   	/**
   	 * Count of the number of times this vma has been opened by different

Regards,

Thanks,
Andi

Tvrtko



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux