[PATCH i-g-t v4 0/5] Fill block-copy test gap for unaligned sizes

[PATCH i-g-t v4 0/5] Fill block-copy test gap for unaligned sizes

[Zbigniew Kempczyński]

Using 512x512 resolution for testing is too optimistic and hides intel_blt
limitation for using different (smaller) resolutions which also might
not be aligned to expected stride and height.

Address this by adding few helpers, change blt buffer creation size
calculation and add "increment" version of the test in xe_ccs.

v2: fix handling Tile64 what also requires bpp in dumping to
    png as helper for aligned height depend on it
v3: fix xmajor stride + make helpers public
v4: use original width/height on filename write to png,
    extend fast-copy with different sizes test

Zbigniew Kempczyński (4):
  lib/intel_blt: Add helpers for calculating stride and aligned height
  lib/intel_blt: Change surface size calculation
  lib/intel_blt: Use object pitch and aligned height on png write
  tests/xe-ccs: Add tests which exercise small to large blit sizes
  tests/xe_exercise_blt: Exercise small to large fast-copy blits

 lib/intel_blt.c                |  89 +++++++++++++++++-
 lib/intel_blt.h                |   6 +-
 tests/intel/gem_ccs.c          |  23 ++---
 tests/intel/gem_exercise_blt.c |  16 ++--
 tests/intel/xe_ccs.c           | 167 ++++++++++++++++++++++++---------
 tests/intel/xe_exercise_blt.c  |  79 ++++++++++++----
 6 files changed, 293 insertions(+), 87 deletions(-)

-- 
2.34.1

[PATCH i-g-t v4 1/5] lib/intel_blt: Add helpers for calculating stride and aligned height

[Zbigniew Kempczyński]

Tiled surfaces have stride / aligned height constraints. Currently
blt library has limitation and doesn't work properly when surface
stride is not valid for specific tiling.

As an example lets say we want to copy from linear to xmajor
33 x 33 x 32bpp surface. Xmajor surface expects stride aligned to
512 bytes and height to 8 rows so this surface will occupy 512B x 40
(128 x 40 x 32 bpp).

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Karolina Drobnik <karolina.drobnik@intel.com>
---
v2: Fix stride/aligned height calculation for Tile64
v3: Fix stride calculation for xmajor
    Make helpers public (fixes compilation warning due to lack
    or static function users)
---
 lib/intel_blt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/intel_blt.h |  4 ++++
 2 files changed, 68 insertions(+)

diff --git a/lib/intel_blt.c b/lib/intel_blt.c
index 25d251c4f8..13b1dbba4f 100644
--- a/lib/intel_blt.c
+++ b/lib/intel_blt.c
@@ -521,6 +521,70 @@ static int __block_tiling(enum blt_tiling_type tiling)
 	return 0;
 }
 
+/**
+ * blt_get_min_stride
+ * @width: width in pixels
+ * @bpp: bits per pixel
+ * @tiling: tiling
+ *
+ * Function returns minimum posibble stride in bytes for width, bpp and tiling.
+ *
+ * Returns:
+ * minimum possible stride in bytes.
+ */
+uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
+			    enum blt_tiling_type tiling)
+{
+	switch (tiling) {
+	case T_LINEAR:
+		return width * bpp / 8;
+	case T_XMAJOR:
+		return ALIGN(width * bpp / 8, 512);
+	case T_TILE64:
+		if (bpp == 8)
+			return ALIGN(width, 256);
+		else if (bpp == 16 || bpp == 32)
+			return ALIGN(width * bpp / 8, 512);
+		return ALIGN(width * bpp / 8, 1024);
+
+	default:
+		return ALIGN(width * bpp / 8, 128);
+	}
+}
+
+/**
+ * blt_get_aligned_height
+ * @height: height in pixels
+ * @bpp: bits per pixel (used for Tile64 due to different tile organization
+ * in pixels)
+ * @tiling: tiling
+ *
+ * Function returns aligned height for specific tiling. Height returned is
+ * important from memory allocation perspective, because each tiling has
+ * specific memory constraints.
+ *
+ * Returns:
+ * height (rows) expected for specific tiling
+ */
+uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
+				enum blt_tiling_type tiling)
+{
+	switch (tiling) {
+	case T_LINEAR:
+		return height;
+	case T_XMAJOR:
+		return ALIGN(height, 8);
+	case T_TILE64:
+		if (bpp == 8)
+			return ALIGN(height, 256);
+		else if (bpp == 16 || bpp == 32)
+			return ALIGN(height, 128);
+		return ALIGN(height, 64);
+	default:
+		return ALIGN(height, 32);
+	}
+}
+
 static int __special_mode(const struct blt_copy_data *blt)
 {
 	if (blt->src.handle == blt->dst.handle &&
diff --git a/lib/intel_blt.h b/lib/intel_blt.h
index d9be22fdf4..e3084dc0cd 100644
--- a/lib/intel_blt.h
+++ b/lib/intel_blt.h
@@ -212,6 +212,10 @@ bool blt_block_copy_supports_compression(int fd);
 bool blt_uses_extended_block_copy(int fd);
 
 const char *blt_tiling_name(enum blt_tiling_type tiling);
+uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
+			    enum blt_tiling_type tiling);
+uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
+				enum blt_tiling_type tiling);
 
 void blt_copy_init(int fd, struct blt_copy_data *blt);
 
-- 
2.34.1

[PATCH i-g-t v4 2/5] lib/intel_blt: Change surface size calculation

[Zbigniew Kempczyński]

For tiled surfaces we need to ensure stride and height are valid
from the blit operation perspective. Calculate required surface
size according to tiling constraints.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Karolina Drobnik <karolina.drobnik@intel.com>
---
 lib/intel_blt.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/intel_blt.c b/lib/intel_blt.c
index 13b1dbba4f..00208fc243 100644
--- a/lib/intel_blt.c
+++ b/lib/intel_blt.c
@@ -1858,13 +1858,21 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
 		  bool create_mapping)
 {
 	struct blt_copy_object *obj;
-	uint64_t size = width * height * bpp / 8;
-	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
+	uint32_t stride, aligned_height;
+	uint64_t size;
 	uint32_t handle;
 	uint8_t pat_index = DEFAULT_PAT_INDEX;
 
 	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
 
+	stride = blt_get_min_stride(width, bpp, tiling);
+	aligned_height = blt_get_aligned_height(height, bpp, tiling);
+	size = stride * aligned_height;
+
+	/* blitter command expects stride in dwords on tiled surfaces */
+	if (tiling)
+		stride /= 4;
+
 	obj = calloc(1, sizeof(*obj));
 
 	obj->size = size;
-- 
2.34.1

[PATCH i-g-t v4 3/5] lib/intel_blt: Use object pitch and aligned height on png write

[Zbigniew Kempczyński]

Pitch and buffer height on tiled surfaces might be bigger than
image width and height so dump memory to png using pitch size
instead width and aligned height instead height.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Karolina Drobnik <karolina.drobnik@intel.com>
---
v4: Use original width/height in filename instead stride/aligned
    height to avoid png overwriting (Zbigniew)
---
 lib/intel_blt.c                | 13 ++++++++++---
 lib/intel_blt.h                |  2 +-
 tests/intel/gem_ccs.c          | 23 ++++++++++++-----------
 tests/intel/gem_exercise_blt.c | 16 ++++++++--------
 tests/intel/xe_ccs.c           | 23 ++++++++++++-----------
 tests/intel/xe_exercise_blt.c  | 16 ++++++++--------
 6 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/lib/intel_blt.c b/lib/intel_blt.c
index 00208fc243..1cb5e68763 100644
--- a/lib/intel_blt.c
+++ b/lib/intel_blt.c
@@ -2076,21 +2076,28 @@ void blt_surface_info(const char *info, const struct blt_copy_object *obj)
  * @obj: blitter copy object (@blt_copy_object) to save to png
  * @width: width
  * @height: height
+ * @bpp: bits per pixel
  *
  * Function save surface to png file. Assumes ARGB format where A == 0xff.
  */
 void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
 			const struct blt_copy_object *obj,
-			uint32_t width, uint32_t height)
+			uint32_t width, uint32_t height, uint32_t bpp)
 {
 	cairo_surface_t *surface;
 	cairo_status_t ret;
+	uint32_t dump_width = width, dump_height = height;
 	uint8_t *map = (uint8_t *) obj->ptr;
 	int format;
 	int stride = obj->tiling ? obj->pitch * 4 : obj->pitch;
 	char filename[FILENAME_MAX];
 	bool is_xe = is_xe_device(fd);
 
+	if (obj->tiling) {
+		dump_width = obj->pitch;
+		dump_height = blt_get_aligned_height(height, bpp, obj->tiling);
+	}
+
 	snprintf(filename, FILENAME_MAX-1, "%d-%s-%s-%ux%u-%s.png",
 		 run_id, fileid, blt_tiling_name(obj->tiling), width, height,
 		 obj->compression ? "compressed" : "uncompressed");
@@ -2103,8 +2110,8 @@ void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
 							obj->size, PROT_READ);
 	}
 	format = CAIRO_FORMAT_RGB24;
-	surface = cairo_image_surface_create_for_data(map,
-						      format, width, height,
+	surface = cairo_image_surface_create_for_data(map, format,
+						      dump_width, dump_height,
 						      stride);
 	ret = cairo_surface_write_to_png(surface, filename);
 	if (ret)
diff --git a/lib/intel_blt.h b/lib/intel_blt.h
index e3084dc0cd..077283a088 100644
--- a/lib/intel_blt.h
+++ b/lib/intel_blt.h
@@ -315,7 +315,7 @@ void blt_surface_fill_rect(int fd, const struct blt_copy_object *obj,
 			   uint32_t width, uint32_t height);
 void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
 			const struct blt_copy_object *obj,
-			uint32_t width, uint32_t height);
+			uint32_t width, uint32_t height, uint32_t bpp);
 void blt_dump_corruption_info_32b(const struct blt_copy_object *surf1,
 				  const struct blt_copy_object *surf2);
 
diff --git a/tests/intel/gem_ccs.c b/tests/intel/gem_ccs.c
index 80a29ecab7..e8f16d7d88 100644
--- a/tests/intel/gem_ccs.c
+++ b/tests/intel/gem_ccs.c
@@ -82,9 +82,9 @@ struct test_config {
 	if (param.print_surface_info) \
 		blt_surface_info((name), (obj)); } while (0)
 
-#define WRITE_PNG(fd, id, name, obj, w, h) do { \
+#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
 	if (param.write_png) \
-		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
+		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
 
 static void surf_copy(int i915,
 		      const intel_ctx_t *ctx,
@@ -98,6 +98,7 @@ static void surf_copy(int i915,
 	struct blt_copy_data blt = {};
 	struct blt_block_copy_data_ext ext = {};
 	struct blt_ctrl_surf_copy_data surf = {};
+	const uint32_t bpp = 32;
 	uint32_t bb1, bb2, ccs, ccs2, *ccsmap, *ccsmap2;
 	uint64_t bb_size, ccssize = mid->size / CCS_RATIO(i915);
 	uint32_t *ccscopy;
@@ -172,7 +173,7 @@ static void surf_copy(int i915,
 	blt_set_batch(&blt.bb, bb2, bb_size, REGION_SMEM);
 	blt_block_copy(i915, ctx, e, ahnd, &blt, &ext);
 	gem_sync(i915, blt.dst.handle);
-	WRITE_PNG(i915, run_id, "corrupted", &blt.dst, dst->x2, dst->y2);
+	WRITE_PNG(i915, run_id, "corrupted", &blt.dst, dst->x2, dst->y2, bpp);
 	result = memcmp(src->ptr, dst->ptr, src->size);
 	igt_assert(result != 0);
 
@@ -182,7 +183,7 @@ static void surf_copy(int i915,
 
 	blt_block_copy(i915, ctx, e, ahnd, &blt, &ext);
 	gem_sync(i915, blt.dst.handle);
-	WRITE_PNG(i915, run_id, "corrected", &blt.dst, dst->x2, dst->y2);
+	WRITE_PNG(i915, run_id, "corrected", &blt.dst, dst->x2, dst->y2, bpp);
 	result = memcmp(src->ptr, dst->ptr, src->size);
 	if (result)
 		blt_dump_corruption_info_32b(src, dst);
@@ -346,7 +347,7 @@ static void block_copy(int i915,
 	PRINT_SURFACE_INFO("dst", dst);
 
 	blt_surface_fill_rect(i915, src, width, height);
-	WRITE_PNG(i915, run_id, "src", src, width, height);
+	WRITE_PNG(i915, run_id, "src", src, width, height, bpp);
 
 	blt.color_depth = CD_32bit;
 	blt.print_bb = param.print_bb;
@@ -363,7 +364,7 @@ static void block_copy(int i915,
 	if (mid->compression)
 		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
 
-	WRITE_PNG(i915, run_id, "mid", &blt.dst, width, height);
+	WRITE_PNG(i915, run_id, "mid", &blt.dst, width, height, bpp);
 
 	if (config->surfcopy && pext) {
 		const intel_ctx_t *surf_ctx = ctx;
@@ -408,7 +409,7 @@ static void block_copy(int i915,
 	blt_set_batch(&blt.bb, bb, bb_size, region1);
 	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
 	gem_sync(i915, blt.dst.handle);
-	WRITE_PNG(i915, run_id, "dst", &blt.dst, width, height);
+	WRITE_PNG(i915, run_id, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
@@ -491,11 +492,11 @@ static void block_multicopy(int i915,
 	blt_block_copy3(i915, ctx, e, ahnd, &blt3, pext3);
 	gem_sync(i915, blt3.final.handle);
 
-	WRITE_PNG(i915, run_id, "src", &blt3.src, width, height);
+	WRITE_PNG(i915, run_id, "src", &blt3.src, width, height, bpp);
 	if (!config->inplace)
-		WRITE_PNG(i915, run_id, "mid", &blt3.mid, width, height);
-	WRITE_PNG(i915, run_id, "dst", &blt3.dst, width, height);
-	WRITE_PNG(i915, run_id, "final", &blt3.final, width, height);
+		WRITE_PNG(i915, run_id, "mid", &blt3.mid, width, height, bpp);
+	WRITE_PNG(i915, run_id, "dst", &blt3.dst, width, height, bpp);
+	WRITE_PNG(i915, run_id, "final", &blt3.final, width, height, bpp);
 
 	result = memcmp(src->ptr, blt3.final.ptr, src->size);
 
diff --git a/tests/intel/gem_exercise_blt.c b/tests/intel/gem_exercise_blt.c
index 7355eabbe9..4a9bc37298 100644
--- a/tests/intel/gem_exercise_blt.c
+++ b/tests/intel/gem_exercise_blt.c
@@ -50,9 +50,9 @@ static struct param {
 	if (param.print_surface_info) \
 		blt_surface_info((name), (obj)); } while (0)
 
-#define WRITE_PNG(fd, id, name, obj, w, h) do { \
+#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
 	if (param.write_png) \
-		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
+		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
 
 struct blt_fast_copy_data {
 	int i915;
@@ -167,7 +167,7 @@ static void fast_copy_emit(int i915, const intel_ctx_t *ctx,
 	PRINT_SURFACE_INFO("dst", dst);
 
 	blt_surface_fill_rect(i915, src, width, height);
-	WRITE_PNG(i915, mid_tiling, "src", src, width, height);
+	WRITE_PNG(i915, mid_tiling, "src", src, width, height, bpp);
 
 	memset(&blt, 0, sizeof(blt));
 	blt.color_depth = CD_32bit;
@@ -180,8 +180,8 @@ static void fast_copy_emit(int i915, const intel_ctx_t *ctx,
 	fast_copy_one_bb(i915, ctx, e, ahnd, &blt);
 	gem_sync(i915, blt.dst.handle);
 
-	WRITE_PNG(i915, mid_tiling, "mid", &blt.mid, width, height);
-	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height);
+	WRITE_PNG(i915, mid_tiling, "mid", &blt.mid, width, height, bpp);
+	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
@@ -234,8 +234,8 @@ static void fast_copy(int i915, const intel_ctx_t *ctx,
 	blt_fast_copy(i915, ctx, e, ahnd, &blt);
 	gem_sync(i915, mid->handle);
 
-	WRITE_PNG(i915, mid_tiling, "src", &blt.src, width, height);
-	WRITE_PNG(i915, mid_tiling, "mid", &blt.dst, width, height);
+	WRITE_PNG(i915, mid_tiling, "src", &blt.src, width, height, bpp);
+	WRITE_PNG(i915, mid_tiling, "mid", &blt.dst, width, height, bpp);
 
 	blt_copy_init(i915, &blt);
 	blt.color_depth = CD_32bit;
@@ -247,7 +247,7 @@ static void fast_copy(int i915, const intel_ctx_t *ctx,
 	blt_fast_copy(i915, ctx, e, ahnd, &blt);
 	gem_sync(i915, blt.dst.handle);
 
-	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height);
+	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
index 7d0e8ed7a5..a7785edcb1 100644
--- a/tests/intel/xe_ccs.c
+++ b/tests/intel/xe_ccs.c
@@ -79,9 +79,9 @@ struct test_config {
 	if (param.print_surface_info) \
 		blt_surface_info((name), (obj)); } while (0)
 
-#define WRITE_PNG(fd, id, name, obj, w, h) do { \
+#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
 	if (param.write_png) \
-		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
+		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
 
 static void surf_copy(int xe,
 		      intel_ctx_t *ctx,
@@ -94,6 +94,7 @@ static void surf_copy(int xe,
 	struct blt_copy_data blt = {};
 	struct blt_block_copy_data_ext ext = {};
 	struct blt_ctrl_surf_copy_data surf = {};
+	const uint32_t bpp = 32;
 	uint32_t bb1, bb2, ccs, ccs2, *ccsmap, *ccsmap2;
 	uint64_t bb_size, ccssize = mid->size / CCS_RATIO(xe);
 	uint64_t ccs_bo_size = xe_get_default_alignment(xe);
@@ -179,7 +180,7 @@ static void surf_copy(int xe,
 	blt_set_batch(&blt.bb, bb2, bb_size, sysmem);
 	blt_block_copy(xe, ctx, NULL, ahnd, &blt, &ext);
 	intel_ctx_xe_sync(ctx, true);
-	WRITE_PNG(xe, run_id, "corrupted", &blt.dst, dst->x2, dst->y2);
+	WRITE_PNG(xe, run_id, "corrupted", &blt.dst, dst->x2, dst->y2, bpp);
 	result = memcmp(src->ptr, dst->ptr, src->size);
 	igt_assert(result != 0);
 
@@ -189,7 +190,7 @@ static void surf_copy(int xe,
 
 	blt_block_copy(xe, ctx, NULL, ahnd, &blt, &ext);
 	intel_ctx_xe_sync(ctx, true);
-	WRITE_PNG(xe, run_id, "corrected", &blt.dst, dst->x2, dst->y2);
+	WRITE_PNG(xe, run_id, "corrected", &blt.dst, dst->x2, dst->y2, bpp);
 	result = memcmp(src->ptr, dst->ptr, src->size);
 	if (result)
 		blt_dump_corruption_info_32b(src, dst);
@@ -326,7 +327,7 @@ static void block_copy(int xe,
 	PRINT_SURFACE_INFO("dst", dst);
 
 	blt_surface_fill_rect(xe, src, width, height);
-	WRITE_PNG(xe, run_id, "src", src, width, height);
+	WRITE_PNG(xe, run_id, "src", src, width, height, bpp);
 
 	blt.color_depth = CD_32bit;
 	blt.print_bb = param.print_bb;
@@ -342,7 +343,7 @@ static void block_copy(int xe,
 	if (mid->compression)
 		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
 
-	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height);
+	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
 
 	if (config->surfcopy && pext) {
 		struct drm_xe_engine_class_instance inst = {
@@ -393,7 +394,7 @@ static void block_copy(int xe,
 	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
 	intel_ctx_xe_sync(ctx, true);
 
-	WRITE_PNG(xe, run_id, "dst", &blt.dst, width, height);
+	WRITE_PNG(xe, run_id, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
@@ -486,11 +487,11 @@ static void block_multicopy(int xe,
 	blt_block_copy3(xe, ctx, ahnd, &blt3, pext3);
 	intel_ctx_xe_sync(ctx, true);
 
-	WRITE_PNG(xe, run_id, "src", &blt3.src, width, height);
+	WRITE_PNG(xe, run_id, "src", &blt3.src, width, height, bpp);
 	if (!config->inplace)
-		WRITE_PNG(xe, run_id, "mid", &blt3.mid, width, height);
-	WRITE_PNG(xe, run_id, "dst", &blt3.dst, width, height);
-	WRITE_PNG(xe, run_id, "final", &blt3.final, width, height);
+		WRITE_PNG(xe, run_id, "mid", &blt3.mid, width, height, bpp);
+	WRITE_PNG(xe, run_id, "dst", &blt3.dst, width, height, bpp);
+	WRITE_PNG(xe, run_id, "final", &blt3.final, width, height, bpp);
 
 	result = memcmp(src->ptr, blt3.final.ptr, src->size);
 
diff --git a/tests/intel/xe_exercise_blt.c b/tests/intel/xe_exercise_blt.c
index c908800cf4..ddf9d188a7 100644
--- a/tests/intel/xe_exercise_blt.c
+++ b/tests/intel/xe_exercise_blt.c
@@ -53,9 +53,9 @@ static struct param {
 	if (param.print_surface_info) \
 		blt_surface_info((name), (obj)); } while (0)
 
-#define WRITE_PNG(fd, id, name, obj, w, h) do { \
+#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
 	if (param.write_png) \
-		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
+		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
 
 struct blt_fast_copy_data {
 	int xe;
@@ -141,7 +141,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
 	PRINT_SURFACE_INFO("dst", dst);
 
 	blt_surface_fill_rect(xe, src, width, height);
-	WRITE_PNG(xe, mid_tiling, "src", src, width, height);
+	WRITE_PNG(xe, mid_tiling, "src", src, width, height, bpp);
 
 	memset(&blt, 0, sizeof(blt));
 	blt.color_depth = CD_32bit;
@@ -153,8 +153,8 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
 
 	fast_copy_one_bb(xe, ctx, ahnd, &blt);
 
-	WRITE_PNG(xe, mid_tiling, "mid", &blt.mid, width, height);
-	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height);
+	WRITE_PNG(xe, mid_tiling, "mid", &blt.mid, width, height, bpp);
+	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
@@ -205,8 +205,8 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
 
 	blt_fast_copy(xe, ctx, NULL, ahnd, &blt);
 
-	WRITE_PNG(xe, mid_tiling, "src", &blt.src, width, height);
-	WRITE_PNG(xe, mid_tiling, "mid", &blt.dst, width, height);
+	WRITE_PNG(xe, mid_tiling, "src", &blt.src, width, height, bpp);
+	WRITE_PNG(xe, mid_tiling, "mid", &blt.dst, width, height, bpp);
 
 	blt_copy_init(xe, &blt);
 	blt.color_depth = CD_32bit;
@@ -217,7 +217,7 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
 
 	blt_fast_copy(xe, ctx, NULL, ahnd, &blt);
 
-	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height);
+	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height, bpp);
 
 	result = memcmp(src->ptr, blt.dst.ptr, src->size);
 
-- 
2.34.1

[PATCH i-g-t v4 4/5] tests/xe-ccs: Add tests which exercise small to large blit sizes

[Zbigniew Kempczyński]

Testing block-copy for 512 x 512 x 32bpp is not enough to verify
blit is working for different (small) and not always aligned
resolutions. Add 'increment' subtests which checks blits from
small (1x1) to large (512x512) resolutions.

To avoid too long execution resolution increment equals 15x15 pixels.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Karolina Drobnik <karolina.drobnik@intel.com>
---
 tests/intel/xe_ccs.c | 144 +++++++++++++++++++++++++++++++++----------
 1 file changed, 112 insertions(+), 32 deletions(-)

diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
index a7785edcb1..627976049c 100644
--- a/tests/intel/xe_ccs.c
+++ b/tests/intel/xe_ccs.c
@@ -28,9 +28,15 @@
  * SUBTEST: block-copy-compressed
  * Description: Check block-copy flatccs compressed blit
  *
+ * SUBTEST: block-copy-compressed-inc-dimension
+ * Description: Check block-copy compressed blit for different sizes
+ *
  * SUBTEST: block-copy-uncompressed
  * Description: Check block-copy uncompressed blit
  *
+ * SUBTEST: block-copy-uncompressed-inc-dimension
+ * Description: Check block-copy uncompressed blit for different sizes
+ *
  * SUBTEST: block-multicopy-compressed
  * Description: Check block-multicopy flatccs compressed blit
  *
@@ -73,6 +79,8 @@ struct test_config {
 	bool surfcopy;
 	bool new_ctx;
 	bool suspend_resume;
+	int width_increment;
+	int width_steps;
 };
 
 #define PRINT_SURFACE_INFO(name, obj) do { \
@@ -286,9 +294,17 @@ static int blt_block_copy3(int xe,
 	return ret;
 }
 
+#define CHECK_MIN_WIDTH 2
+#define CHECK_MIN_HEIGHT 2
+#define MIN_EXP_WH(w, h) ((w) >= CHECK_MIN_WIDTH && (h) >= CHECK_MIN_HEIGHT)
+#define CHECK_FROM_WIDTH 256
+#define CHECK_FROM_HEIGHT 256
+#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
+
 static void block_copy(int xe,
 		       intel_ctx_t *ctx,
 		       uint32_t region1, uint32_t region2,
+		       uint32_t width, uint32_t height,
 		       enum blt_tiling_type mid_tiling,
 		       const struct test_config *config)
 {
@@ -301,7 +317,7 @@ static void block_copy(int xe,
 	uint32_t run_id = mid_tiling;
 	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
 							!xe_has_vram(xe)) ? region1 : region2;
-	uint32_t width = param.width, height = param.height, bb;
+	uint32_t bb;
 	enum blt_compression mid_compression = config->compression;
 	int mid_compression_format = param.compression_format;
 	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
@@ -339,9 +355,16 @@ static void block_copy(int xe,
 	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
 	intel_ctx_xe_sync(ctx, true);
 
-	/* We expect mid != src if there's compression */
-	if (mid->compression)
-		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
+	/*
+	 * We expect mid != src if there's compression. Ignore this for small
+	 * width x height for linear as compression for gradient occurs in the
+	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
+	 * xmajor.
+	 */
+	if (mid->compression && MIN_EXP_WH(width, height)) {
+		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
+			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
+	}
 
 	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
 
@@ -416,6 +439,7 @@ static void block_copy(int xe,
 static void block_multicopy(int xe,
 			    intel_ctx_t *ctx,
 			    uint32_t region1, uint32_t region2,
+			    uint32_t width, uint32_t height,
 			    enum blt_tiling_type mid_tiling,
 			    const struct test_config *config)
 {
@@ -429,7 +453,7 @@ static void block_multicopy(int xe,
 	uint32_t run_id = mid_tiling;
 	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
 							!xe_has_vram(xe)) ? region1 : region2;
-	uint32_t width = param.width, height = param.height, bb;
+	uint32_t bb;
 	enum blt_compression mid_compression = config->compression;
 	int mid_compression_format = param.compression_format;
 	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
@@ -521,6 +545,7 @@ static const struct {
 	void (*copyfn)(int fd,
 		       intel_ctx_t *ctx,
 		       uint32_t region1, uint32_t region2,
+		       uint32_t width, uint32_t height,
 		       enum blt_tiling_type btype,
 		       const struct test_config *config);
 } copyfns[] = {
@@ -528,17 +553,44 @@ static const struct {
 	[BLOCK_MULTICOPY] = { "-multicopy", block_multicopy },
 };
 
-static void block_copy_test(int xe,
-			    const struct test_config *config,
-			    struct igt_collection *set,
-			    enum copy_func copy_function)
+static void single_copy(int xe, const struct test_config *config,
+			int32_t region1, uint32_t region2,
+			uint32_t width, uint32_t height,
+			int tiling, enum copy_func copy_function)
 {
 	struct drm_xe_engine_class_instance inst = {
 		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
 	};
+	uint32_t vm, exec_queue;
+	uint32_t sync_bind, sync_out;
 	intel_ctx_t *ctx;
+
+	vm = xe_vm_create(xe, 0, 0);
+	exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
+	sync_bind = syncobj_create(xe, 0);
+	sync_out = syncobj_create(xe, 0);
+	ctx = intel_ctx_xe(xe, vm, exec_queue,
+			   0, sync_bind, sync_out);
+
+	copyfns[copy_function].copyfn(xe, ctx,
+				      region1, region2,
+				      width, height,
+				      tiling, config);
+
+	xe_exec_queue_destroy(xe, exec_queue);
+	xe_vm_destroy(xe, vm);
+	syncobj_destroy(xe, sync_bind);
+	syncobj_destroy(xe, sync_out);
+	free(ctx);
+}
+
+static void block_copy_test(int xe,
+			    const struct test_config *config,
+			    struct igt_collection *set,
+			    enum copy_func copy_function)
+{
+
 	struct igt_collection *regions;
-	uint32_t vm, exec_queue;
 	int tiling;
 
 	if (config->compression && !blt_block_copy_supports_compression(xe))
@@ -555,6 +607,7 @@ static void block_copy_test(int xe,
 		for_each_variation_r(regions, 2, set) {
 			uint32_t region1, region2;
 			char *regtxt;
+			char testname[256];
 
 			region1 = igt_collection_get_value(regions, 0);
 			region2 = igt_collection_get_value(regions, 1);
@@ -566,30 +619,36 @@ static void block_copy_test(int xe,
 
 			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
 
-			igt_dynamic_f("%s-%s-compfmt%d-%s%s",
-				      blt_tiling_name(tiling),
-				      config->compression ?
-					      "compressed" : "uncompressed",
-				      param.compression_format, regtxt,
-				      copyfns[copy_function].suffix) {
-				uint32_t sync_bind, sync_out;
+			snprintf(testname, sizeof(testname),
+				 "%s-%s-compfmt%d-%s%s",
+				 blt_tiling_name(tiling),
+				 config->compression ?
+					 "compressed" : "uncompressed",
+				 param.compression_format, regtxt,
+				 copyfns[copy_function].suffix);
 
-				vm = xe_vm_create(xe, 0, 0);
-				exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
-				sync_bind = syncobj_create(xe, 0);
-				sync_out = syncobj_create(xe, 0);
-				ctx = intel_ctx_xe(xe, vm, exec_queue,
-						   0, sync_bind, sync_out);
+			if (!config->width_increment) {
+				igt_dynamic(testname)
+					single_copy(xe, config, region1, region2,
+						    param.width, param.height,
+						    tiling, copy_function);
+			} else {
+				for (int w = param.width;
+				     w < param.width + config->width_steps;
+				     w += config->width_increment) {
+					snprintf(testname, sizeof(testname),
+						 "%s-%s-compfmt%d-%s%s-%dx%d",
+						 blt_tiling_name(tiling),
+						 config->compression ?
+							 "compressed" : "uncompressed",
+						 param.compression_format, regtxt,
+						 copyfns[copy_function].suffix,
+						 w, w);
+					igt_dynamic(testname)
+						single_copy(xe, config, region1, region2,
+							    w, w, tiling, copy_function);
+				}
 
-				copyfns[copy_function].copyfn(xe, ctx,
-							      region1, region2,
-							      tiling, config);
-
-				xe_exec_queue_destroy(xe, exec_queue);
-				xe_vm_destroy(xe, vm);
-				syncobj_destroy(xe, sync_bind);
-				syncobj_destroy(xe, sync_out);
-				free(ctx);
 			}
 
 			free(regtxt);
@@ -669,6 +728,16 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
 		block_copy_test(xe, &config, set, BLOCK_COPY);
 	}
 
+	igt_describe("Check block-copy uncompressed blit with increment width/height");
+	igt_subtest_with_dynamic("block-copy-uncompressed-inc-dimension") {
+		struct test_config config = { .width_increment = 15,
+					      .width_steps = 512 };
+		param.width = 1;
+		param.height = 1;
+
+		block_copy_test(xe, &config, set, BLOCK_COPY);
+	}
+
 	igt_describe("Check block-copy flatccs compressed blit");
 	igt_subtest_with_dynamic("block-copy-compressed") {
 		struct test_config config = { .compression = true };
@@ -676,6 +745,17 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
 		block_copy_test(xe, &config, set, BLOCK_COPY);
 	}
 
+	igt_describe("Check block-copy compressed blit with increment width/height");
+	igt_subtest_with_dynamic("block-copy-compressed-inc-dimension") {
+		struct test_config config = { .compression = true,
+					      .width_increment = 15,
+					      .width_steps = 512 };
+		param.width = 1;
+		param.height = 1;
+
+		block_copy_test(xe, &config, set, BLOCK_COPY);
+	}
+
 	igt_describe("Check block-multicopy flatccs compressed blit");
 	igt_subtest_with_dynamic("block-multicopy-compressed") {
 		struct test_config config = { .compression = true };
-- 
2.34.1

[PATCH i-g-t v4 5/5] tests/xe_exercise_blt: Exercise small to large fast-copy blits

[Zbigniew Kempczyński]

Testing blitter with large size like 512x512 may be not enough
to verify small or unaligned blits works properly. Add subtest
which spread fast-copy blits from small to large.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Karolina Drobnik <karolina.drobnik@intel.com>
---
 tests/intel/xe_exercise_blt.c | 63 +++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/tests/intel/xe_exercise_blt.c b/tests/intel/xe_exercise_blt.c
index ddf9d188a7..8b4bae282b 100644
--- a/tests/intel/xe_exercise_blt.c
+++ b/tests/intel/xe_exercise_blt.c
@@ -25,6 +25,10 @@
  *   Check fast-copy blit
  *   blitter
  *
+ * SUBTEST: fast-copy-inc-dimension
+ * Description:
+ *   Check fast-copy blit with sizes from small to large
+ *
  * SUBTEST: fast-copy-emit
  * Description:
  *   Check multiple fast-copy in one batch
@@ -40,6 +44,8 @@ static struct param {
 	bool print_surface_info;
 	int width;
 	int height;
+	int width_increment;
+	int width_steps;
 } param = {
 	.tiling = -1,
 	.write_png = false,
@@ -111,6 +117,7 @@ static int fast_copy_one_bb(int xe,
 }
 
 static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
+			   uint32_t width, uint32_t height,
 			   uint32_t region1, uint32_t region2,
 			   enum blt_tiling_type mid_tiling)
 {
@@ -122,7 +129,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
 	uint64_t ahnd = intel_allocator_open_full(xe, ctx->vm, 0, 0,
 						  INTEL_ALLOCATOR_SIMPLE,
 						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
-	uint32_t bb, width = param.width, height = param.height;
+	uint32_t bb;
 	int result;
 
 	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
@@ -170,6 +177,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
 }
 
 static void fast_copy(int xe, const intel_ctx_t *ctx,
+		      uint32_t width, uint32_t height,
 		      uint32_t region1, uint32_t region2,
 		      enum blt_tiling_type mid_tiling)
 {
@@ -181,7 +189,6 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
 						  INTEL_ALLOCATOR_SIMPLE,
 						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
 	uint32_t bb;
-	uint32_t width = param.width, height = param.height;
 	int result;
 
 	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
@@ -241,14 +248,20 @@ enum fast_copy_func {
 };
 
 static char
-	*full_subtest_str(char *regtxt, enum blt_tiling_type tiling,
+	*full_subtest_str(char *regtxt, uint32_t width, uint32_t height,
+			  enum blt_tiling_type tiling,
 			  enum fast_copy_func func)
 {
 	char *name;
 	uint32_t len;
 
-	len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
-		       func == FAST_COPY_EMIT ? "-emit" : "");
+	if (!width || !height)
+		len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
+			       func == FAST_COPY_EMIT ? "-emit" : "");
+	else
+		len = asprintf(&name, "%s-%s%s-%ux%u", blt_tiling_name(tiling), regtxt,
+			       func == FAST_COPY_EMIT ? "-emit" : "",
+			       width, height);
 
 	igt_assert_f(len >= 0, "asprintf failed!\n");
 
@@ -264,6 +277,7 @@ static void fast_copy_test(int xe,
 	};
 	struct igt_collection *regions;
 	void (*copy_func)(int xe, const intel_ctx_t *ctx,
+			  uint32_t width, uint32_t height,
 			  uint32_t r1, uint32_t r2, enum blt_tiling_type tiling);
 	intel_ctx_t *ctx;
 	int tiling;
@@ -286,16 +300,32 @@ static void fast_copy_test(int xe,
 
 			copy_func = (func == FAST_COPY) ? fast_copy : fast_copy_emit;
 			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
-			test_name = full_subtest_str(regtxt, tiling, func);
 
-			igt_dynamic_f("%s", test_name) {
-				copy_func(xe, ctx,
-					  region1, region2,
-					  tiling);
+			if (!param.width_increment) {
+				test_name = full_subtest_str(regtxt, 0, 0, tiling, func);
+				igt_dynamic_f("%s", test_name) {
+					copy_func(xe, ctx,
+						  param.width, param.height,
+						  region1, region2,
+						  tiling);
+				}
+				free(test_name);
+			} else {
+				for (int w = param.width;
+				     w < param.width + param.width_steps;
+				     w += param.width_increment) {
+					test_name = full_subtest_str(regtxt, w, w, tiling, func);
+					igt_dynamic_f("%s", test_name) {
+						copy_func(xe, ctx,
+							  w, w,
+							  region1, region2,
+							  tiling);
+					}
+					free(test_name);
+				}
 			}
 
 			free(regtxt);
-			free(test_name);
 			xe_exec_queue_destroy(xe, exec_queue);
 			xe_vm_destroy(xe, vm);
 			free(ctx);
@@ -367,6 +397,17 @@ igt_main_args("b:pst:W:H:", NULL, help_str, opt_handler, NULL)
 		fast_copy_test(xe, set, FAST_COPY);
 	}
 
+	igt_describe("Check fast-copy with increment width/height");
+	igt_subtest_with_dynamic("fast-copy-inc-dimension") {
+		param.width = 1;
+		param.height = 1;
+		param.width_increment = 15;
+		param.width_steps = 512;
+
+		fast_copy_test(xe, set, FAST_COPY);
+	}
+
+
 	igt_describe("Check multiple fast-copy in one batch");
 	igt_subtest_with_dynamic("fast-copy-emit") {
 		fast_copy_test(xe, set, FAST_COPY_EMIT);
-- 
2.34.1

✓ CI.xeBAT: success for Fill block-copy test gap for unaligned sizes (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

== Series Details ==

Series: Fill block-copy test gap for unaligned sizes (rev4)
URL   : https://patchwork.freedesktop.org/series/129362/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7700_BAT -> XEIGTPW_10621_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (4 -> 4)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in XEIGTPW_10621_BAT that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-3:
    - bat-dg2-oem2:       [PASS][1] -> [FAIL][2] ([Intel XE#400] / [Intel XE#616]) +1 other test fail
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7700/bat-dg2-oem2/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-3.html
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10621/bat-dg2-oem2/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-3.html

  
  [Intel XE#400]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/400
  [Intel XE#616]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/616


Build changes
-------------

  * IGT: IGT_7700 -> IGTPW_10621
  * Linux: xe-712-cba66c6a2af4df1b9b420fbad0a9ac1e68f14030 -> xe-714-bc72404452d402682457b998a9884af537adddd2

  IGTPW_10621: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10621/index.html
  IGT_7700: 7700
  xe-712-cba66c6a2af4df1b9b420fbad0a9ac1e68f14030: cba66c6a2af4df1b9b420fbad0a9ac1e68f14030
  xe-714-bc72404452d402682457b998a9884af537adddd2: bc72404452d402682457b998a9884af537adddd2

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10621/index.html

[-- Attachment #2: Type: text/html, Size: 2336 bytes --]

Re: [PATCH i-g-t v4 1/5] lib/intel_blt: Add helpers for calculating stride and aligned height

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> Tiled surfaces have stride / aligned height constraints. Currently
> blt library has limitation and doesn't work properly when surface
> stride is not valid for specific tiling.
> 
> As an example lets say we want to copy from linear to xmajor
> 33 x 33 x 32bpp surface. Xmajor surface expects stride aligned to
> 512 bytes and height to 8 rows so this surface will occupy 512B x 40
> (128 x 40 x 32 bpp).
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>

Just a bunch of nits, nothing major.

> ---
> v2: Fix stride/aligned height calculation for Tile64
> v3: Fix stride calculation for xmajor
>      Make helpers public (fixes compilation warning due to lack
>      or static function users)
> ---
>   lib/intel_blt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/intel_blt.h |  4 ++++
>   2 files changed, 68 insertions(+)
> 
> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> index 25d251c4f8..13b1dbba4f 100644
> --- a/lib/intel_blt.c
> +++ b/lib/intel_blt.c
> @@ -521,6 +521,70 @@ static int __block_tiling(enum blt_tiling_type tiling)
>   	return 0;
>   }
>   
> +/**
> + * blt_get_min_stride
> + * @width: width in pixels
> + * @bpp: bits per pixel
> + * @tiling: tiling
> + *
> + * Function returns minimum posibble stride in bytes for width, bpp and tiling.

"return" is already described, we could say "calculates minimum possible
stride..."

> + *
> + * Returns:
> + * minimum possible stride in bytes.
> + */
> +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> +			    enum blt_tiling_type tiling)
> +{
> +	switch (tiling) {
> +	case T_LINEAR:
> +		return width * bpp / 8;
> +	case T_XMAJOR:
> +		return ALIGN(width * bpp / 8, 512);
> +	case T_TILE64:
> +		if (bpp == 8)
> +			return ALIGN(width, 256);
> +		else if (bpp == 16 || bpp == 32)
> +			return ALIGN(width * bpp / 8, 512);
> +		return ALIGN(width * bpp / 8, 1024);
> +
> +	default:
> +		return ALIGN(width * bpp / 8, 128);
> +	}
> +}
> +
> +/**
> + * blt_get_aligned_height
> + * @height: height in pixels
> + * @bpp: bits per pixel (used for Tile64 due to different tile organization
> + * in pixels)
> + * @tiling: tiling
> + *
> + * Function returns aligned height for specific tiling. Height returned is
> + * important from memory allocation perspective, because each tiling has
> + * specific memory constraints.

I'd move the comment on Tile64 to the description body and integrate it
with the last sentence. You could say that each tiling has constrains
dependent on bpp and mention a different pixel order for Tile64 as an
example.

Apart from that, it's looking good to me:

Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

> + *
> + * Returns:
> + * height (rows) expected for specific tiling
> + */
> +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> +				enum blt_tiling_type tiling)
> +{
> +	switch (tiling) {
> +	case T_LINEAR:
> +		return height;
> +	case T_XMAJOR:
> +		return ALIGN(height, 8);
> +	case T_TILE64:
> +		if (bpp == 8)
> +			return ALIGN(height, 256);
> +		else if (bpp == 16 || bpp == 32)
> +			return ALIGN(height, 128);
> +		return ALIGN(height, 64);
> +	default:
> +		return ALIGN(height, 32);
> +	}
> +}
> +
>   static int __special_mode(const struct blt_copy_data *blt)
>   {
>   	if (blt->src.handle == blt->dst.handle &&
> diff --git a/lib/intel_blt.h b/lib/intel_blt.h
> index d9be22fdf4..e3084dc0cd 100644
> --- a/lib/intel_blt.h
> +++ b/lib/intel_blt.h
> @@ -212,6 +212,10 @@ bool blt_block_copy_supports_compression(int fd);
>   bool blt_uses_extended_block_copy(int fd);
>   
>   const char *blt_tiling_name(enum blt_tiling_type tiling);
> +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> +			    enum blt_tiling_type tiling);
> +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> +				enum blt_tiling_type tiling);
>   
>   void blt_copy_init(int fd, struct blt_copy_data *blt);
>   

Re: [PATCH i-g-t v4 2/5] lib/intel_blt: Change surface size calculation

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> For tiled surfaces we need to ensure stride and height are valid
> from the blit operation perspective. Calculate required surface
> size according to tiling constraints.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>

I'm looking at blt_set_geom() function. We still pass normal height and
width to it. Is this correct? Or should we change height to
aligned_height? Because if we should, this patch should be updated to
reflect that.

If that's not a problem, then:

Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

> ---
>   lib/intel_blt.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> index 13b1dbba4f..00208fc243 100644
> --- a/lib/intel_blt.c
> +++ b/lib/intel_blt.c
> @@ -1858,13 +1858,21 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
>   		  bool create_mapping)
>   {
>   	struct blt_copy_object *obj;
> -	uint64_t size = width * height * bpp / 8;
> -	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
> +	uint32_t stride, aligned_height;
> +	uint64_t size;
>   	uint32_t handle;
>   	uint8_t pat_index = DEFAULT_PAT_INDEX;
>   
>   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
>   
> +	stride = blt_get_min_stride(width, bpp, tiling);
> +	aligned_height = blt_get_aligned_height(height, bpp, tiling);
> +	size = stride * aligned_height;
> +
> +	/* blitter command expects stride in dwords on tiled surfaces */
> +	if (tiling)
> +		stride /= 4;
> +
>   	obj = calloc(1, sizeof(*obj));
>   
>   	obj->size = size;

Re: [PATCH i-g-t v4 3/5] lib/intel_blt: Use object pitch and aligned height on png write

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> Pitch and buffer height on tiled surfaces might be bigger than
> image width and height so dump memory to png using pitch size
> instead width and aligned height instead height.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> ---
> v4: Use original width/height in filename instead stride/aligned
>      height to avoid png overwriting (Zbigniew)

Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

> ---
>   lib/intel_blt.c                | 13 ++++++++++---
>   lib/intel_blt.h                |  2 +-
>   tests/intel/gem_ccs.c          | 23 ++++++++++++-----------
>   tests/intel/gem_exercise_blt.c | 16 ++++++++--------
>   tests/intel/xe_ccs.c           | 23 ++++++++++++-----------
>   tests/intel/xe_exercise_blt.c  | 16 ++++++++--------
>   6 files changed, 51 insertions(+), 42 deletions(-)
> 
> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> index 00208fc243..1cb5e68763 100644
> --- a/lib/intel_blt.c
> +++ b/lib/intel_blt.c
> @@ -2076,21 +2076,28 @@ void blt_surface_info(const char *info, const struct blt_copy_object *obj)
>    * @obj: blitter copy object (@blt_copy_object) to save to png
>    * @width: width
>    * @height: height
> + * @bpp: bits per pixel
>    *
>    * Function save surface to png file. Assumes ARGB format where A == 0xff.
>    */
>   void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
>   			const struct blt_copy_object *obj,
> -			uint32_t width, uint32_t height)
> +			uint32_t width, uint32_t height, uint32_t bpp)
>   {
>   	cairo_surface_t *surface;
>   	cairo_status_t ret;
> +	uint32_t dump_width = width, dump_height = height;
>   	uint8_t *map = (uint8_t *) obj->ptr;
>   	int format;
>   	int stride = obj->tiling ? obj->pitch * 4 : obj->pitch;
>   	char filename[FILENAME_MAX];
>   	bool is_xe = is_xe_device(fd);
>   
> +	if (obj->tiling) {
> +		dump_width = obj->pitch;
> +		dump_height = blt_get_aligned_height(height, bpp, obj->tiling);
> +	}
> +
>   	snprintf(filename, FILENAME_MAX-1, "%d-%s-%s-%ux%u-%s.png",
>   		 run_id, fileid, blt_tiling_name(obj->tiling), width, height,
>   		 obj->compression ? "compressed" : "uncompressed");
> @@ -2103,8 +2110,8 @@ void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
>   							obj->size, PROT_READ);
>   	}
>   	format = CAIRO_FORMAT_RGB24;
> -	surface = cairo_image_surface_create_for_data(map,
> -						      format, width, height,
> +	surface = cairo_image_surface_create_for_data(map, format,
> +						      dump_width, dump_height,
>   						      stride);
>   	ret = cairo_surface_write_to_png(surface, filename);
>   	if (ret)
> diff --git a/lib/intel_blt.h b/lib/intel_blt.h
> index e3084dc0cd..077283a088 100644
> --- a/lib/intel_blt.h
> +++ b/lib/intel_blt.h
> @@ -315,7 +315,7 @@ void blt_surface_fill_rect(int fd, const struct blt_copy_object *obj,
>   			   uint32_t width, uint32_t height);
>   void blt_surface_to_png(int fd, uint32_t run_id, const char *fileid,
>   			const struct blt_copy_object *obj,
> -			uint32_t width, uint32_t height);
> +			uint32_t width, uint32_t height, uint32_t bpp);
>   void blt_dump_corruption_info_32b(const struct blt_copy_object *surf1,
>   				  const struct blt_copy_object *surf2);
>   
> diff --git a/tests/intel/gem_ccs.c b/tests/intel/gem_ccs.c
> index 80a29ecab7..e8f16d7d88 100644
> --- a/tests/intel/gem_ccs.c
> +++ b/tests/intel/gem_ccs.c
> @@ -82,9 +82,9 @@ struct test_config {
>   	if (param.print_surface_info) \
>   		blt_surface_info((name), (obj)); } while (0)
>   
> -#define WRITE_PNG(fd, id, name, obj, w, h) do { \
> +#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
>   	if (param.write_png) \
> -		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
> +		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
>   
>   static void surf_copy(int i915,
>   		      const intel_ctx_t *ctx,
> @@ -98,6 +98,7 @@ static void surf_copy(int i915,
>   	struct blt_copy_data blt = {};
>   	struct blt_block_copy_data_ext ext = {};
>   	struct blt_ctrl_surf_copy_data surf = {};
> +	const uint32_t bpp = 32;
>   	uint32_t bb1, bb2, ccs, ccs2, *ccsmap, *ccsmap2;
>   	uint64_t bb_size, ccssize = mid->size / CCS_RATIO(i915);
>   	uint32_t *ccscopy;
> @@ -172,7 +173,7 @@ static void surf_copy(int i915,
>   	blt_set_batch(&blt.bb, bb2, bb_size, REGION_SMEM);
>   	blt_block_copy(i915, ctx, e, ahnd, &blt, &ext);
>   	gem_sync(i915, blt.dst.handle);
> -	WRITE_PNG(i915, run_id, "corrupted", &blt.dst, dst->x2, dst->y2);
> +	WRITE_PNG(i915, run_id, "corrupted", &blt.dst, dst->x2, dst->y2, bpp);
>   	result = memcmp(src->ptr, dst->ptr, src->size);
>   	igt_assert(result != 0);
>   
> @@ -182,7 +183,7 @@ static void surf_copy(int i915,
>   
>   	blt_block_copy(i915, ctx, e, ahnd, &blt, &ext);
>   	gem_sync(i915, blt.dst.handle);
> -	WRITE_PNG(i915, run_id, "corrected", &blt.dst, dst->x2, dst->y2);
> +	WRITE_PNG(i915, run_id, "corrected", &blt.dst, dst->x2, dst->y2, bpp);
>   	result = memcmp(src->ptr, dst->ptr, src->size);
>   	if (result)
>   		blt_dump_corruption_info_32b(src, dst);
> @@ -346,7 +347,7 @@ static void block_copy(int i915,
>   	PRINT_SURFACE_INFO("dst", dst);
>   
>   	blt_surface_fill_rect(i915, src, width, height);
> -	WRITE_PNG(i915, run_id, "src", src, width, height);
> +	WRITE_PNG(i915, run_id, "src", src, width, height, bpp);
>   
>   	blt.color_depth = CD_32bit;
>   	blt.print_bb = param.print_bb;
> @@ -363,7 +364,7 @@ static void block_copy(int i915,
>   	if (mid->compression)
>   		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
>   
> -	WRITE_PNG(i915, run_id, "mid", &blt.dst, width, height);
> +	WRITE_PNG(i915, run_id, "mid", &blt.dst, width, height, bpp);
>   
>   	if (config->surfcopy && pext) {
>   		const intel_ctx_t *surf_ctx = ctx;
> @@ -408,7 +409,7 @@ static void block_copy(int i915,
>   	blt_set_batch(&blt.bb, bb, bb_size, region1);
>   	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
>   	gem_sync(i915, blt.dst.handle);
> -	WRITE_PNG(i915, run_id, "dst", &blt.dst, width, height);
> +	WRITE_PNG(i915, run_id, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   
> @@ -491,11 +492,11 @@ static void block_multicopy(int i915,
>   	blt_block_copy3(i915, ctx, e, ahnd, &blt3, pext3);
>   	gem_sync(i915, blt3.final.handle);
>   
> -	WRITE_PNG(i915, run_id, "src", &blt3.src, width, height);
> +	WRITE_PNG(i915, run_id, "src", &blt3.src, width, height, bpp);
>   	if (!config->inplace)
> -		WRITE_PNG(i915, run_id, "mid", &blt3.mid, width, height);
> -	WRITE_PNG(i915, run_id, "dst", &blt3.dst, width, height);
> -	WRITE_PNG(i915, run_id, "final", &blt3.final, width, height);
> +		WRITE_PNG(i915, run_id, "mid", &blt3.mid, width, height, bpp);
> +	WRITE_PNG(i915, run_id, "dst", &blt3.dst, width, height, bpp);
> +	WRITE_PNG(i915, run_id, "final", &blt3.final, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt3.final.ptr, src->size);
>   
> diff --git a/tests/intel/gem_exercise_blt.c b/tests/intel/gem_exercise_blt.c
> index 7355eabbe9..4a9bc37298 100644
> --- a/tests/intel/gem_exercise_blt.c
> +++ b/tests/intel/gem_exercise_blt.c
> @@ -50,9 +50,9 @@ static struct param {
>   	if (param.print_surface_info) \
>   		blt_surface_info((name), (obj)); } while (0)
>   
> -#define WRITE_PNG(fd, id, name, obj, w, h) do { \
> +#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
>   	if (param.write_png) \
> -		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
> +		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
>   
>   struct blt_fast_copy_data {
>   	int i915;
> @@ -167,7 +167,7 @@ static void fast_copy_emit(int i915, const intel_ctx_t *ctx,
>   	PRINT_SURFACE_INFO("dst", dst);
>   
>   	blt_surface_fill_rect(i915, src, width, height);
> -	WRITE_PNG(i915, mid_tiling, "src", src, width, height);
> +	WRITE_PNG(i915, mid_tiling, "src", src, width, height, bpp);
>   
>   	memset(&blt, 0, sizeof(blt));
>   	blt.color_depth = CD_32bit;
> @@ -180,8 +180,8 @@ static void fast_copy_emit(int i915, const intel_ctx_t *ctx,
>   	fast_copy_one_bb(i915, ctx, e, ahnd, &blt);
>   	gem_sync(i915, blt.dst.handle);
>   
> -	WRITE_PNG(i915, mid_tiling, "mid", &blt.mid, width, height);
> -	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height);
> +	WRITE_PNG(i915, mid_tiling, "mid", &blt.mid, width, height, bpp);
> +	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   
> @@ -234,8 +234,8 @@ static void fast_copy(int i915, const intel_ctx_t *ctx,
>   	blt_fast_copy(i915, ctx, e, ahnd, &blt);
>   	gem_sync(i915, mid->handle);
>   
> -	WRITE_PNG(i915, mid_tiling, "src", &blt.src, width, height);
> -	WRITE_PNG(i915, mid_tiling, "mid", &blt.dst, width, height);
> +	WRITE_PNG(i915, mid_tiling, "src", &blt.src, width, height, bpp);
> +	WRITE_PNG(i915, mid_tiling, "mid", &blt.dst, width, height, bpp);
>   
>   	blt_copy_init(i915, &blt);
>   	blt.color_depth = CD_32bit;
> @@ -247,7 +247,7 @@ static void fast_copy(int i915, const intel_ctx_t *ctx,
>   	blt_fast_copy(i915, ctx, e, ahnd, &blt);
>   	gem_sync(i915, blt.dst.handle);
>   
> -	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height);
> +	WRITE_PNG(i915, mid_tiling, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   
> diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> index 7d0e8ed7a5..a7785edcb1 100644
> --- a/tests/intel/xe_ccs.c
> +++ b/tests/intel/xe_ccs.c
> @@ -79,9 +79,9 @@ struct test_config {
>   	if (param.print_surface_info) \
>   		blt_surface_info((name), (obj)); } while (0)
>   
> -#define WRITE_PNG(fd, id, name, obj, w, h) do { \
> +#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
>   	if (param.write_png) \
> -		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
> +		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
>   
>   static void surf_copy(int xe,
>   		      intel_ctx_t *ctx,
> @@ -94,6 +94,7 @@ static void surf_copy(int xe,
>   	struct blt_copy_data blt = {};
>   	struct blt_block_copy_data_ext ext = {};
>   	struct blt_ctrl_surf_copy_data surf = {};
> +	const uint32_t bpp = 32;
>   	uint32_t bb1, bb2, ccs, ccs2, *ccsmap, *ccsmap2;
>   	uint64_t bb_size, ccssize = mid->size / CCS_RATIO(xe);
>   	uint64_t ccs_bo_size = xe_get_default_alignment(xe);
> @@ -179,7 +180,7 @@ static void surf_copy(int xe,
>   	blt_set_batch(&blt.bb, bb2, bb_size, sysmem);
>   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, &ext);
>   	intel_ctx_xe_sync(ctx, true);
> -	WRITE_PNG(xe, run_id, "corrupted", &blt.dst, dst->x2, dst->y2);
> +	WRITE_PNG(xe, run_id, "corrupted", &blt.dst, dst->x2, dst->y2, bpp);
>   	result = memcmp(src->ptr, dst->ptr, src->size);
>   	igt_assert(result != 0);
>   
> @@ -189,7 +190,7 @@ static void surf_copy(int xe,
>   
>   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, &ext);
>   	intel_ctx_xe_sync(ctx, true);
> -	WRITE_PNG(xe, run_id, "corrected", &blt.dst, dst->x2, dst->y2);
> +	WRITE_PNG(xe, run_id, "corrected", &blt.dst, dst->x2, dst->y2, bpp);
>   	result = memcmp(src->ptr, dst->ptr, src->size);
>   	if (result)
>   		blt_dump_corruption_info_32b(src, dst);
> @@ -326,7 +327,7 @@ static void block_copy(int xe,
>   	PRINT_SURFACE_INFO("dst", dst);
>   
>   	blt_surface_fill_rect(xe, src, width, height);
> -	WRITE_PNG(xe, run_id, "src", src, width, height);
> +	WRITE_PNG(xe, run_id, "src", src, width, height, bpp);
>   
>   	blt.color_depth = CD_32bit;
>   	blt.print_bb = param.print_bb;
> @@ -342,7 +343,7 @@ static void block_copy(int xe,
>   	if (mid->compression)
>   		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
>   
> -	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height);
> +	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
>   
>   	if (config->surfcopy && pext) {
>   		struct drm_xe_engine_class_instance inst = {
> @@ -393,7 +394,7 @@ static void block_copy(int xe,
>   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
>   	intel_ctx_xe_sync(ctx, true);
>   
> -	WRITE_PNG(xe, run_id, "dst", &blt.dst, width, height);
> +	WRITE_PNG(xe, run_id, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   
> @@ -486,11 +487,11 @@ static void block_multicopy(int xe,
>   	blt_block_copy3(xe, ctx, ahnd, &blt3, pext3);
>   	intel_ctx_xe_sync(ctx, true);
>   
> -	WRITE_PNG(xe, run_id, "src", &blt3.src, width, height);
> +	WRITE_PNG(xe, run_id, "src", &blt3.src, width, height, bpp);
>   	if (!config->inplace)
> -		WRITE_PNG(xe, run_id, "mid", &blt3.mid, width, height);
> -	WRITE_PNG(xe, run_id, "dst", &blt3.dst, width, height);
> -	WRITE_PNG(xe, run_id, "final", &blt3.final, width, height);
> +		WRITE_PNG(xe, run_id, "mid", &blt3.mid, width, height, bpp);
> +	WRITE_PNG(xe, run_id, "dst", &blt3.dst, width, height, bpp);
> +	WRITE_PNG(xe, run_id, "final", &blt3.final, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt3.final.ptr, src->size);
>   
> diff --git a/tests/intel/xe_exercise_blt.c b/tests/intel/xe_exercise_blt.c
> index c908800cf4..ddf9d188a7 100644
> --- a/tests/intel/xe_exercise_blt.c
> +++ b/tests/intel/xe_exercise_blt.c
> @@ -53,9 +53,9 @@ static struct param {
>   	if (param.print_surface_info) \
>   		blt_surface_info((name), (obj)); } while (0)
>   
> -#define WRITE_PNG(fd, id, name, obj, w, h) do { \
> +#define WRITE_PNG(fd, id, name, obj, w, h, bpp) do { \
>   	if (param.write_png) \
> -		blt_surface_to_png((fd), (id), (name), (obj), (w), (h)); } while (0)
> +		blt_surface_to_png((fd), (id), (name), (obj), (w), (h), (bpp)); } while (0)
>   
>   struct blt_fast_copy_data {
>   	int xe;
> @@ -141,7 +141,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
>   	PRINT_SURFACE_INFO("dst", dst);
>   
>   	blt_surface_fill_rect(xe, src, width, height);
> -	WRITE_PNG(xe, mid_tiling, "src", src, width, height);
> +	WRITE_PNG(xe, mid_tiling, "src", src, width, height, bpp);
>   
>   	memset(&blt, 0, sizeof(blt));
>   	blt.color_depth = CD_32bit;
> @@ -153,8 +153,8 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
>   
>   	fast_copy_one_bb(xe, ctx, ahnd, &blt);
>   
> -	WRITE_PNG(xe, mid_tiling, "mid", &blt.mid, width, height);
> -	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height);
> +	WRITE_PNG(xe, mid_tiling, "mid", &blt.mid, width, height, bpp);
> +	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   
> @@ -205,8 +205,8 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
>   
>   	blt_fast_copy(xe, ctx, NULL, ahnd, &blt);
>   
> -	WRITE_PNG(xe, mid_tiling, "src", &blt.src, width, height);
> -	WRITE_PNG(xe, mid_tiling, "mid", &blt.dst, width, height);
> +	WRITE_PNG(xe, mid_tiling, "src", &blt.src, width, height, bpp);
> +	WRITE_PNG(xe, mid_tiling, "mid", &blt.dst, width, height, bpp);
>   
>   	blt_copy_init(xe, &blt);
>   	blt.color_depth = CD_32bit;
> @@ -217,7 +217,7 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
>   
>   	blt_fast_copy(xe, ctx, NULL, ahnd, &blt);
>   
> -	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height);
> +	WRITE_PNG(xe, mid_tiling, "dst", &blt.dst, width, height, bpp);
>   
>   	result = memcmp(src->ptr, blt.dst.ptr, src->size);
>   

Re: [PATCH i-g-t v4 4/5] tests/xe-ccs: Add tests which exercise small to large blit sizes

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> Testing block-copy for 512 x 512 x 32bpp is not enough to verify
> blit is working for different (small) and not always aligned
> resolutions. Add 'increment' subtests which checks blits from
> small (1x1) to large (512x512) resolutions.
> 
> To avoid too long execution resolution increment equals 15x15 pixels.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> ---

Thanks for the comment on why the step is 15 px wide.

I wonder if we could just start testing the compressed case from bigger 
widths and heights, instead of adding that if on the mid surface. But 
probably we'd limit the coverage, so let's leave it as it is.

A small nit below.

>   tests/intel/xe_ccs.c | 144 +++++++++++++++++++++++++++++++++----------
>   1 file changed, 112 insertions(+), 32 deletions(-)
> 
> diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> index a7785edcb1..627976049c 100644
> --- a/tests/intel/xe_ccs.c
> +++ b/tests/intel/xe_ccs.c
> @@ -28,9 +28,15 @@
>    * SUBTEST: block-copy-compressed
>    * Description: Check block-copy flatccs compressed blit
>    *
> + * SUBTEST: block-copy-compressed-inc-dimension
> + * Description: Check block-copy compressed blit for different sizes
> + *
>    * SUBTEST: block-copy-uncompressed
>    * Description: Check block-copy uncompressed blit
>    *
> + * SUBTEST: block-copy-uncompressed-inc-dimension
> + * Description: Check block-copy uncompressed blit for different sizes
> + *
>    * SUBTEST: block-multicopy-compressed
>    * Description: Check block-multicopy flatccs compressed blit
>    *
> @@ -73,6 +79,8 @@ struct test_config {
>   	bool surfcopy;
>   	bool new_ctx;
>   	bool suspend_resume;
> +	int width_increment;
> +	int width_steps;
>   };
>   
>   #define PRINT_SURFACE_INFO(name, obj) do { \
> @@ -286,9 +294,17 @@ static int blt_block_copy3(int xe,
>   	return ret;
>   }
>   
> +#define CHECK_MIN_WIDTH 2
> +#define CHECK_MIN_HEIGHT 2
> +#define MIN_EXP_WH(w, h) ((w) >= CHECK_MIN_WIDTH && (h) >= CHECK_MIN_HEIGHT)
> +#define CHECK_FROM_WIDTH 256
> +#define CHECK_FROM_HEIGHT 256
> +#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
> +
>   static void block_copy(int xe,
>   		       intel_ctx_t *ctx,
>   		       uint32_t region1, uint32_t region2,
> +		       uint32_t width, uint32_t height,
>   		       enum blt_tiling_type mid_tiling,
>   		       const struct test_config *config)
>   {
> @@ -301,7 +317,7 @@ static void block_copy(int xe,
>   	uint32_t run_id = mid_tiling;
>   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
>   							!xe_has_vram(xe)) ? region1 : region2;
> -	uint32_t width = param.width, height = param.height, bb;
> +	uint32_t bb;
>   	enum blt_compression mid_compression = config->compression;
>   	int mid_compression_format = param.compression_format;
>   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> @@ -339,9 +355,16 @@ static void block_copy(int xe,
>   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
>   	intel_ctx_xe_sync(ctx, true);
>   
> -	/* We expect mid != src if there's compression */
> -	if (mid->compression)
> -		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> +	/*
> +	 * We expect mid != src if there's compression. Ignore this for small
> +	 * width x height for linear as compression for gradient occurs in the
> +	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
> +	 * xmajor.
> +	 */
> +	if (mid->compression && MIN_EXP_WH(width, height)) {
> +		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
> +			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> +	}
>   
>   	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
>   
> @@ -416,6 +439,7 @@ static void block_copy(int xe,
>   static void block_multicopy(int xe,
>   			    intel_ctx_t *ctx,
>   			    uint32_t region1, uint32_t region2,
> +			    uint32_t width, uint32_t height,
>   			    enum blt_tiling_type mid_tiling,
>   			    const struct test_config *config)
>   {
> @@ -429,7 +453,7 @@ static void block_multicopy(int xe,
>   	uint32_t run_id = mid_tiling;
>   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
>   							!xe_has_vram(xe)) ? region1 : region2;
> -	uint32_t width = param.width, height = param.height, bb;
> +	uint32_t bb;
>   	enum blt_compression mid_compression = config->compression;
>   	int mid_compression_format = param.compression_format;
>   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> @@ -521,6 +545,7 @@ static const struct {
>   	void (*copyfn)(int fd,
>   		       intel_ctx_t *ctx,
>   		       uint32_t region1, uint32_t region2,
> +		       uint32_t width, uint32_t height,
>   		       enum blt_tiling_type btype,
>   		       const struct test_config *config);
>   } copyfns[] = {
> @@ -528,17 +553,44 @@ static const struct {
>   	[BLOCK_MULTICOPY] = { "-multicopy", block_multicopy },
>   };
>   
> -static void block_copy_test(int xe,
> -			    const struct test_config *config,
> -			    struct igt_collection *set,
> -			    enum copy_func copy_function)
> +static void single_copy(int xe, const struct test_config *config,
> +			int32_t region1, uint32_t region2,
> +			uint32_t width, uint32_t height,
> +			int tiling, enum copy_func copy_function)
>   {
>   	struct drm_xe_engine_class_instance inst = {
>   		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
>   	};
> +	uint32_t vm, exec_queue;
> +	uint32_t sync_bind, sync_out;
>   	intel_ctx_t *ctx;
> +
> +	vm = xe_vm_create(xe, 0, 0);
> +	exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> +	sync_bind = syncobj_create(xe, 0);
> +	sync_out = syncobj_create(xe, 0);
> +	ctx = intel_ctx_xe(xe, vm, exec_queue,
> +			   0, sync_bind, sync_out);
> +
> +	copyfns[copy_function].copyfn(xe, ctx,
> +				      region1, region2,
> +				      width, height,
> +				      tiling, config);
> +
> +	xe_exec_queue_destroy(xe, exec_queue);
> +	xe_vm_destroy(xe, vm);
> +	syncobj_destroy(xe, sync_bind);
> +	syncobj_destroy(xe, sync_out);
> +	free(ctx);
> +}
> +
> +static void block_copy_test(int xe,
> +			    const struct test_config *config,
> +			    struct igt_collection *set,
> +			    enum copy_func copy_function)
> +{
> +

^-- there's an empty line (I almost missed that!)

But apart from that, the patch is looking good, I think. Just fix that 
line, and:

Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

>   	struct igt_collection *regions;
> -	uint32_t vm, exec_queue;
>   	int tiling;
>   
>   	if (config->compression && !blt_block_copy_supports_compression(xe))
> @@ -555,6 +607,7 @@ static void block_copy_test(int xe,
>   		for_each_variation_r(regions, 2, set) {
>   			uint32_t region1, region2;
>   			char *regtxt;
> +			char testname[256];
>   
>   			region1 = igt_collection_get_value(regions, 0);
>   			region2 = igt_collection_get_value(regions, 1);
> @@ -566,30 +619,36 @@ static void block_copy_test(int xe,
>   
>   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
>   
> -			igt_dynamic_f("%s-%s-compfmt%d-%s%s",
> -				      blt_tiling_name(tiling),
> -				      config->compression ?
> -					      "compressed" : "uncompressed",
> -				      param.compression_format, regtxt,
> -				      copyfns[copy_function].suffix) {
> -				uint32_t sync_bind, sync_out;
> +			snprintf(testname, sizeof(testname),
> +				 "%s-%s-compfmt%d-%s%s",
> +				 blt_tiling_name(tiling),
> +				 config->compression ?
> +					 "compressed" : "uncompressed",
> +				 param.compression_format, regtxt,
> +				 copyfns[copy_function].suffix);
>   
> -				vm = xe_vm_create(xe, 0, 0);
> -				exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> -				sync_bind = syncobj_create(xe, 0);
> -				sync_out = syncobj_create(xe, 0);
> -				ctx = intel_ctx_xe(xe, vm, exec_queue,
> -						   0, sync_bind, sync_out);
> +			if (!config->width_increment) {
> +				igt_dynamic(testname)
> +					single_copy(xe, config, region1, region2,
> +						    param.width, param.height,
> +						    tiling, copy_function);
> +			} else {
> +				for (int w = param.width;
> +				     w < param.width + config->width_steps;
> +				     w += config->width_increment) {
> +					snprintf(testname, sizeof(testname),
> +						 "%s-%s-compfmt%d-%s%s-%dx%d",
> +						 blt_tiling_name(tiling),
> +						 config->compression ?
> +							 "compressed" : "uncompressed",
> +						 param.compression_format, regtxt,
> +						 copyfns[copy_function].suffix,
> +						 w, w);
> +					igt_dynamic(testname)
> +						single_copy(xe, config, region1, region2,
> +							    w, w, tiling, copy_function);
> +				}
>   
> -				copyfns[copy_function].copyfn(xe, ctx,
> -							      region1, region2,
> -							      tiling, config);
> -
> -				xe_exec_queue_destroy(xe, exec_queue);
> -				xe_vm_destroy(xe, vm);
> -				syncobj_destroy(xe, sync_bind);
> -				syncobj_destroy(xe, sync_out);
> -				free(ctx);
>   			}
>   
>   			free(regtxt);
> @@ -669,6 +728,16 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
>   		block_copy_test(xe, &config, set, BLOCK_COPY);
>   	}
>   
> +	igt_describe("Check block-copy uncompressed blit with increment width/height");
> +	igt_subtest_with_dynamic("block-copy-uncompressed-inc-dimension") {
> +		struct test_config config = { .width_increment = 15,
> +					      .width_steps = 512 };
> +		param.width = 1;
> +		param.height = 1;
> +
> +		block_copy_test(xe, &config, set, BLOCK_COPY);
> +	}
> +
>   	igt_describe("Check block-copy flatccs compressed blit");
>   	igt_subtest_with_dynamic("block-copy-compressed") {
>   		struct test_config config = { .compression = true };
> @@ -676,6 +745,17 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
>   		block_copy_test(xe, &config, set, BLOCK_COPY);
>   	}
>   
> +	igt_describe("Check block-copy compressed blit with increment width/height");
> +	igt_subtest_with_dynamic("block-copy-compressed-inc-dimension") {
> +		struct test_config config = { .compression = true,
> +					      .width_increment = 15,
> +					      .width_steps = 512 };
> +		param.width = 1;
> +		param.height = 1;
> +
> +		block_copy_test(xe, &config, set, BLOCK_COPY);
> +	}
> +
>   	igt_describe("Check block-multicopy flatccs compressed blit");
>   	igt_subtest_with_dynamic("block-multicopy-compressed") {
>   		struct test_config config = { .compression = true };

Re: [PATCH i-g-t v4 4/5] tests/xe-ccs: Add tests which exercise small to large blit sizes

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> Testing block-copy for 512 x 512 x 32bpp is not enough to verify
> blit is working for different (small) and not always aligned
> resolutions. Add 'increment' subtests which checks blits from
> small (1x1) to large (512x512) resolutions.
> 
> To avoid too long execution resolution increment equals 15x15 pixels.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> ---
>   tests/intel/xe_ccs.c | 144 +++++++++++++++++++++++++++++++++----------
>   1 file changed, 112 insertions(+), 32 deletions(-)

Ah, one question actually. We use width == height, and I wonder if we 
want to test rectangular surfaces. I think that caused problems in the 
past, but maybe I'm misremembering something.

All the best,
Karolina

> 
> diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> index a7785edcb1..627976049c 100644
> --- a/tests/intel/xe_ccs.c
> +++ b/tests/intel/xe_ccs.c
> @@ -28,9 +28,15 @@
>    * SUBTEST: block-copy-compressed
>    * Description: Check block-copy flatccs compressed blit
>    *
> + * SUBTEST: block-copy-compressed-inc-dimension
> + * Description: Check block-copy compressed blit for different sizes
> + *
>    * SUBTEST: block-copy-uncompressed
>    * Description: Check block-copy uncompressed blit
>    *
> + * SUBTEST: block-copy-uncompressed-inc-dimension
> + * Description: Check block-copy uncompressed blit for different sizes
> + *
>    * SUBTEST: block-multicopy-compressed
>    * Description: Check block-multicopy flatccs compressed blit
>    *
> @@ -73,6 +79,8 @@ struct test_config {
>   	bool surfcopy;
>   	bool new_ctx;
>   	bool suspend_resume;
> +	int width_increment;
> +	int width_steps;
>   };
>   
>   #define PRINT_SURFACE_INFO(name, obj) do { \
> @@ -286,9 +294,17 @@ static int blt_block_copy3(int xe,
>   	return ret;
>   }
>   
> +#define CHECK_MIN_WIDTH 2
> +#define CHECK_MIN_HEIGHT 2
> +#define MIN_EXP_WH(w, h) ((w) >= CHECK_MIN_WIDTH && (h) >= CHECK_MIN_HEIGHT)
> +#define CHECK_FROM_WIDTH 256
> +#define CHECK_FROM_HEIGHT 256
> +#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
> +
>   static void block_copy(int xe,
>   		       intel_ctx_t *ctx,
>   		       uint32_t region1, uint32_t region2,
> +		       uint32_t width, uint32_t height,
>   		       enum blt_tiling_type mid_tiling,
>   		       const struct test_config *config)
>   {
> @@ -301,7 +317,7 @@ static void block_copy(int xe,
>   	uint32_t run_id = mid_tiling;
>   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
>   							!xe_has_vram(xe)) ? region1 : region2;
> -	uint32_t width = param.width, height = param.height, bb;
> +	uint32_t bb;
>   	enum blt_compression mid_compression = config->compression;
>   	int mid_compression_format = param.compression_format;
>   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> @@ -339,9 +355,16 @@ static void block_copy(int xe,
>   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
>   	intel_ctx_xe_sync(ctx, true);
>   
> -	/* We expect mid != src if there's compression */
> -	if (mid->compression)
> -		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> +	/*
> +	 * We expect mid != src if there's compression. Ignore this for small
> +	 * width x height for linear as compression for gradient occurs in the
> +	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
> +	 * xmajor.
> +	 */
> +	if (mid->compression && MIN_EXP_WH(width, height)) {
> +		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
> +			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> +	}
>   
>   	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
>   
> @@ -416,6 +439,7 @@ static void block_copy(int xe,
>   static void block_multicopy(int xe,
>   			    intel_ctx_t *ctx,
>   			    uint32_t region1, uint32_t region2,
> +			    uint32_t width, uint32_t height,
>   			    enum blt_tiling_type mid_tiling,
>   			    const struct test_config *config)
>   {
> @@ -429,7 +453,7 @@ static void block_multicopy(int xe,
>   	uint32_t run_id = mid_tiling;
>   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
>   							!xe_has_vram(xe)) ? region1 : region2;
> -	uint32_t width = param.width, height = param.height, bb;
> +	uint32_t bb;
>   	enum blt_compression mid_compression = config->compression;
>   	int mid_compression_format = param.compression_format;
>   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> @@ -521,6 +545,7 @@ static const struct {
>   	void (*copyfn)(int fd,
>   		       intel_ctx_t *ctx,
>   		       uint32_t region1, uint32_t region2,
> +		       uint32_t width, uint32_t height,
>   		       enum blt_tiling_type btype,
>   		       const struct test_config *config);
>   } copyfns[] = {
> @@ -528,17 +553,44 @@ static const struct {
>   	[BLOCK_MULTICOPY] = { "-multicopy", block_multicopy },
>   };
>   
> -static void block_copy_test(int xe,
> -			    const struct test_config *config,
> -			    struct igt_collection *set,
> -			    enum copy_func copy_function)
> +static void single_copy(int xe, const struct test_config *config,
> +			int32_t region1, uint32_t region2,
> +			uint32_t width, uint32_t height,
> +			int tiling, enum copy_func copy_function)
>   {
>   	struct drm_xe_engine_class_instance inst = {
>   		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
>   	};
> +	uint32_t vm, exec_queue;
> +	uint32_t sync_bind, sync_out;
>   	intel_ctx_t *ctx;
> +
> +	vm = xe_vm_create(xe, 0, 0);
> +	exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> +	sync_bind = syncobj_create(xe, 0);
> +	sync_out = syncobj_create(xe, 0);
> +	ctx = intel_ctx_xe(xe, vm, exec_queue,
> +			   0, sync_bind, sync_out);
> +
> +	copyfns[copy_function].copyfn(xe, ctx,
> +				      region1, region2,
> +				      width, height,
> +				      tiling, config);
> +
> +	xe_exec_queue_destroy(xe, exec_queue);
> +	xe_vm_destroy(xe, vm);
> +	syncobj_destroy(xe, sync_bind);
> +	syncobj_destroy(xe, sync_out);
> +	free(ctx);
> +}
> +
> +static void block_copy_test(int xe,
> +			    const struct test_config *config,
> +			    struct igt_collection *set,
> +			    enum copy_func copy_function)
> +{
> +
>   	struct igt_collection *regions;
> -	uint32_t vm, exec_queue;
>   	int tiling;
>   
>   	if (config->compression && !blt_block_copy_supports_compression(xe))
> @@ -555,6 +607,7 @@ static void block_copy_test(int xe,
>   		for_each_variation_r(regions, 2, set) {
>   			uint32_t region1, region2;
>   			char *regtxt;
> +			char testname[256];
>   
>   			region1 = igt_collection_get_value(regions, 0);
>   			region2 = igt_collection_get_value(regions, 1);
> @@ -566,30 +619,36 @@ static void block_copy_test(int xe,
>   
>   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
>   
> -			igt_dynamic_f("%s-%s-compfmt%d-%s%s",
> -				      blt_tiling_name(tiling),
> -				      config->compression ?
> -					      "compressed" : "uncompressed",
> -				      param.compression_format, regtxt,
> -				      copyfns[copy_function].suffix) {
> -				uint32_t sync_bind, sync_out;
> +			snprintf(testname, sizeof(testname),
> +				 "%s-%s-compfmt%d-%s%s",
> +				 blt_tiling_name(tiling),
> +				 config->compression ?
> +					 "compressed" : "uncompressed",
> +				 param.compression_format, regtxt,
> +				 copyfns[copy_function].suffix);
>   
> -				vm = xe_vm_create(xe, 0, 0);
> -				exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> -				sync_bind = syncobj_create(xe, 0);
> -				sync_out = syncobj_create(xe, 0);
> -				ctx = intel_ctx_xe(xe, vm, exec_queue,
> -						   0, sync_bind, sync_out);
> +			if (!config->width_increment) {
> +				igt_dynamic(testname)
> +					single_copy(xe, config, region1, region2,
> +						    param.width, param.height,
> +						    tiling, copy_function);
> +			} else {
> +				for (int w = param.width;
> +				     w < param.width + config->width_steps;
> +				     w += config->width_increment) {
> +					snprintf(testname, sizeof(testname),
> +						 "%s-%s-compfmt%d-%s%s-%dx%d",
> +						 blt_tiling_name(tiling),
> +						 config->compression ?
> +							 "compressed" : "uncompressed",
> +						 param.compression_format, regtxt,
> +						 copyfns[copy_function].suffix,
> +						 w, w);
> +					igt_dynamic(testname)
> +						single_copy(xe, config, region1, region2,
> +							    w, w, tiling, copy_function);
> +				}
>   
> -				copyfns[copy_function].copyfn(xe, ctx,
> -							      region1, region2,
> -							      tiling, config);
> -
> -				xe_exec_queue_destroy(xe, exec_queue);
> -				xe_vm_destroy(xe, vm);
> -				syncobj_destroy(xe, sync_bind);
> -				syncobj_destroy(xe, sync_out);
> -				free(ctx);
>   			}
>   
>   			free(regtxt);
> @@ -669,6 +728,16 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
>   		block_copy_test(xe, &config, set, BLOCK_COPY);
>   	}
>   
> +	igt_describe("Check block-copy uncompressed blit with increment width/height");
> +	igt_subtest_with_dynamic("block-copy-uncompressed-inc-dimension") {
> +		struct test_config config = { .width_increment = 15,
> +					      .width_steps = 512 };
> +		param.width = 1;
> +		param.height = 1;
> +
> +		block_copy_test(xe, &config, set, BLOCK_COPY);
> +	}
> +
>   	igt_describe("Check block-copy flatccs compressed blit");
>   	igt_subtest_with_dynamic("block-copy-compressed") {
>   		struct test_config config = { .compression = true };
> @@ -676,6 +745,17 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
>   		block_copy_test(xe, &config, set, BLOCK_COPY);
>   	}
>   
> +	igt_describe("Check block-copy compressed blit with increment width/height");
> +	igt_subtest_with_dynamic("block-copy-compressed-inc-dimension") {
> +		struct test_config config = { .compression = true,
> +					      .width_increment = 15,
> +					      .width_steps = 512 };
> +		param.width = 1;
> +		param.height = 1;
> +
> +		block_copy_test(xe, &config, set, BLOCK_COPY);
> +	}
> +
>   	igt_describe("Check block-multicopy flatccs compressed blit");
>   	igt_subtest_with_dynamic("block-multicopy-compressed") {
>   		struct test_config config = { .compression = true };

Re: [PATCH i-g-t v4 5/5] tests/xe_exercise_blt: Exercise small to large fast-copy blits

[Karolina Stolarek]

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> Testing blitter with large size like 512x512 may be not enough
> to verify small or unaligned blits works properly. Add subtest
> which spread fast-copy blits from small to large.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> ---
>   tests/intel/xe_exercise_blt.c | 63 +++++++++++++++++++++++++++++------
>   1 file changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/intel/xe_exercise_blt.c b/tests/intel/xe_exercise_blt.c
> index ddf9d188a7..8b4bae282b 100644
> --- a/tests/intel/xe_exercise_blt.c
> +++ b/tests/intel/xe_exercise_blt.c
> @@ -25,6 +25,10 @@
>    *   Check fast-copy blit
>    *   blitter
>    *
> + * SUBTEST: fast-copy-inc-dimension
> + * Description:
> + *   Check fast-copy blit with sizes from small to large
> + *
>    * SUBTEST: fast-copy-emit
>    * Description:
>    *   Check multiple fast-copy in one batch
> @@ -40,6 +44,8 @@ static struct param {
>   	bool print_surface_info;
>   	int width;
>   	int height;
> +	int width_increment;
> +	int width_steps;
>   } param = {
>   	.tiling = -1,
>   	.write_png = false,
> @@ -111,6 +117,7 @@ static int fast_copy_one_bb(int xe,
>   }
>   
>   static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
> +			   uint32_t width, uint32_t height,
>   			   uint32_t region1, uint32_t region2,
>   			   enum blt_tiling_type mid_tiling)
>   {
> @@ -122,7 +129,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
>   	uint64_t ahnd = intel_allocator_open_full(xe, ctx->vm, 0, 0,
>   						  INTEL_ALLOCATOR_SIMPLE,
>   						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
> -	uint32_t bb, width = param.width, height = param.height;
> +	uint32_t bb;
>   	int result;
>   
>   	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
> @@ -170,6 +177,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
>   }
>   
>   static void fast_copy(int xe, const intel_ctx_t *ctx,
> +		      uint32_t width, uint32_t height,
>   		      uint32_t region1, uint32_t region2,
>   		      enum blt_tiling_type mid_tiling)
>   {
> @@ -181,7 +189,6 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
>   						  INTEL_ALLOCATOR_SIMPLE,
>   						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
>   	uint32_t bb;
> -	uint32_t width = param.width, height = param.height;
>   	int result;
>   
>   	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
> @@ -241,14 +248,20 @@ enum fast_copy_func {
>   };
>   
>   static char
> -	*full_subtest_str(char *regtxt, enum blt_tiling_type tiling,
> +	*full_subtest_str(char *regtxt, uint32_t width, uint32_t height,
> +			  enum blt_tiling_type tiling,
>   			  enum fast_copy_func func)
>   {
>   	char *name;
>   	uint32_t len;
>   
> -	len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
> -		       func == FAST_COPY_EMIT ? "-emit" : "");
> +	if (!width || !height)
> +		len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
> +			       func == FAST_COPY_EMIT ? "-emit" : "");
> +	else
> +		len = asprintf(&name, "%s-%s%s-%ux%u", blt_tiling_name(tiling), regtxt,
> +			       func == FAST_COPY_EMIT ? "-emit" : "",
> +			       width, height);
>   
>   	igt_assert_f(len >= 0, "asprintf failed!\n");
>   
> @@ -264,6 +277,7 @@ static void fast_copy_test(int xe,
>   	};
>   	struct igt_collection *regions;
>   	void (*copy_func)(int xe, const intel_ctx_t *ctx,
> +			  uint32_t width, uint32_t height,
>   			  uint32_t r1, uint32_t r2, enum blt_tiling_type tiling);
>   	intel_ctx_t *ctx;
>   	int tiling;
> @@ -286,16 +300,32 @@ static void fast_copy_test(int xe,
>   
>   			copy_func = (func == FAST_COPY) ? fast_copy : fast_copy_emit;
>   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
> -			test_name = full_subtest_str(regtxt, tiling, func);
>   
> -			igt_dynamic_f("%s", test_name) {
> -				copy_func(xe, ctx,
> -					  region1, region2,
> -					  tiling);
> +			if (!param.width_increment) {
> +				test_name = full_subtest_str(regtxt, 0, 0, tiling, func);

I'd simply pass param.width and param.height here and use the common
name for both width_increment and fixed size test cases. We don't have
to keep the same "%s-%s%s" format, extending it would be fine (and
helpful, I think).

All the best,
Karolina

> +				igt_dynamic_f("%s", test_name) {
> +					copy_func(xe, ctx,
> +						  param.width, param.height,
> +						  region1, region2,
> +						  tiling);
> +				}
> +				free(test_name);
> +			} else {
> +				for (int w = param.width;
> +				     w < param.width + param.width_steps;
> +				     w += param.width_increment) {
> +					test_name = full_subtest_str(regtxt, w, w, tiling, func);
> +					igt_dynamic_f("%s", test_name) {
> +						copy_func(xe, ctx,
> +							  w, w,
> +							  region1, region2,
> +							  tiling);
> +					}
> +					free(test_name);
> +				}
>   			}
>   
>   			free(regtxt);
> -			free(test_name);
>   			xe_exec_queue_destroy(xe, exec_queue);
>   			xe_vm_destroy(xe, vm);
>   			free(ctx);
> @@ -367,6 +397,17 @@ igt_main_args("b:pst:W:H:", NULL, help_str, opt_handler, NULL)
>   		fast_copy_test(xe, set, FAST_COPY);
>   	}
>   
> +	igt_describe("Check fast-copy with increment width/height");
> +	igt_subtest_with_dynamic("fast-copy-inc-dimension") {
> +		param.width = 1;
> +		param.height = 1;
> +		param.width_increment = 15;
> +		param.width_steps = 512;
> +
> +		fast_copy_test(xe, set, FAST_COPY);
> +	}
> +
> +
>   	igt_describe("Check multiple fast-copy in one batch");
>   	igt_subtest_with_dynamic("fast-copy-emit") {
>   		fast_copy_test(xe, set, FAST_COPY_EMIT);

Re: [PATCH i-g-t v4 5/5] tests/xe_exercise_blt: Exercise small to large fast-copy blits

[Karolina Stolarek]

Ah, forgot to tell you that checkpatch was slightly uhappy with two 
blank lines after inc-dimension subtest...

On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
(...)
>   
> +	igt_describe("Check fast-copy with increment width/height");
> +	igt_subtest_with_dynamic("fast-copy-inc-dimension") {
> +		param.width = 1;
> +		param.height = 1;
> +		param.width_increment = 15;
> +		param.width_steps = 512;
> +
> +		fast_copy_test(xe, set, FAST_COPY);
> +	}
> +
> +
  ^-- here!

Sorry for the email noise.

All the best,
Karolina

>   	igt_describe("Check multiple fast-copy in one batch");
>   	igt_subtest_with_dynamic("fast-copy-emit") {
>   		fast_copy_test(xe, set, FAST_COPY_EMIT);

Re: [PATCH i-g-t v4 1/5] lib/intel_blt: Add helpers for calculating stride and aligned height

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 01:05:20PM +0100, Karolina Stolarek wrote:
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > Tiled surfaces have stride / aligned height constraints. Currently
> > blt library has limitation and doesn't work properly when surface
> > stride is not valid for specific tiling.
> > 
> > As an example lets say we want to copy from linear to xmajor
> > 33 x 33 x 32bpp surface. Xmajor surface expects stride aligned to
> > 512 bytes and height to 8 rows so this surface will occupy 512B x 40
> > (128 x 40 x 32 bpp).
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> 
> Just a bunch of nits, nothing major.
> 
> > ---
> > v2: Fix stride/aligned height calculation for Tile64
> > v3: Fix stride calculation for xmajor
> >      Make helpers public (fixes compilation warning due to lack
> >      or static function users)
> > ---
> >   lib/intel_blt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   lib/intel_blt.h |  4 ++++
> >   2 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index 25d251c4f8..13b1dbba4f 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -521,6 +521,70 @@ static int __block_tiling(enum blt_tiling_type tiling)
> >   	return 0;
> >   }
> > +/**
> > + * blt_get_min_stride
> > + * @width: width in pixels
> > + * @bpp: bits per pixel
> > + * @tiling: tiling
> > + *
> > + * Function returns minimum posibble stride in bytes for width, bpp and tiling.
> 
> "return" is already described, we could say "calculates minimum possible
> stride..."

Agree, repeating same looks odd.

> 
> > + *
> > + * Returns:
> > + * minimum possible stride in bytes.
> > + */
> > +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> > +			    enum blt_tiling_type tiling)
> > +{
> > +	switch (tiling) {
> > +	case T_LINEAR:
> > +		return width * bpp / 8;
> > +	case T_XMAJOR:
> > +		return ALIGN(width * bpp / 8, 512);
> > +	case T_TILE64:
> > +		if (bpp == 8)
> > +			return ALIGN(width, 256);
> > +		else if (bpp == 16 || bpp == 32)
> > +			return ALIGN(width * bpp / 8, 512);
> > +		return ALIGN(width * bpp / 8, 1024);
> > +
> > +	default:
> > +		return ALIGN(width * bpp / 8, 128);
> > +	}
> > +}
> > +
> > +/**
> > + * blt_get_aligned_height
> > + * @height: height in pixels
> > + * @bpp: bits per pixel (used for Tile64 due to different tile organization
> > + * in pixels)
> > + * @tiling: tiling
> > + *
> > + * Function returns aligned height for specific tiling. Height returned is
> > + * important from memory allocation perspective, because each tiling has
> > + * specific memory constraints.
> 
> I'd move the comment on Tile64 to the description body and integrate it
> with the last sentence. You could say that each tiling has constrains
> dependent on bpp and mention a different pixel order for Tile64 as an
> example.

I've added some note about Tile64 to commit message to explain why
we need to pass bpp either.

Thanks for the review.

> 
> Apart from that, it's looking good to me:
> 
> Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

R-b taken.

--
Zbigniew
> 
> > + *
> > + * Returns:
> > + * height (rows) expected for specific tiling
> > + */
> > +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> > +				enum blt_tiling_type tiling)
> > +{
> > +	switch (tiling) {
> > +	case T_LINEAR:
> > +		return height;
> > +	case T_XMAJOR:
> > +		return ALIGN(height, 8);
> > +	case T_TILE64:
> > +		if (bpp == 8)
> > +			return ALIGN(height, 256);
> > +		else if (bpp == 16 || bpp == 32)
> > +			return ALIGN(height, 128);
> > +		return ALIGN(height, 64);
> > +	default:
> > +		return ALIGN(height, 32);
> > +	}
> > +}
> > +
> >   static int __special_mode(const struct blt_copy_data *blt)
> >   {
> >   	if (blt->src.handle == blt->dst.handle &&
> > diff --git a/lib/intel_blt.h b/lib/intel_blt.h
> > index d9be22fdf4..e3084dc0cd 100644
> > --- a/lib/intel_blt.h
> > +++ b/lib/intel_blt.h
> > @@ -212,6 +212,10 @@ bool blt_block_copy_supports_compression(int fd);
> >   bool blt_uses_extended_block_copy(int fd);
> >   const char *blt_tiling_name(enum blt_tiling_type tiling);
> > +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> > +			    enum blt_tiling_type tiling);
> > +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> > +				enum blt_tiling_type tiling);
> >   void blt_copy_init(int fd, struct blt_copy_data *blt);

Re: [PATCH i-g-t v4 2/5] lib/intel_blt: Change surface size calculation

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 02:44:56PM +0100, Karolina Stolarek wrote:
> 
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > For tiled surfaces we need to ensure stride and height are valid
> > from the blit operation perspective. Calculate required surface
> > size according to tiling constraints.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> 
> I'm looking at blt_set_geom() function. We still pass normal height and
> width to it. Is this correct? Or should we change height to
> aligned_height? Because if we should, this patch should be updated to
> reflect that.

Function creates an object, which should be correct from stride/aligned
height perspective. I mean you need to allocate enough memory to handle
all pixels which will be touched during blit. So the best is to allocate
whole tile (x/y/4/64). Then you'll be sure hw won't touch addresses
outside your object and you don't hang the engine.

For example you want to blit 1x128x32bpp between two surfaces for
Tile64. Even you're using single pixel in x (one dword) you need to
provide stride 128 (in dwords). That's hardware requirement and even
if this looks like waste of memory (127 pixels in x won't be used at
all) hardware expects that. I learned this hard way during extending
block-copy from small to large buffers.

I wondered to refactor blt_copy_object to contain bpp (this way we
may warn if user passes invalid stride during object setup) but at
the moment I resisted as my primary goal is to fill the test gap
in blit/render-copy from small to large buffers.


> 
> If that's not a problem, then:
> 
> Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

Thanks for the review.

--
Zbigniew

> 
> > ---
> >   lib/intel_blt.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index 13b1dbba4f..00208fc243 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -1858,13 +1858,21 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
> >   		  bool create_mapping)
> >   {
> >   	struct blt_copy_object *obj;
> > -	uint64_t size = width * height * bpp / 8;
> > -	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
> > +	uint32_t stride, aligned_height;
> > +	uint64_t size;
> >   	uint32_t handle;
> >   	uint8_t pat_index = DEFAULT_PAT_INDEX;
> >   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
> > +	stride = blt_get_min_stride(width, bpp, tiling);
> > +	aligned_height = blt_get_aligned_height(height, bpp, tiling);
> > +	size = stride * aligned_height;
> > +
> > +	/* blitter command expects stride in dwords on tiled surfaces */
> > +	if (tiling)
> > +		stride /= 4;
> > +
> >   	obj = calloc(1, sizeof(*obj));
> >   	obj->size = size;

Re: [PATCH i-g-t v4 4/5] tests/xe-ccs: Add tests which exercise small to large blit sizes

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 03:09:30PM +0100, Karolina Stolarek wrote:
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > Testing block-copy for 512 x 512 x 32bpp is not enough to verify
> > blit is working for different (small) and not always aligned
> > resolutions. Add 'increment' subtests which checks blits from
> > small (1x1) to large (512x512) resolutions.
> > 
> > To avoid too long execution resolution increment equals 15x15 pixels.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> > ---
> 
> Thanks for the comment on why the step is 15 px wide.
> 
> I wonder if we could just start testing the compressed case from bigger
> widths and heights, instead of adding that if on the mid surface. But
> probably we'd limit the coverage, so let's leave it as it is.

Both - small and large may lead to fail, depending on situation.
Especially in Tile64. 15x15 is picked intentionally to step by step
increase size which at some point will be modulo 16.
> 
> A small nit below.
> 
> >   tests/intel/xe_ccs.c | 144 +++++++++++++++++++++++++++++++++----------
> >   1 file changed, 112 insertions(+), 32 deletions(-)
> > 
> > diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> > index a7785edcb1..627976049c 100644
> > --- a/tests/intel/xe_ccs.c
> > +++ b/tests/intel/xe_ccs.c
> > @@ -28,9 +28,15 @@
> >    * SUBTEST: block-copy-compressed
> >    * Description: Check block-copy flatccs compressed blit
> >    *
> > + * SUBTEST: block-copy-compressed-inc-dimension
> > + * Description: Check block-copy compressed blit for different sizes
> > + *
> >    * SUBTEST: block-copy-uncompressed
> >    * Description: Check block-copy uncompressed blit
> >    *
> > + * SUBTEST: block-copy-uncompressed-inc-dimension
> > + * Description: Check block-copy uncompressed blit for different sizes
> > + *
> >    * SUBTEST: block-multicopy-compressed
> >    * Description: Check block-multicopy flatccs compressed blit
> >    *
> > @@ -73,6 +79,8 @@ struct test_config {
> >   	bool surfcopy;
> >   	bool new_ctx;
> >   	bool suspend_resume;
> > +	int width_increment;
> > +	int width_steps;
> >   };
> >   #define PRINT_SURFACE_INFO(name, obj) do { \
> > @@ -286,9 +294,17 @@ static int blt_block_copy3(int xe,
> >   	return ret;
> >   }
> > +#define CHECK_MIN_WIDTH 2
> > +#define CHECK_MIN_HEIGHT 2
> > +#define MIN_EXP_WH(w, h) ((w) >= CHECK_MIN_WIDTH && (h) >= CHECK_MIN_HEIGHT)
> > +#define CHECK_FROM_WIDTH 256
> > +#define CHECK_FROM_HEIGHT 256
> > +#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
> > +
> >   static void block_copy(int xe,
> >   		       intel_ctx_t *ctx,
> >   		       uint32_t region1, uint32_t region2,
> > +		       uint32_t width, uint32_t height,
> >   		       enum blt_tiling_type mid_tiling,
> >   		       const struct test_config *config)
> >   {
> > @@ -301,7 +317,7 @@ static void block_copy(int xe,
> >   	uint32_t run_id = mid_tiling;
> >   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
> >   							!xe_has_vram(xe)) ? region1 : region2;
> > -	uint32_t width = param.width, height = param.height, bb;
> > +	uint32_t bb;
> >   	enum blt_compression mid_compression = config->compression;
> >   	int mid_compression_format = param.compression_format;
> >   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> > @@ -339,9 +355,16 @@ static void block_copy(int xe,
> >   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
> >   	intel_ctx_xe_sync(ctx, true);
> > -	/* We expect mid != src if there's compression */
> > -	if (mid->compression)
> > -		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> > +	/*
> > +	 * We expect mid != src if there's compression. Ignore this for small
> > +	 * width x height for linear as compression for gradient occurs in the
> > +	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
> > +	 * xmajor.
> > +	 */
> > +	if (mid->compression && MIN_EXP_WH(width, height)) {
> > +		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
> > +			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> > +	}
> >   	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
> > @@ -416,6 +439,7 @@ static void block_copy(int xe,
> >   static void block_multicopy(int xe,
> >   			    intel_ctx_t *ctx,
> >   			    uint32_t region1, uint32_t region2,
> > +			    uint32_t width, uint32_t height,
> >   			    enum blt_tiling_type mid_tiling,
> >   			    const struct test_config *config)
> >   {
> > @@ -429,7 +453,7 @@ static void block_multicopy(int xe,
> >   	uint32_t run_id = mid_tiling;
> >   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
> >   							!xe_has_vram(xe)) ? region1 : region2;
> > -	uint32_t width = param.width, height = param.height, bb;
> > +	uint32_t bb;
> >   	enum blt_compression mid_compression = config->compression;
> >   	int mid_compression_format = param.compression_format;
> >   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> > @@ -521,6 +545,7 @@ static const struct {
> >   	void (*copyfn)(int fd,
> >   		       intel_ctx_t *ctx,
> >   		       uint32_t region1, uint32_t region2,
> > +		       uint32_t width, uint32_t height,
> >   		       enum blt_tiling_type btype,
> >   		       const struct test_config *config);
> >   } copyfns[] = {
> > @@ -528,17 +553,44 @@ static const struct {
> >   	[BLOCK_MULTICOPY] = { "-multicopy", block_multicopy },
> >   };
> > -static void block_copy_test(int xe,
> > -			    const struct test_config *config,
> > -			    struct igt_collection *set,
> > -			    enum copy_func copy_function)
> > +static void single_copy(int xe, const struct test_config *config,
> > +			int32_t region1, uint32_t region2,
> > +			uint32_t width, uint32_t height,
> > +			int tiling, enum copy_func copy_function)
> >   {
> >   	struct drm_xe_engine_class_instance inst = {
> >   		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
> >   	};
> > +	uint32_t vm, exec_queue;
> > +	uint32_t sync_bind, sync_out;
> >   	intel_ctx_t *ctx;
> > +
> > +	vm = xe_vm_create(xe, 0, 0);
> > +	exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> > +	sync_bind = syncobj_create(xe, 0);
> > +	sync_out = syncobj_create(xe, 0);
> > +	ctx = intel_ctx_xe(xe, vm, exec_queue,
> > +			   0, sync_bind, sync_out);
> > +
> > +	copyfns[copy_function].copyfn(xe, ctx,
> > +				      region1, region2,
> > +				      width, height,
> > +				      tiling, config);
> > +
> > +	xe_exec_queue_destroy(xe, exec_queue);
> > +	xe_vm_destroy(xe, vm);
> > +	syncobj_destroy(xe, sync_bind);
> > +	syncobj_destroy(xe, sync_out);
> > +	free(ctx);
> > +}
> > +
> > +static void block_copy_test(int xe,
> > +			    const struct test_config *config,
> > +			    struct igt_collection *set,
> > +			    enum copy_func copy_function)
> > +{
> > +
> 
> ^-- there's an empty line (I almost missed that!)

Thanks for spotting this.

> 
> But apart from that, the patch is looking good, I think. Just fix that line,
> and:
> 
> Reviewed-by: Karolina Stolarek <karolina.stolarek@intel.com>

Ok, thanks for the review.

--
Zbigniew

> 
> >   	struct igt_collection *regions;
> > -	uint32_t vm, exec_queue;
> >   	int tiling;
> >   	if (config->compression && !blt_block_copy_supports_compression(xe))
> > @@ -555,6 +607,7 @@ static void block_copy_test(int xe,
> >   		for_each_variation_r(regions, 2, set) {
> >   			uint32_t region1, region2;
> >   			char *regtxt;
> > +			char testname[256];
> >   			region1 = igt_collection_get_value(regions, 0);
> >   			region2 = igt_collection_get_value(regions, 1);
> > @@ -566,30 +619,36 @@ static void block_copy_test(int xe,
> >   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
> > -			igt_dynamic_f("%s-%s-compfmt%d-%s%s",
> > -				      blt_tiling_name(tiling),
> > -				      config->compression ?
> > -					      "compressed" : "uncompressed",
> > -				      param.compression_format, regtxt,
> > -				      copyfns[copy_function].suffix) {
> > -				uint32_t sync_bind, sync_out;
> > +			snprintf(testname, sizeof(testname),
> > +				 "%s-%s-compfmt%d-%s%s",
> > +				 blt_tiling_name(tiling),
> > +				 config->compression ?
> > +					 "compressed" : "uncompressed",
> > +				 param.compression_format, regtxt,
> > +				 copyfns[copy_function].suffix);
> > -				vm = xe_vm_create(xe, 0, 0);
> > -				exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> > -				sync_bind = syncobj_create(xe, 0);
> > -				sync_out = syncobj_create(xe, 0);
> > -				ctx = intel_ctx_xe(xe, vm, exec_queue,
> > -						   0, sync_bind, sync_out);
> > +			if (!config->width_increment) {
> > +				igt_dynamic(testname)
> > +					single_copy(xe, config, region1, region2,
> > +						    param.width, param.height,
> > +						    tiling, copy_function);
> > +			} else {
> > +				for (int w = param.width;
> > +				     w < param.width + config->width_steps;
> > +				     w += config->width_increment) {
> > +					snprintf(testname, sizeof(testname),
> > +						 "%s-%s-compfmt%d-%s%s-%dx%d",
> > +						 blt_tiling_name(tiling),
> > +						 config->compression ?
> > +							 "compressed" : "uncompressed",
> > +						 param.compression_format, regtxt,
> > +						 copyfns[copy_function].suffix,
> > +						 w, w);
> > +					igt_dynamic(testname)
> > +						single_copy(xe, config, region1, region2,
> > +							    w, w, tiling, copy_function);
> > +				}
> > -				copyfns[copy_function].copyfn(xe, ctx,
> > -							      region1, region2,
> > -							      tiling, config);
> > -
> > -				xe_exec_queue_destroy(xe, exec_queue);
> > -				xe_vm_destroy(xe, vm);
> > -				syncobj_destroy(xe, sync_bind);
> > -				syncobj_destroy(xe, sync_out);
> > -				free(ctx);
> >   			}
> >   			free(regtxt);
> > @@ -669,6 +728,16 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
> >   		block_copy_test(xe, &config, set, BLOCK_COPY);
> >   	}
> > +	igt_describe("Check block-copy uncompressed blit with increment width/height");
> > +	igt_subtest_with_dynamic("block-copy-uncompressed-inc-dimension") {
> > +		struct test_config config = { .width_increment = 15,
> > +					      .width_steps = 512 };
> > +		param.width = 1;
> > +		param.height = 1;
> > +
> > +		block_copy_test(xe, &config, set, BLOCK_COPY);
> > +	}
> > +
> >   	igt_describe("Check block-copy flatccs compressed blit");
> >   	igt_subtest_with_dynamic("block-copy-compressed") {
> >   		struct test_config config = { .compression = true };
> > @@ -676,6 +745,17 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
> >   		block_copy_test(xe, &config, set, BLOCK_COPY);
> >   	}
> > +	igt_describe("Check block-copy compressed blit with increment width/height");
> > +	igt_subtest_with_dynamic("block-copy-compressed-inc-dimension") {
> > +		struct test_config config = { .compression = true,
> > +					      .width_increment = 15,
> > +					      .width_steps = 512 };
> > +		param.width = 1;
> > +		param.height = 1;
> > +
> > +		block_copy_test(xe, &config, set, BLOCK_COPY);
> > +	}
> > +
> >   	igt_describe("Check block-multicopy flatccs compressed blit");
> >   	igt_subtest_with_dynamic("block-multicopy-compressed") {
> >   		struct test_config config = { .compression = true };

Re: [PATCH i-g-t v4 4/5] tests/xe-ccs: Add tests which exercise small to large blit sizes

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 03:10:51PM +0100, Karolina Stolarek wrote:
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > Testing block-copy for 512 x 512 x 32bpp is not enough to verify
> > blit is working for different (small) and not always aligned
> > resolutions. Add 'increment' subtests which checks blits from
> > small (1x1) to large (512x512) resolutions.
> > 
> > To avoid too long execution resolution increment equals 15x15 pixels.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> > ---
> >   tests/intel/xe_ccs.c | 144 +++++++++++++++++++++++++++++++++----------
> >   1 file changed, 112 insertions(+), 32 deletions(-)
> 
> Ah, one question actually. We use width == height, and I wonder if we want
> to test rectangular surfaces. I think that caused problems in the past, but
> maybe I'm misremembering something.

Good question. I also thought to increment w/h depending on tile size
but due to unnecessary code complication I dropped this idea. Going
toward 512x512 in square shape imo is good enough to catch out of
boundary accesses (try decreasing stride/aligned height by half in helpers
and see how tests are failing).

--
Zbigniew

> 
> All the best,
> Karolina
> 
> > 
> > diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> > index a7785edcb1..627976049c 100644
> > --- a/tests/intel/xe_ccs.c
> > +++ b/tests/intel/xe_ccs.c
> > @@ -28,9 +28,15 @@
> >    * SUBTEST: block-copy-compressed
> >    * Description: Check block-copy flatccs compressed blit
> >    *
> > + * SUBTEST: block-copy-compressed-inc-dimension
> > + * Description: Check block-copy compressed blit for different sizes
> > + *
> >    * SUBTEST: block-copy-uncompressed
> >    * Description: Check block-copy uncompressed blit
> >    *
> > + * SUBTEST: block-copy-uncompressed-inc-dimension
> > + * Description: Check block-copy uncompressed blit for different sizes
> > + *
> >    * SUBTEST: block-multicopy-compressed
> >    * Description: Check block-multicopy flatccs compressed blit
> >    *
> > @@ -73,6 +79,8 @@ struct test_config {
> >   	bool surfcopy;
> >   	bool new_ctx;
> >   	bool suspend_resume;
> > +	int width_increment;
> > +	int width_steps;
> >   };
> >   #define PRINT_SURFACE_INFO(name, obj) do { \
> > @@ -286,9 +294,17 @@ static int blt_block_copy3(int xe,
> >   	return ret;
> >   }
> > +#define CHECK_MIN_WIDTH 2
> > +#define CHECK_MIN_HEIGHT 2
> > +#define MIN_EXP_WH(w, h) ((w) >= CHECK_MIN_WIDTH && (h) >= CHECK_MIN_HEIGHT)
> > +#define CHECK_FROM_WIDTH 256
> > +#define CHECK_FROM_HEIGHT 256
> > +#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
> > +
> >   static void block_copy(int xe,
> >   		       intel_ctx_t *ctx,
> >   		       uint32_t region1, uint32_t region2,
> > +		       uint32_t width, uint32_t height,
> >   		       enum blt_tiling_type mid_tiling,
> >   		       const struct test_config *config)
> >   {
> > @@ -301,7 +317,7 @@ static void block_copy(int xe,
> >   	uint32_t run_id = mid_tiling;
> >   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
> >   							!xe_has_vram(xe)) ? region1 : region2;
> > -	uint32_t width = param.width, height = param.height, bb;
> > +	uint32_t bb;
> >   	enum blt_compression mid_compression = config->compression;
> >   	int mid_compression_format = param.compression_format;
> >   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> > @@ -339,9 +355,16 @@ static void block_copy(int xe,
> >   	blt_block_copy(xe, ctx, NULL, ahnd, &blt, pext);
> >   	intel_ctx_xe_sync(ctx, true);
> > -	/* We expect mid != src if there's compression */
> > -	if (mid->compression)
> > -		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> > +	/*
> > +	 * We expect mid != src if there's compression. Ignore this for small
> > +	 * width x height for linear as compression for gradient occurs in the
> > +	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
> > +	 * xmajor.
> > +	 */
> > +	if (mid->compression && MIN_EXP_WH(width, height)) {
> > +		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
> > +			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> > +	}
> >   	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
> > @@ -416,6 +439,7 @@ static void block_copy(int xe,
> >   static void block_multicopy(int xe,
> >   			    intel_ctx_t *ctx,
> >   			    uint32_t region1, uint32_t region2,
> > +			    uint32_t width, uint32_t height,
> >   			    enum blt_tiling_type mid_tiling,
> >   			    const struct test_config *config)
> >   {
> > @@ -429,7 +453,7 @@ static void block_multicopy(int xe,
> >   	uint32_t run_id = mid_tiling;
> >   	uint32_t mid_region = (AT_LEAST_GEN(intel_get_drm_devid(xe), 20) &
> >   							!xe_has_vram(xe)) ? region1 : region2;
> > -	uint32_t width = param.width, height = param.height, bb;
> > +	uint32_t bb;
> >   	enum blt_compression mid_compression = config->compression;
> >   	int mid_compression_format = param.compression_format;
> >   	enum blt_compression_type comp_type = COMPRESSION_TYPE_3D;
> > @@ -521,6 +545,7 @@ static const struct {
> >   	void (*copyfn)(int fd,
> >   		       intel_ctx_t *ctx,
> >   		       uint32_t region1, uint32_t region2,
> > +		       uint32_t width, uint32_t height,
> >   		       enum blt_tiling_type btype,
> >   		       const struct test_config *config);
> >   } copyfns[] = {
> > @@ -528,17 +553,44 @@ static const struct {
> >   	[BLOCK_MULTICOPY] = { "-multicopy", block_multicopy },
> >   };
> > -static void block_copy_test(int xe,
> > -			    const struct test_config *config,
> > -			    struct igt_collection *set,
> > -			    enum copy_func copy_function)
> > +static void single_copy(int xe, const struct test_config *config,
> > +			int32_t region1, uint32_t region2,
> > +			uint32_t width, uint32_t height,
> > +			int tiling, enum copy_func copy_function)
> >   {
> >   	struct drm_xe_engine_class_instance inst = {
> >   		.engine_class = DRM_XE_ENGINE_CLASS_COPY,
> >   	};
> > +	uint32_t vm, exec_queue;
> > +	uint32_t sync_bind, sync_out;
> >   	intel_ctx_t *ctx;
> > +
> > +	vm = xe_vm_create(xe, 0, 0);
> > +	exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> > +	sync_bind = syncobj_create(xe, 0);
> > +	sync_out = syncobj_create(xe, 0);
> > +	ctx = intel_ctx_xe(xe, vm, exec_queue,
> > +			   0, sync_bind, sync_out);
> > +
> > +	copyfns[copy_function].copyfn(xe, ctx,
> > +				      region1, region2,
> > +				      width, height,
> > +				      tiling, config);
> > +
> > +	xe_exec_queue_destroy(xe, exec_queue);
> > +	xe_vm_destroy(xe, vm);
> > +	syncobj_destroy(xe, sync_bind);
> > +	syncobj_destroy(xe, sync_out);
> > +	free(ctx);
> > +}
> > +
> > +static void block_copy_test(int xe,
> > +			    const struct test_config *config,
> > +			    struct igt_collection *set,
> > +			    enum copy_func copy_function)
> > +{
> > +
> >   	struct igt_collection *regions;
> > -	uint32_t vm, exec_queue;
> >   	int tiling;
> >   	if (config->compression && !blt_block_copy_supports_compression(xe))
> > @@ -555,6 +607,7 @@ static void block_copy_test(int xe,
> >   		for_each_variation_r(regions, 2, set) {
> >   			uint32_t region1, region2;
> >   			char *regtxt;
> > +			char testname[256];
> >   			region1 = igt_collection_get_value(regions, 0);
> >   			region2 = igt_collection_get_value(regions, 1);
> > @@ -566,30 +619,36 @@ static void block_copy_test(int xe,
> >   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
> > -			igt_dynamic_f("%s-%s-compfmt%d-%s%s",
> > -				      blt_tiling_name(tiling),
> > -				      config->compression ?
> > -					      "compressed" : "uncompressed",
> > -				      param.compression_format, regtxt,
> > -				      copyfns[copy_function].suffix) {
> > -				uint32_t sync_bind, sync_out;
> > +			snprintf(testname, sizeof(testname),
> > +				 "%s-%s-compfmt%d-%s%s",
> > +				 blt_tiling_name(tiling),
> > +				 config->compression ?
> > +					 "compressed" : "uncompressed",
> > +				 param.compression_format, regtxt,
> > +				 copyfns[copy_function].suffix);
> > -				vm = xe_vm_create(xe, 0, 0);
> > -				exec_queue = xe_exec_queue_create(xe, vm, &inst, 0);
> > -				sync_bind = syncobj_create(xe, 0);
> > -				sync_out = syncobj_create(xe, 0);
> > -				ctx = intel_ctx_xe(xe, vm, exec_queue,
> > -						   0, sync_bind, sync_out);
> > +			if (!config->width_increment) {
> > +				igt_dynamic(testname)
> > +					single_copy(xe, config, region1, region2,
> > +						    param.width, param.height,
> > +						    tiling, copy_function);
> > +			} else {
> > +				for (int w = param.width;
> > +				     w < param.width + config->width_steps;
> > +				     w += config->width_increment) {
> > +					snprintf(testname, sizeof(testname),
> > +						 "%s-%s-compfmt%d-%s%s-%dx%d",
> > +						 blt_tiling_name(tiling),
> > +						 config->compression ?
> > +							 "compressed" : "uncompressed",
> > +						 param.compression_format, regtxt,
> > +						 copyfns[copy_function].suffix,
> > +						 w, w);
> > +					igt_dynamic(testname)
> > +						single_copy(xe, config, region1, region2,
> > +							    w, w, tiling, copy_function);
> > +				}
> > -				copyfns[copy_function].copyfn(xe, ctx,
> > -							      region1, region2,
> > -							      tiling, config);
> > -
> > -				xe_exec_queue_destroy(xe, exec_queue);
> > -				xe_vm_destroy(xe, vm);
> > -				syncobj_destroy(xe, sync_bind);
> > -				syncobj_destroy(xe, sync_out);
> > -				free(ctx);
> >   			}
> >   			free(regtxt);
> > @@ -669,6 +728,16 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
> >   		block_copy_test(xe, &config, set, BLOCK_COPY);
> >   	}
> > +	igt_describe("Check block-copy uncompressed blit with increment width/height");
> > +	igt_subtest_with_dynamic("block-copy-uncompressed-inc-dimension") {
> > +		struct test_config config = { .width_increment = 15,
> > +					      .width_steps = 512 };
> > +		param.width = 1;
> > +		param.height = 1;
> > +
> > +		block_copy_test(xe, &config, set, BLOCK_COPY);
> > +	}
> > +
> >   	igt_describe("Check block-copy flatccs compressed blit");
> >   	igt_subtest_with_dynamic("block-copy-compressed") {
> >   		struct test_config config = { .compression = true };
> > @@ -676,6 +745,17 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
> >   		block_copy_test(xe, &config, set, BLOCK_COPY);
> >   	}
> > +	igt_describe("Check block-copy compressed blit with increment width/height");
> > +	igt_subtest_with_dynamic("block-copy-compressed-inc-dimension") {
> > +		struct test_config config = { .compression = true,
> > +					      .width_increment = 15,
> > +					      .width_steps = 512 };
> > +		param.width = 1;
> > +		param.height = 1;
> > +
> > +		block_copy_test(xe, &config, set, BLOCK_COPY);
> > +	}
> > +
> >   	igt_describe("Check block-multicopy flatccs compressed blit");
> >   	igt_subtest_with_dynamic("block-multicopy-compressed") {
> >   		struct test_config config = { .compression = true };

Re: [PATCH i-g-t v4 5/5] tests/xe_exercise_blt: Exercise small to large fast-copy blits

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 03:18:02PM +0100, Karolina Stolarek wrote:
> 
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > Testing blitter with large size like 512x512 may be not enough
> > to verify small or unaligned blits works properly. Add subtest
> > which spread fast-copy blits from small to large.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik@intel.com>
> > ---
> >   tests/intel/xe_exercise_blt.c | 63 +++++++++++++++++++++++++++++------
> >   1 file changed, 52 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tests/intel/xe_exercise_blt.c b/tests/intel/xe_exercise_blt.c
> > index ddf9d188a7..8b4bae282b 100644
> > --- a/tests/intel/xe_exercise_blt.c
> > +++ b/tests/intel/xe_exercise_blt.c
> > @@ -25,6 +25,10 @@
> >    *   Check fast-copy blit
> >    *   blitter
> >    *
> > + * SUBTEST: fast-copy-inc-dimension
> > + * Description:
> > + *   Check fast-copy blit with sizes from small to large
> > + *
> >    * SUBTEST: fast-copy-emit
> >    * Description:
> >    *   Check multiple fast-copy in one batch
> > @@ -40,6 +44,8 @@ static struct param {
> >   	bool print_surface_info;
> >   	int width;
> >   	int height;
> > +	int width_increment;
> > +	int width_steps;
> >   } param = {
> >   	.tiling = -1,
> >   	.write_png = false,
> > @@ -111,6 +117,7 @@ static int fast_copy_one_bb(int xe,
> >   }
> >   static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
> > +			   uint32_t width, uint32_t height,
> >   			   uint32_t region1, uint32_t region2,
> >   			   enum blt_tiling_type mid_tiling)
> >   {
> > @@ -122,7 +129,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
> >   	uint64_t ahnd = intel_allocator_open_full(xe, ctx->vm, 0, 0,
> >   						  INTEL_ALLOCATOR_SIMPLE,
> >   						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
> > -	uint32_t bb, width = param.width, height = param.height;
> > +	uint32_t bb;
> >   	int result;
> >   	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
> > @@ -170,6 +177,7 @@ static void fast_copy_emit(int xe, const intel_ctx_t *ctx,
> >   }
> >   static void fast_copy(int xe, const intel_ctx_t *ctx,
> > +		      uint32_t width, uint32_t height,
> >   		      uint32_t region1, uint32_t region2,
> >   		      enum blt_tiling_type mid_tiling)
> >   {
> > @@ -181,7 +189,6 @@ static void fast_copy(int xe, const intel_ctx_t *ctx,
> >   						  INTEL_ALLOCATOR_SIMPLE,
> >   						  ALLOC_STRATEGY_LOW_TO_HIGH, 0);
> >   	uint32_t bb;
> > -	uint32_t width = param.width, height = param.height;
> >   	int result;
> >   	bb = xe_bo_create(xe, 0, bb_size, region1, 0);
> > @@ -241,14 +248,20 @@ enum fast_copy_func {
> >   };
> >   static char
> > -	*full_subtest_str(char *regtxt, enum blt_tiling_type tiling,
> > +	*full_subtest_str(char *regtxt, uint32_t width, uint32_t height,
> > +			  enum blt_tiling_type tiling,
> >   			  enum fast_copy_func func)
> >   {
> >   	char *name;
> >   	uint32_t len;
> > -	len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
> > -		       func == FAST_COPY_EMIT ? "-emit" : "");
> > +	if (!width || !height)
> > +		len = asprintf(&name, "%s-%s%s", blt_tiling_name(tiling), regtxt,
> > +			       func == FAST_COPY_EMIT ? "-emit" : "");
> > +	else
> > +		len = asprintf(&name, "%s-%s%s-%ux%u", blt_tiling_name(tiling), regtxt,
> > +			       func == FAST_COPY_EMIT ? "-emit" : "",
> > +			       width, height);
> >   	igt_assert_f(len >= 0, "asprintf failed!\n");
> > @@ -264,6 +277,7 @@ static void fast_copy_test(int xe,
> >   	};
> >   	struct igt_collection *regions;
> >   	void (*copy_func)(int xe, const intel_ctx_t *ctx,
> > +			  uint32_t width, uint32_t height,
> >   			  uint32_t r1, uint32_t r2, enum blt_tiling_type tiling);
> >   	intel_ctx_t *ctx;
> >   	int tiling;
> > @@ -286,16 +300,32 @@ static void fast_copy_test(int xe,
> >   			copy_func = (func == FAST_COPY) ? fast_copy : fast_copy_emit;
> >   			regtxt = xe_memregion_dynamic_subtest_name(xe, regions);
> > -			test_name = full_subtest_str(regtxt, tiling, func);
> > -			igt_dynamic_f("%s", test_name) {
> > -				copy_func(xe, ctx,
> > -					  region1, region2,
> > -					  tiling);
> > +			if (!param.width_increment) {
> > +				test_name = full_subtest_str(regtxt, 0, 0, tiling, func);
> 
> I'd simply pass param.width and param.height here and use the common
> name for both width_increment and fixed size test cases. We don't have
> to keep the same "%s-%s%s" format, extending it would be fine (and
> helpful, I think).

I wanted to keep subtest name for previous test to avoid changes on
patchwork. That's why I passed 0 here.

--
Zbigniew

> 
> All the best,
> Karolina
> 
> > +				igt_dynamic_f("%s", test_name) {
> > +					copy_func(xe, ctx,
> > +						  param.width, param.height,
> > +						  region1, region2,
> > +						  tiling);
> > +				}
> > +				free(test_name);
> > +			} else {
> > +				for (int w = param.width;
> > +				     w < param.width + param.width_steps;
> > +				     w += param.width_increment) {
> > +					test_name = full_subtest_str(regtxt, w, w, tiling, func);
> > +					igt_dynamic_f("%s", test_name) {
> > +						copy_func(xe, ctx,
> > +							  w, w,
> > +							  region1, region2,
> > +							  tiling);
> > +					}
> > +					free(test_name);
> > +				}
> >   			}
> >   			free(regtxt);
> > -			free(test_name);
> >   			xe_exec_queue_destroy(xe, exec_queue);
> >   			xe_vm_destroy(xe, vm);
> >   			free(ctx);
> > @@ -367,6 +397,17 @@ igt_main_args("b:pst:W:H:", NULL, help_str, opt_handler, NULL)
> >   		fast_copy_test(xe, set, FAST_COPY);
> >   	}
> > +	igt_describe("Check fast-copy with increment width/height");
> > +	igt_subtest_with_dynamic("fast-copy-inc-dimension") {
> > +		param.width = 1;
> > +		param.height = 1;
> > +		param.width_increment = 15;
> > +		param.width_steps = 512;
> > +
> > +		fast_copy_test(xe, set, FAST_COPY);
> > +	}
> > +
> > +
> >   	igt_describe("Check multiple fast-copy in one batch");
> >   	igt_subtest_with_dynamic("fast-copy-emit") {
> >   		fast_copy_test(xe, set, FAST_COPY_EMIT);

Re: [PATCH i-g-t v4 5/5] tests/xe_exercise_blt: Exercise small to large fast-copy blits

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 03:23:54PM +0100, Karolina Stolarek wrote:
> Ah, forgot to tell you that checkpatch was slightly uhappy with two blank
> lines after inc-dimension subtest...
> 
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> (...)
> > +	igt_describe("Check fast-copy with increment width/height");
> > +	igt_subtest_with_dynamic("fast-copy-inc-dimension") {
> > +		param.width = 1;
> > +		param.height = 1;
> > +		param.width_increment = 15;
> > +		param.width_steps = 512;
> > +
> > +		fast_copy_test(xe, set, FAST_COPY);
> > +	}
> > +
> > +
>  ^-- here!
> 
> Sorry for the email noise.

No problem at all, thanks for spotting this. I'll resubmit on v5.

--
Zbigniew

> 
> All the best,
> Karolina
> 
> >   	igt_describe("Check multiple fast-copy in one batch");
> >   	igt_subtest_with_dynamic("fast-copy-emit") {
> >   		fast_copy_test(xe, set, FAST_COPY_EMIT);