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

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Wed Nov 23 11:13:23 UTC 2022


On 22/11/2022 18:57, Andi Shyti wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
> 
> Introduce the concept of padding the i915_vma with guard pages before
> and after. The major consequence is that all ordinary uses of i915_vma
> must use i915_vma_offset/i915_vma_size and not i915_vma.node.start/size
> directly, as the drm_mm_node will include the guard pages that surround
> our object.
> 
> The biggest connundrum is how exactly to mix requesting a fixed address
> with guard pages, particularly through the existing uABI. The user does
> not know about guard pages, so such must be transparent to the user, and
> so the execobj.offset must be that of the object itself excluding the
> guard. So a PIN_OFFSET_FIXED must then be exclusive of the guard pages.
> The caveat is that some placements will be impossible with guard pages,
> as wrap arounds need to be avoided, and the vma itself will require a
> larger node. We must not report EINVAL but ENOSPC as these are unavailable
> locations within the GTT rather than conflicting user requirements.
> 
> In the next patch, we start using guard pages for scanout objects. While
> these are limited to GGTT vma, on a few platforms these vma (or at least
> an alias of the vma) is shared with userspace, so we may leak the
> existence of such guards if we are not careful to ensure that the
> execobj.offset is transparent and excludes the guards. (On such platforms
> like ivb, without full-ppgtt, userspace has to use relocations so the
> presence of more untouchable regions within its GTT such be of no further
> issue.)
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay at intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_ggtt.c     | 14 ++++++++----
>   drivers/gpu/drm/i915/i915_gem_gtt.h      |  3 ++-
>   drivers/gpu/drm/i915/i915_vma.c          | 27 ++++++++++++++++++------
>   drivers/gpu/drm/i915/i915_vma.h          |  5 +++--
>   drivers/gpu/drm/i915/i915_vma_resource.c |  4 ++--
>   drivers/gpu/drm/i915/i915_vma_resource.h |  7 +++++-
>   drivers/gpu/drm/i915/i915_vma_types.h    |  3 ++-
>   7 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 8145851ad23d5..133710258eae6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -287,8 +287,11 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>   	 */
>   
>   	gte = (gen8_pte_t __iomem *)ggtt->gsm;
> -	gte += vma_res->start / I915_GTT_PAGE_SIZE;
> -	end = gte + vma_res->node_size / I915_GTT_PAGE_SIZE;
> +	gte += (vma_res->start - vma_res->guard) / I915_GTT_PAGE_SIZE;
> +	end = gte + vma_res->guard / I915_GTT_PAGE_SIZE;
> +	while (gte < end)
> +		gen8_set_pte(gte++, vm->scratch[0]->encode);
> +	end += (vma_res->node_size + vma_res->guard) / I915_GTT_PAGE_SIZE;
>   
>   	for_each_sgt_daddr(addr, iter, vma_res->bi.pages)
>   		gen8_set_pte(gte++, pte_encode | addr);
> @@ -338,9 +341,12 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>   	dma_addr_t addr;
>   
>   	gte = (gen6_pte_t __iomem *)ggtt->gsm;
> -	gte += vma_res->start / I915_GTT_PAGE_SIZE;
> -	end = gte + vma_res->node_size / I915_GTT_PAGE_SIZE;
> +	gte += (vma_res->start - vma_res->guard) / I915_GTT_PAGE_SIZE;
>   
> +	end = gte + vma_res->guard / I915_GTT_PAGE_SIZE;
> +	while (gte < end)
> +		iowrite32(vm->scratch[0]->encode, gte++);
> +	end += (vma_res->node_size + vma_res->guard) / I915_GTT_PAGE_SIZE;
>   	for_each_sgt_daddr(addr, iter, vma_res->bi.pages)
>   		iowrite32(vm->pte_encode(addr, level, flags), gte++);
>   	GEM_BUG_ON(gte > end);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 8c2f57eb5ddaa..2434197830523 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -44,7 +44,8 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
>   #define PIN_HIGH		BIT_ULL(5)
>   #define PIN_OFFSET_BIAS		BIT_ULL(6)
>   #define PIN_OFFSET_FIXED	BIT_ULL(7)
> -#define PIN_VALIDATE		BIT_ULL(8) /* validate placement only, no need to call unpin() */
> +#define PIN_OFFSET_GUARD	BIT_ULL(8)
> +#define PIN_VALIDATE		BIT_ULL(9) /* validate placement only, no need to call unpin() */
>   
>   #define PIN_GLOBAL		BIT_ULL(10) /* I915_VMA_GLOBAL_BIND */
>   #define PIN_USER		BIT_ULL(11) /* I915_VMA_LOCAL_BIND */
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 2232118babeb3..457e35e03895f 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -419,7 +419,7 @@ i915_vma_resource_init_from_vma(struct i915_vma_resource *vma_res,
>   			       obj->mm.rsgt, i915_gem_object_is_readonly(obj),
>   			       i915_gem_object_is_lmem(obj), obj->mm.region,
>   			       vma->ops, vma->private, __i915_vma_offset(vma),
> -			       __i915_vma_size(vma), vma->size);
> +			       __i915_vma_size(vma), vma->size, vma->guard);
>   }
>   
>   /**
> @@ -749,7 +749,7 @@ static int
>   i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>   		u64 size, u64 alignment, u64 flags)
>   {
> -	unsigned long color;
> +	unsigned long color, guard;
>   	u64 start, end;
>   	int ret;
>   
> @@ -757,7 +757,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>   	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
>   
>   	size = max(size, vma->size);
> -	alignment = max(alignment, vma->display_alignment);
> +	alignment = max_t(typeof(alignment), alignment, vma->display_alignment);
>   	if (flags & PIN_MAPPABLE) {
>   		size = max_t(typeof(size), size, vma->fence_size);
>   		alignment = max_t(typeof(alignment),
> @@ -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?

> +
>   	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>   	GEM_BUG_ON(!IS_ALIGNED(start, I915_GTT_PAGE_SIZE));
>   
> @@ -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?

>   
>   	alignment = max(alignment, i915_vm_obj_min_alignment(vma->vm, vma->obj));
>   
> @@ -784,7 +788,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>   	 * aperture has, reject it early before evicting everything in a vain
>   	 * attempt to find space.
>   	 */
> -	if (size > end) {
> +	if (size > end - 2 * guard) {
>   		drm_dbg(&to_i915(vma->obj->base.dev)->drm,
>   			"Attempting to bind an object larger than the aperture: request=%llu > %s aperture=%llu\n",
>   			size, flags & PIN_MAPPABLE ? "mappable" : "total", end);
> @@ -801,13 +805,23 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>   		if (!IS_ALIGNED(offset, alignment) ||
>   		    range_overflows(offset, size, end))
>   			return -EINVAL;
> +		/*
> +		 * The caller knows not of the guard added by others and
> +		 * requests for the offset of the start of its buffer
> +		 * to be fixed, which may not be the same as the position
> +		 * of the vma->node due to the guard pages.
> +		 */
> +		if (offset < guard || offset + size > end - guard)
> +			return -ENOSPC;
>   
>   		ret = i915_gem_gtt_reserve(vma->vm, ww, &vma->node,
> -					   size, offset, color,
> -					   flags);
> +					   size + 2 * guard,
> +					   offset - guard,
> +					   color, flags);
>   		if (ret)
>   			return ret;
>   	} else {
> +		size += 2 * guard;
>   		/*
>   		 * We only support huge gtt pages through the 48b PPGTT,
>   		 * however we also don't want to force any alignment for
> @@ -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?

>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index 3fd4512b1f65f..ed5c9d682a1b2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -128,7 +128,7 @@ static inline bool i915_vma_is_closed(const struct i915_vma *vma)
>   /* Internal use only. */
>   static inline u64 __i915_vma_size(const struct i915_vma *vma)
>   {
> -	return vma->node.size;
> +	return vma->node.size - 2 * vma->guard;
>   }
>   
>   /**
> @@ -150,7 +150,8 @@ static inline u64 i915_vma_size(const struct i915_vma *vma)
>   /* Internal use only. */
>   static inline u64 __i915_vma_offset(const struct i915_vma *vma)
>   {
> -	return vma->node.start;
> +	/* The actual start of the vma->pages is after the guard pages. */
> +	return vma->node.start + vma->guard;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/i915/i915_vma_resource.c b/drivers/gpu/drm/i915/i915_vma_resource.c
> index de1342dbfa128..6ba7a7feceba1 100644
> --- a/drivers/gpu/drm/i915/i915_vma_resource.c
> +++ b/drivers/gpu/drm/i915/i915_vma_resource.c
> @@ -34,8 +34,8 @@ static struct kmem_cache *slab_vma_resources;
>    * and removal of fences increases as O(ln(pending_unbinds)) instead of
>    * O(1) for a single fence without interval tree.
>    */
> -#define VMA_RES_START(_node) ((_node)->start)
> -#define VMA_RES_LAST(_node) ((_node)->start + (_node)->node_size - 1)
> +#define VMA_RES_START(_node) ((_node)->start - (_node)->guard)
> +#define VMA_RES_LAST(_node) ((_node)->start + (_node)->node_size + (_node)->guard - 1)
>   INTERVAL_TREE_DEFINE(struct i915_vma_resource, rb,
>   		     u64, __subtree_last,
>   		     VMA_RES_START, VMA_RES_LAST, static, vma_res_itree);
> diff --git a/drivers/gpu/drm/i915/i915_vma_resource.h b/drivers/gpu/drm/i915/i915_vma_resource.h
> index 54edf3739ca0b..c1864e3d0b43e 100644
> --- a/drivers/gpu/drm/i915/i915_vma_resource.h
> +++ b/drivers/gpu/drm/i915/i915_vma_resource.h
> @@ -57,6 +57,7 @@ struct i915_page_sizes {
>    * @node_size: Size of the allocated range manager node with padding
>    * subtracted.
>    * @vma_size: Bind size.
> + * @guard: The size of guard area preceding and trailing the bind.
>    * @page_sizes_gtt: Resulting page sizes from the bind operation.
>    * @bound_flags: Flags indicating binding status.
>    * @allocated: Backend private data. TODO: Should move into @private.
> @@ -115,6 +116,7 @@ struct i915_vma_resource {
>   	u64 start;
>   	u64 node_size;
>   	u64 vma_size;
> +	u32 guard;
>   	u32 page_sizes_gtt;
>   
>   	u32 bound_flags;
> @@ -179,6 +181,7 @@ static inline void i915_vma_resource_put(struct i915_vma_resource *vma_res)
>    * @start: Offset into the address space of bind range start after padding.
>    * @node_size: Size of the allocated range manager node minus padding.
>    * @size: Bind size.
> + * @guard: The size of the guard area preceding and trailing the bind.
>    *
>    * Initializes a vma resource allocated using i915_vma_resource_alloc().
>    * The reason for having separate allocate and initialize function is that
> @@ -197,7 +200,8 @@ static inline void i915_vma_resource_init(struct i915_vma_resource *vma_res,
>   					  void *private,
>   					  u64 start,
>   					  u64 node_size,
> -					  u64 size)
> +					  u64 size,
> +					  u32 guard)
>   {
>   	__i915_vma_resource_init(vma_res);
>   	vma_res->vm = vm;
> @@ -215,6 +219,7 @@ static inline void i915_vma_resource_init(struct i915_vma_resource *vma_res,
>   	vma_res->start = start;
>   	vma_res->node_size = node_size;
>   	vma_res->vma_size = size;
> +	vma_res->guard = guard;
>   }
>   
>   static inline void i915_vma_resource_fini(struct i915_vma_resource *vma_res)
> diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> index ec0f6c9f57d02..77fda2244d161 100644
> --- a/drivers/gpu/drm/i915/i915_vma_types.h
> +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> @@ -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?

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

Regards,

Tvrtko


More information about the dri-devel mailing list