[igt-dev] [PATCH i-g-t v2 09/12] lib/intel_buf: support pat_index

Zbigniew Kempczyński zbigniew.kempczynski at intel.com
Fri Oct 13 08:07:40 UTC 2023


On Wed, Oct 11, 2023 at 04:09:50PM +0100, Matthew Auld wrote:
> Some users need to able select their own pat_index. Some display tests
> use igt_draw which in turn uses intel_batchbuffer and intel_buf.  We
> also have a couple more display tests directly using these interfaces
> directly. Idea is to select wt/uc for anything display related, but also
> allow any test to select a pat_index for a given intel_buf.
> 
> v2: (Zbigniew):
>   - Add some macro helpers for decoding pat_index and range in rsvd1 (Zbigniew):
>   - Rather use uc than wt. On xe2+ wt uses compression so CPU access
>     might not work as expected, so for now just use uc.
> 
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Pallavi Mishra <pallavi.mishra at intel.com>
> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> ---
>  lib/igt_draw.c            |  7 ++++-
>  lib/igt_fb.c              |  3 ++-
>  lib/intel_allocator.c     |  1 +
>  lib/intel_allocator.h     |  1 +
>  lib/intel_batchbuffer.c   | 55 ++++++++++++++++++++++++++++++---------
>  lib/intel_bufops.c        | 29 ++++++++++++++-------
>  lib/intel_bufops.h        |  9 +++++--
>  tests/intel/kms_big_fb.c  |  4 ++-
>  tests/intel/kms_dirtyfb.c |  7 +++--
>  tests/intel/kms_psr.c     |  4 ++-
>  tests/intel/xe_intel_bb.c |  3 ++-
>  11 files changed, 93 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/igt_draw.c b/lib/igt_draw.c
> index 1cf9d87c9..efd3a2436 100644
> --- a/lib/igt_draw.c
> +++ b/lib/igt_draw.c
> @@ -31,6 +31,7 @@
>  #include "intel_batchbuffer.h"
>  #include "intel_chipset.h"
>  #include "intel_mocs.h"
> +#include "intel_pat.h"
>  #include "igt_core.h"
>  #include "igt_fb.h"
>  #include "ioctl_wrappers.h"
> @@ -75,6 +76,7 @@ struct buf_data {
>  	uint32_t size;
>  	uint32_t stride;
>  	int bpp;
> +	uint8_t pat_index;
>  };
>  
>  struct rect {
> @@ -658,7 +660,8 @@ static struct intel_buf *create_buf(int fd, struct buf_ops *bops,
>  				    width, height, from->bpp, 0,
>  				    tiling, 0,
>  				    size, 0,
> -				    region);
> +				    region,
> +				    from->pat_index);
>  
>  	/* Make sure we close handle on destroy path */
>  	intel_buf_set_ownership(buf, true);
> @@ -791,6 +794,7 @@ static void draw_rect_render(int fd, struct cmd_data *cmd_data,
>  	igt_skip_on(!rendercopy);
>  
>  	/* We create a temporary buffer and copy from it using rendercopy. */
> +	tmp.pat_index = buf->pat_index;
>  	tmp.size = rect->w * rect->h * pixel_size;
>  	if (is_i915_device(fd))
>  		tmp.handle = gem_create(fd, tmp.size);
> @@ -858,6 +862,7 @@ void igt_draw_rect(int fd, struct buf_ops *bops, uint32_t ctx,
>  		.size = buf_size,
>  		.stride = buf_stride,
>  		.bpp = bpp,
> +		.pat_index = intel_get_pat_idx_uc(fd),
>  	};
>  	struct rect rect = {
>  		.x = rect_x,
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 0a9855453..9a70f7d6a 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2637,7 +2637,8 @@ igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
>  				    igt_fb_mod_to_tiling(fb->modifier),
>  				    compression, fb->size,
>  				    fb->strides[0],
> -				    region);
> +				    region,
> +				    intel_get_pat_idx_uc(fd));
>  	intel_buf_set_name(buf, name);
>  
>  	/* Make sure we close handle on destroy path */
> diff --git a/lib/intel_allocator.c b/lib/intel_allocator.c
> index d7e9dc0a6..52b4e5bb0 100644
> --- a/lib/intel_allocator.c
> +++ b/lib/intel_allocator.c
> @@ -1452,6 +1452,7 @@ bool intel_allocator_is_reserved(uint64_t allocator_handle,
>  bool intel_allocator_reserve_if_not_allocated(uint64_t allocator_handle,
>  					      uint32_t handle,
>  					      uint64_t size, uint64_t offset,
> +					      uint8_t pat_index,

I haven't seen you extended reservation request with pat_index in
allocator so this finally is no-op. I would likely drop this
pat_index for reserve_if_not_allocated as you're not passing
it to intel_allocator_is_allocated() (checking is this range
is occupied regardless pat index is fine for me).

--
Zbigniew


>  					      bool *is_allocatedp)
>  {
>  	struct alloc_req req = { .request_type = REQ_RESERVE_IF_NOT_ALLOCATED,
> diff --git a/lib/intel_allocator.h b/lib/intel_allocator.h
> index 4b6292f06..14852db2d 100644
> --- a/lib/intel_allocator.h
> +++ b/lib/intel_allocator.h
> @@ -206,6 +206,7 @@ bool intel_allocator_is_reserved(uint64_t allocator_handle,
>  bool intel_allocator_reserve_if_not_allocated(uint64_t allocator_handle,
>  					      uint32_t handle,
>  					      uint64_t size, uint64_t offset,
> +					      uint8_t pat_index,
>  					      bool *is_allocatedp);
>  
>  void intel_allocator_print(uint64_t allocator_handle);
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index e7b1b755f..b7d63765b 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -38,6 +38,7 @@
>  #include "intel_batchbuffer.h"
>  #include "intel_bufops.h"
>  #include "intel_chipset.h"
> +#include "intel_pat.h"
>  #include "media_fill.h"
>  #include "media_spin.h"
>  #include "sw_sync.h"
> @@ -825,15 +826,18 @@ static void __reallocate_objects(struct intel_bb *ibb)
>  static inline uint64_t __intel_bb_get_offset(struct intel_bb *ibb,
>  					     uint32_t handle,
>  					     uint64_t size,
> -					     uint32_t alignment)
> +					     uint32_t alignment,
> +					     uint8_t pat_index)
>  {
>  	uint64_t offset;
>  
>  	if (ibb->enforce_relocs)
>  		return 0;
>  
> -	offset = intel_allocator_alloc(ibb->allocator_handle,
> -				       handle, size, alignment);
> +	offset = __intel_allocator_alloc(ibb->allocator_handle, handle,
> +					 size, alignment, pat_index,
> +					 ALLOC_STRATEGY_NONE);
> +	igt_assert(offset != ALLOC_INVALID_ADDRESS);
>  
>  	return offset;
>  }
> @@ -1280,6 +1284,10 @@ void intel_bb_destroy(struct intel_bb *ibb)
>  	free(ibb);
>  }
>  
> +#define SZ_4K	0x1000
> +#define XE_OBJ_SIZE(rsvd1) ((rsvd1) & ~(SZ_4K-1))
> +#define XE_OBJ_PAT_IDX(rsvd1) ((rsvd1) & (SZ_4K-1))
> +
>  static struct drm_xe_vm_bind_op *xe_alloc_bind_ops(struct intel_bb *ibb,
>  						   uint32_t op, uint32_t region)
>  {
> @@ -1300,11 +1308,14 @@ static struct drm_xe_vm_bind_op *xe_alloc_bind_ops(struct intel_bb *ibb,
>  		ops->op = op;
>  		ops->obj_offset = 0;
>  		ops->addr = objects[i]->offset;
> -		ops->range = objects[i]->rsvd1;
> +		ops->range = XE_OBJ_SIZE(objects[i]->rsvd1);
>  		ops->region = region;
> +		if (set_obj)
> +			ops->pat_index = XE_OBJ_PAT_IDX(objects[i]->rsvd1);
>  
> -		igt_debug("  [%d]: handle: %u, offset: %llx, size: %llx\n",
> -			  i, ops->obj, (long long)ops->addr, (long long)ops->range);
> +		igt_debug("  [%d]: handle: %u, offset: %llx, size: %llx pat_index: %u\n",
> +			  i, ops->obj, (long long)ops->addr, (long long)ops->range,
> +			  ops->pat_index);
>  	}
>  
>  	return bind_ops;
> @@ -1409,7 +1420,8 @@ void intel_bb_reset(struct intel_bb *ibb, bool purge_objects_cache)
>  		ibb->batch_offset = __intel_bb_get_offset(ibb,
>  							  ibb->handle,
>  							  ibb->size,
> -							  ibb->alignment);
> +							  ibb->alignment,
> +							  DEFAULT_PAT_INDEX);
>  
>  	intel_bb_add_object(ibb, ibb->handle, ibb->size,
>  			    ibb->batch_offset,
> @@ -1645,7 +1657,8 @@ static void __remove_from_objects(struct intel_bb *ibb,
>   */
>  static struct drm_i915_gem_exec_object2 *
>  __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
> -		      uint64_t offset, uint64_t alignment, bool write)
> +		      uint64_t offset, uint64_t alignment, uint8_t pat_index,
> +		      bool write)
>  {
>  	struct drm_i915_gem_exec_object2 *object;
>  
> @@ -1661,6 +1674,9 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
>  	object = __add_to_cache(ibb, handle);
>  	__add_to_objects(ibb, object);
>  
> +	if (pat_index == DEFAULT_PAT_INDEX)
> +		pat_index = intel_get_pat_idx_wb(ibb->fd);
> +
>  	/*
>  	 * If object->offset == INVALID_ADDRESS we added freshly object to the
>  	 * cache. In that case we have two choices:
> @@ -1670,7 +1686,7 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
>  	if (INVALID_ADDR(object->offset)) {
>  		if (INVALID_ADDR(offset)) {
>  			offset = __intel_bb_get_offset(ibb, handle, size,
> -						       alignment);
> +						       alignment, pat_index);
>  		} else {
>  			offset = offset & (ibb->gtt_size - 1);
>  
> @@ -1683,6 +1699,7 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
>  
>  				reserved = intel_allocator_reserve_if_not_allocated(ibb->allocator_handle,
>  										    handle, size, offset,
> +										    pat_index,
>  										    &allocated);
>  				igt_assert_f(allocated || reserved,
>  					     "Can't get offset, allocated: %d, reserved: %d\n",
> @@ -1721,6 +1738,18 @@ __intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
>  	if (ibb->driver == INTEL_DRIVER_XE) {
>  		object->alignment = alignment;
>  		object->rsvd1 = size;
> +		igt_assert(!XE_OBJ_PAT_IDX(object->rsvd1));
> +
> +		if (pat_index == DEFAULT_PAT_INDEX)
> +			pat_index = intel_get_pat_idx_wb(ibb->fd);
> +
> +		/*
> +		 * XXX: For now encode the pat_index in the first few bits of
> +		 * rsvd1. intel_batchbuffer should really stop using the i915
> +		 * drm_i915_gem_exec_object2 to encode VMA placement
> +		 * information on xe...
> +		 */
> +		object->rsvd1 |= pat_index;
>  	}
>  
>  	return object;
> @@ -1733,7 +1762,7 @@ intel_bb_add_object(struct intel_bb *ibb, uint32_t handle, uint64_t size,
>  	struct drm_i915_gem_exec_object2 *obj = NULL;
>  
>  	obj = __intel_bb_add_object(ibb, handle, size, offset,
> -				    alignment, write);
> +				    alignment, DEFAULT_PAT_INDEX, write);
>  	igt_assert(obj);
>  
>  	return obj;
> @@ -1795,8 +1824,10 @@ __intel_bb_add_intel_buf(struct intel_bb *ibb, struct intel_buf *buf,
>  		}
>  	}
>  
> -	obj = intel_bb_add_object(ibb, buf->handle, intel_buf_bo_size(buf),
> -				  buf->addr.offset, alignment, write);
> +	obj = __intel_bb_add_object(ibb, buf->handle, intel_buf_bo_size(buf),
> +				    buf->addr.offset, alignment, buf->pat_index,
> +				    write);
> +	igt_assert(obj);
>  	buf->addr.offset = obj->offset;
>  
>  	if (igt_list_empty(&buf->link)) {
> diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> index 2c91adb88..fbee4748e 100644
> --- a/lib/intel_bufops.c
> +++ b/lib/intel_bufops.c
> @@ -29,6 +29,7 @@
>  #include "igt.h"
>  #include "igt_x86.h"
>  #include "intel_bufops.h"
> +#include "intel_pat.h"
>  #include "xe/xe_ioctl.h"
>  #include "xe/xe_query.h"
>  
> @@ -818,7 +819,7 @@ static void __intel_buf_init(struct buf_ops *bops,
>  			     int width, int height, int bpp, int alignment,
>  			     uint32_t req_tiling, uint32_t compression,
>  			     uint64_t bo_size, int bo_stride,
> -			     uint64_t region)
> +			     uint64_t region, uint8_t pat_index)
>  {
>  	uint32_t tiling = req_tiling;
>  	uint64_t size;
> @@ -839,6 +840,10 @@ static void __intel_buf_init(struct buf_ops *bops,
>  	IGT_INIT_LIST_HEAD(&buf->link);
>  	buf->mocs = INTEL_BUF_MOCS_DEFAULT;
>  
> +	if (pat_index == DEFAULT_PAT_INDEX)
> +		pat_index = intel_get_pat_idx_wb(bops->fd);
> +	buf->pat_index = pat_index;
> +
>  	if (compression) {
>  		igt_require(bops->intel_gen >= 9);
>  		igt_assert(req_tiling == I915_TILING_Y ||
> @@ -957,7 +962,7 @@ void intel_buf_init(struct buf_ops *bops,
>  	region = bops->driver == INTEL_DRIVER_I915 ? I915_SYSTEM_MEMORY :
>  						     system_memory(bops->fd);
>  	__intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
> -			 tiling, compression, 0, 0, region);
> +			 tiling, compression, 0, 0, region, DEFAULT_PAT_INDEX);
>  
>  	intel_buf_set_ownership(buf, true);
>  }
> @@ -974,7 +979,7 @@ void intel_buf_init_in_region(struct buf_ops *bops,
>  			      uint64_t region)
>  {
>  	__intel_buf_init(bops, 0, buf, width, height, bpp, alignment,
> -			 tiling, compression, 0, 0, region);
> +			 tiling, compression, 0, 0, region, DEFAULT_PAT_INDEX);
>  
>  	intel_buf_set_ownership(buf, true);
>  }
> @@ -1033,7 +1038,7 @@ void intel_buf_init_using_handle(struct buf_ops *bops,
>  				 uint32_t req_tiling, uint32_t compression)
>  {
>  	__intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> -			 req_tiling, compression, 0, 0, -1);
> +			 req_tiling, compression, 0, 0, -1, DEFAULT_PAT_INDEX);
>  }
>  
>  /**
> @@ -1050,6 +1055,7 @@ void intel_buf_init_using_handle(struct buf_ops *bops,
>   * @size: real bo size
>   * @stride: bo stride
>   * @region: region
> + * @pat_index: pat_index to use for the binding (only used on xe)
>   *
>   * Function configures BO handle within intel_buf structure passed by the caller
>   * (with all its metadata - width, height, ...). Useful if BO was created
> @@ -1067,10 +1073,12 @@ void intel_buf_init_full(struct buf_ops *bops,
>  			 uint32_t compression,
>  			 uint64_t size,
>  			 int stride,
> -			 uint64_t region)
> +			 uint64_t region,
> +			 uint8_t pat_index)
>  {
>  	__intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> -			 req_tiling, compression, size, stride, region);
> +			 req_tiling, compression, size, stride, region,
> +			 pat_index);
>  }
>  
>  /**
> @@ -1149,7 +1157,8 @@ struct intel_buf *intel_buf_create_using_handle_and_size(struct buf_ops *bops,
>  							 int stride)
>  {
>  	return intel_buf_create_full(bops, handle, width, height, bpp, alignment,
> -				     req_tiling, compression, size, stride, -1);
> +				     req_tiling, compression, size, stride, -1,
> +				     DEFAULT_PAT_INDEX);
>  }
>  
>  struct intel_buf *intel_buf_create_full(struct buf_ops *bops,
> @@ -1160,7 +1169,8 @@ struct intel_buf *intel_buf_create_full(struct buf_ops *bops,
>  					uint32_t compression,
>  					uint64_t size,
>  					int stride,
> -					uint64_t region)
> +					uint64_t region,
> +					uint8_t pat_index)
>  {
>  	struct intel_buf *buf;
>  
> @@ -1170,7 +1180,8 @@ struct intel_buf *intel_buf_create_full(struct buf_ops *bops,
>  	igt_assert(buf);
>  
>  	__intel_buf_init(bops, handle, buf, width, height, bpp, alignment,
> -			 req_tiling, compression, size, stride, region);
> +			 req_tiling, compression, size, stride, region,
> +			 pat_index);
>  
>  	return buf;
>  }
> diff --git a/lib/intel_bufops.h b/lib/intel_bufops.h
> index 4dfe4681c..b6048402b 100644
> --- a/lib/intel_bufops.h
> +++ b/lib/intel_bufops.h
> @@ -63,6 +63,9 @@ struct intel_buf {
>  	/* Content Protection*/
>  	bool is_protected;
>  
> +	/* pat_index to use for mapping this buf. Only used in Xe. */
> +	uint8_t pat_index;
> +
>  	/* For debugging purposes */
>  	char name[INTEL_BUF_NAME_MAXSIZE + 1];
>  };
> @@ -161,7 +164,8 @@ void intel_buf_init_full(struct buf_ops *bops,
>  			 uint32_t compression,
>  			 uint64_t size,
>  			 int stride,
> -			 uint64_t region);
> +			 uint64_t region,
> +			 uint8_t pat_index);
>  
>  struct intel_buf *intel_buf_create(struct buf_ops *bops,
>  				   int width, int height,
> @@ -192,7 +196,8 @@ struct intel_buf *intel_buf_create_full(struct buf_ops *bops,
>  					uint32_t compression,
>  					uint64_t size,
>  					int stride,
> -					uint64_t region);
> +					uint64_t region,
> +					uint8_t pat_index);
>  void intel_buf_destroy(struct intel_buf *buf);
>  
>  static inline void intel_buf_set_pxp(struct intel_buf *buf, bool new_pxp_state)
> diff --git a/tests/intel/kms_big_fb.c b/tests/intel/kms_big_fb.c
> index 2c7b24fca..64a67e34a 100644
> --- a/tests/intel/kms_big_fb.c
> +++ b/tests/intel/kms_big_fb.c
> @@ -34,6 +34,7 @@
>  #include <string.h>
>  
>  #include "i915/gem_create.h"
> +#include "intel_pat.h"
>  #include "xe/xe_ioctl.h"
>  #include "xe/xe_query.h"
>  
> @@ -88,7 +89,8 @@ static struct intel_buf *init_buf(data_t *data,
>  	handle = gem_open(data->drm_fd, name);
>  	buf = intel_buf_create_full(data->bops, handle, width, height,
>  				    bpp, 0, tiling, 0, size, 0,
> -				    region);
> +				    region,
> +				    intel_get_pat_idx_uc(data->drm_fd));
>  
>  	intel_buf_set_name(buf, buf_name);
>  	intel_buf_set_ownership(buf, true);
> diff --git a/tests/intel/kms_dirtyfb.c b/tests/intel/kms_dirtyfb.c
> index cc9529178..bf9f91505 100644
> --- a/tests/intel/kms_dirtyfb.c
> +++ b/tests/intel/kms_dirtyfb.c
> @@ -10,6 +10,7 @@
>  
>  #include "i915/intel_drrs.h"
>  #include "i915/intel_fbc.h"
> +#include "intel_pat.h"
>  
>  #include "xe/xe_query.h"
>  
> @@ -246,14 +247,16 @@ static void run_test(data_t *data)
>  				    0,
>  				    igt_fb_mod_to_tiling(data->fbs[1].modifier),
>  				    0, 0, 0, is_xe_device(data->drm_fd) ?
> -				    system_memory(data->drm_fd) : 0);
> +				    system_memory(data->drm_fd) : 0,
> +				    intel_get_pat_idx_uc(data->drm_fd));
>  	dst = intel_buf_create_full(data->bops, data->fbs[2].gem_handle,
>  				    data->fbs[2].width,
>  				    data->fbs[2].height,
>  				    igt_drm_format_to_bpp(data->fbs[2].drm_format),
>  				    0, igt_fb_mod_to_tiling(data->fbs[2].modifier),
>  				    0, 0, 0, is_xe_device(data->drm_fd) ?
> -				    system_memory(data->drm_fd) : 0);
> +				    system_memory(data->drm_fd) : 0,
> +				    intel_get_pat_idx_uc(data->drm_fd));
>  	ibb = intel_bb_create(data->drm_fd, PAGE_SIZE);
>  
>  	spin = igt_spin_new(data->drm_fd, .ahnd = ibb->allocator_handle);
> diff --git a/tests/intel/kms_psr.c b/tests/intel/kms_psr.c
> index ffecc5222..4cc41e479 100644
> --- a/tests/intel/kms_psr.c
> +++ b/tests/intel/kms_psr.c
> @@ -31,6 +31,7 @@
>  #include "igt.h"
>  #include "igt_sysfs.h"
>  #include "igt_psr.h"
> +#include "intel_pat.h"
>  #include <errno.h>
>  #include <stdbool.h>
>  #include <stdio.h>
> @@ -356,7 +357,8 @@ static struct intel_buf *create_buf_from_fb(data_t *data,
>  	name = gem_flink(data->drm_fd, fb->gem_handle);
>  	handle = gem_open(data->drm_fd, name);
>  	buf = intel_buf_create_full(data->bops, handle, width, height,
> -				    bpp, 0, tiling, 0, size, stride, region);
> +				    bpp, 0, tiling, 0, size, stride, region,
> +				    intel_get_pat_idx_uc(data->drm_fd));
>  	intel_buf_set_ownership(buf, true);
>  
>  	return buf;
> diff --git a/tests/intel/xe_intel_bb.c b/tests/intel/xe_intel_bb.c
> index 0159a3164..e2480acf8 100644
> --- a/tests/intel/xe_intel_bb.c
> +++ b/tests/intel/xe_intel_bb.c
> @@ -19,6 +19,7 @@
>  #include "igt.h"
>  #include "igt_crc.h"
>  #include "intel_bufops.h"
> +#include "intel_pat.h"
>  #include "xe/xe_ioctl.h"
>  #include "xe/xe_query.h"
>  
> @@ -400,7 +401,7 @@ static void create_in_region(struct buf_ops *bops, uint64_t region)
>  	intel_buf_init_full(bops, handle, &buf,
>  			    width/4, height, 32, 0,
>  			    I915_TILING_NONE, 0,
> -			    size, 0, region);
> +			    size, 0, region, DEFAULT_PAT_INDEX);
>  	intel_buf_set_ownership(&buf, true);
>  
>  	intel_bb_add_intel_buf(ibb, &buf, false);
> -- 
> 2.41.0
> 


More information about the igt-dev mailing list