[PATCH i-g-t 10/15] tests/xe_copy_basic: replace size to rect which keeps objects geometry

[Zbigniew Kempczyński]

On Thu, May 22, 2025 at 02:05:51PM +0200, Francois Dugast wrote:
> On Tue, May 13, 2025 at 08:58:05PM +0200, Zbigniew Kempczyński wrote:
> > Testing byte/page + linear/matrix mem-copy requires passing different
> > pitch/width/height so replace simple size to geometry.
> > 
> > Change mem-copy and mem-set to use rect instead of size for
> > copying bytes.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Francois Dugast <francois.dugast at intel.com>
> > ---
> >  tests/intel/xe_copy_basic.c | 44 +++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tests/intel/xe_copy_basic.c b/tests/intel/xe_copy_basic.c
> > index f252d29fd4..7bec248b06 100644
> > --- a/tests/intel/xe_copy_basic.c
> > +++ b/tests/intel/xe_copy_basic.c
> > @@ -19,6 +19,13 @@
> >  
> >  #define MEM_FILL 0x8b
> >  
> > +struct rect {
> > +	uint32_t pitch;
> > +	uint32_t width;
> > +	uint32_t height;
> > +	enum blt_memop_mode mode;
> > +};
> > +
> >  /**
> >   * TEST: Test to validate copy commands on xe
> >   * Category: Core
> > @@ -42,7 +49,9 @@
> >   */
> >  static void
> >  mem_copy(int fd, uint32_t src_handle, uint32_t dst_handle, const intel_ctx_t *ctx,
> > -	 uint32_t size, uint32_t width, uint32_t height, uint32_t region)
> > +	 enum blt_memop_type type, enum blt_memop_mode mode,
> > +	 uint32_t size, uint32_t pitch,
> > +	 uint32_t width, uint32_t height, uint32_t region)
> >  {
> >  	struct blt_mem_copy_data mem = {};
> >  	uint64_t bb_size = xe_bb_size(fd, SZ_4K);
> > @@ -125,13 +134,15 @@ mem_set(int fd, uint32_t dst_handle, const intel_ctx_t *ctx, uint32_t size,
> >  	munmap(mem.dst.ptr, size);
> >  }
> >  
> > -static void copy_test(int fd, uint32_t size, enum blt_cmd_type cmd, uint32_t region)
> > +static void copy_test(int fd, struct rect *rect, enum blt_cmd_type cmd, uint32_t region)
> >  {
> >  	struct drm_xe_engine_class_instance inst = {
> >  		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
> >  	};
> > -	uint32_t src_handle, dst_handle, vm, exec_queue, src_size, dst_size;
> > -	uint32_t bo_size = ALIGN(size, xe_get_default_alignment(fd));
> > +	uint32_t src_handle, dst_handle, vm, exec_queue;
> > +	uint32_t pitch = rect->pitch ?: rect->width;
> > +	uint32_t blocksize = rect->mode ? pitch << 8 : pitch;
> 
> Is this assuming enum values for MODE_BYTE and MODE_PAGE? Better use
> explicit comparison with: rect->mode == MODE_PAGE ?

Agree. Explicit comparison shows my intention.
> 
> > +	uint32_t bo_size = ALIGN(blocksize * rect->height, xe_get_default_alignment(fd));
> >  	intel_ctx_t *ctx;
> >  
> >  	src_handle = xe_bo_create(fd, 0, bo_size, region, 0);
> > @@ -140,13 +151,11 @@ static void copy_test(int fd, uint32_t size, enum blt_cmd_type cmd, uint32_t reg
> >  	exec_queue = xe_exec_queue_create(fd, vm, &inst, 0);
> >  	ctx = intel_ctx_xe(fd, vm, exec_queue, 0, 0, 0);
> >  
> > -	src_size = bo_size;
> > -	dst_size = bo_size;
> > -
> >  	if (cmd == MEM_COPY)
> > -		mem_copy(fd, src_handle, dst_handle, ctx, src_size, size, 1, region);
> > +		mem_copy(fd, src_handle, dst_handle, ctx, TYPE_LINEAR, rect->mode,
> > +			 bo_size, pitch, rect->width, rect->height, region);
> >  	else if (cmd == MEM_SET)
> > -		mem_set(fd, dst_handle, ctx, dst_size, size, 1, MEM_FILL, region);
> > +		mem_set(fd, dst_handle, ctx, bo_size, rect->width, 1, MEM_FILL, region);
> >  
> >  	gem_close(fd, src_handle);
> >  	gem_close(fd, dst_handle);
> > @@ -160,7 +169,10 @@ igt_main
> >  	int fd;
> >  	struct igt_collection *set, *regions;
> >  	uint32_t region;
> > -	uint64_t size[] = {0xFD, 0x369, 0x3FFF, 0xFFFE};
> > +	struct rect linear[] = { { 0, 0xfd, 1 },
> > +				 { 0, 0x369, 0x1 },
> 
> Any reason for using 0x1 here, unlike for others? What about the mode?

I missed 0x1, it should be 1 to be consistent.

Regarding mode - before I've splitted series I had one single patch
where I changed everything (imagine squash of the series you're
reviewing). On the beginning I didn't exactly knew what I will change
so when everything was already written division to patches required
rework to introduce changes partially. That's why MODE_PAGE may
exists a little bit earlier than it was used. I tried to keep progression
here as much as possible so maybe I'm not consistent about this.

--
Zbigniew

> 
> > +				 { 0, 0x3fff, 1 },
> > +				 { 0, 0xfffe, 1 } };
> >  
> >  	igt_fixture {
> >  		fd = drm_open_driver(DRIVER_XE);
> > @@ -170,22 +182,22 @@ igt_main
> >  					       DRM_XE_MEM_REGION_CLASS_VRAM);
> >  	}
> >  
> > -	for (int i = 0; i < ARRAY_SIZE(size); i++) {
> > -		igt_subtest_f("mem-copy-linear-0x%"PRIx64"", size[i]) {
> > +	for (int i = 0; i < ARRAY_SIZE(linear); i++) {
> > +		igt_subtest_f("mem-copy-linear-0x%x", linear[i].width) {
> >  			igt_require(blt_has_mem_copy(fd));
> >  			for_each_variation_r(regions, 1, set) {
> >  				region = igt_collection_get_value(regions, 0);
> > -				copy_test(fd, size[i], MEM_COPY, region);
> > +				copy_test(fd, &linear[i], MEM_COPY, region);
> >  			}
> >  		}
> >  	}
> >  
> > -	for (int i = 0; i < ARRAY_SIZE(size); i++) {
> > -		igt_subtest_f("mem-set-linear-0x%"PRIx64"", size[i]) {
> > +	for (int i = 0; i < ARRAY_SIZE(linear); i++) {
> > +		igt_subtest_f("mem-set-linear-0x%x", linear[i].width) {
> >  			igt_require(blt_has_mem_set(fd));
> >  			for_each_variation_r(regions, 1, set) {
> >  				region = igt_collection_get_value(regions, 0);
> > -				copy_test(fd, size[i], MEM_SET, region);
> > +				copy_test(fd, &linear[i], MEM_SET, region);
> >  			}
> >  		}
> >  	}
> > -- 
> > 2.43.0
> >