[PATCH 0/4] KUnit tests for RGB565 conversion

[PATCH 0/4] KUnit tests for RGB565 conversion

[José Expósito]

Hello everyone,

This series is a follow up of the XRGB8888 to RGB332 conversion KUnit tests.

The first 3 patches refactor the existing test to make them agnostic of the target format and add support for "swab".

The last patch adds the RGB565 conversion values, and shows how more formats will be easily added in the future.

Thank you very much in advance for your feedback,
José Expósito

José Expósito (4):
  drm/format-helper: Rename test cases to make them more generic
  drm/format-helper: Transform tests to be agnostic of target format
  drm/format-helper: Add support for conversion functions with swab
  drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()

 .../gpu/drm/tests/drm_format_helper_test.c    | 231 +++++++++++++++---
 1 file changed, 196 insertions(+), 35 deletions(-)


base-commit: 6fde8eec71796f3534f0c274066862829813b21f
prerequisite-patch-id: 8a16f4c8004d6161035eaea275c8eafaa0ac927e
prerequisite-patch-id: 53fded2a49e6212b546db76ec52563a683752e65
prerequisite-patch-id: 294b0ca27a6ee57096c8f097c0572336b8a2d583
prerequisite-patch-id: 5e05bfc5287d16c207bfc616b2776ad72eb4ab29
prerequisite-patch-id: e94560be85dffb62a5b3cf58d1f0fc3d278ad806
prerequisite-patch-id: a471df39c7b32c69dd2b138a7d0af015ea42e00a
-- 
2.25.1

[PATCH 1/4] drm/format-helper: Rename test cases to make them more generic

[José Expósito]

The tests available at the moment only check the conversion from
XRGB8888 to RGB332. However, more conversion will be tested in the
future.

In order to make the struct and functions present in the tests more
generic, rename xrgb8888_to_rgb332_* to convert_xrgb8888_*.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 .../gpu/drm/tests/drm_format_helper_test.c    | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 98583bf56044..de8cf525109e 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -16,7 +16,7 @@
 
 #define TEST_BUF_SIZE 50
 
-struct xrgb8888_to_rgb332_case {
+struct convert_xrgb8888_case {
 	const char *name;
 	unsigned int pitch;
 	unsigned int dst_pitch;
@@ -25,7 +25,7 @@ struct xrgb8888_to_rgb332_case {
 	const u8 expected[4 * TEST_BUF_SIZE];
 };
 
-static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
+static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 	{
 		.name = "single_pixel_source_buffer",
 		.pitch = 1 * 4,
@@ -111,18 +111,18 @@ static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
 	return dst_pitch * drm_rect_height(clip);
 }
 
-static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
-					 char *desc)
+static void convert_xrgb8888_case_desc(struct convert_xrgb8888_case *t,
+				       char *desc)
 {
 	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
 }
 
-KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
-		  xrgb8888_to_rgb332_case_desc);
+KUNIT_ARRAY_PARAM(convert_xrgb8888, convert_xrgb8888_cases,
+		  convert_xrgb8888_case_desc);
 
-static void xrgb8888_to_rgb332_test(struct kunit *test)
+static void convert_xrgb8888_test(struct kunit *test)
 {
-	const struct xrgb8888_to_rgb332_case *params = test->param_value;
+	const struct convert_xrgb8888_case *params = test->param_value;
 	size_t dst_size;
 	__u8 *dst = NULL;
 
@@ -144,8 +144,7 @@ static void xrgb8888_to_rgb332_test(struct kunit *test)
 }
 
 static struct kunit_case drm_format_helper_test_cases[] = {
-	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
-			 xrgb8888_to_rgb332_gen_params),
+	KUNIT_CASE_PARAM(convert_xrgb8888_test, convert_xrgb8888_gen_params),
 	{}
 };
 
-- 
2.25.1

[PATCH 2/4] drm/format-helper: Transform tests to be agnostic of target format

[José Expósito]

In order to support multiple destination format conversions, store the
target format, conversion function, parameters and expected result in
its own structure.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 .../gpu/drm/tests/drm_format_helper_test.c    | 88 ++++++++++++++-----
 1 file changed, 64 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index de8cf525109e..732d945e7f4e 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -16,34 +16,55 @@
 
 #define TEST_BUF_SIZE 50
 
+struct convert_xrgb8888_result {
+	u32 dst_format;
+	void (*conv_func)(void *dst, unsigned int dst_pitch,
+			  const void *src,
+			  const struct drm_framebuffer *fb,
+			  const struct drm_rect *clip);
+	unsigned int dst_pitch;
+	const u8 expected[4 * TEST_BUF_SIZE];
+};
+
 struct convert_xrgb8888_case {
 	const char *name;
 	unsigned int pitch;
-	unsigned int dst_pitch;
 	struct drm_rect clip;
 	const u32 xrgb8888[TEST_BUF_SIZE];
-	const u8 expected[4 * TEST_BUF_SIZE];
+	struct convert_xrgb8888_result results[1];
 };
 
 static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 	{
 		.name = "single_pixel_source_buffer",
 		.pitch = 1 * 4,
-		.dst_pitch = 0,
 		.clip = DRM_RECT_INIT(0, 0, 1, 1),
 		.xrgb8888 = { 0x01FF0000 },
-		.expected = { 0xE0 },
+		.results = {
+			{
+				.dst_format = DRM_FORMAT_RGB332,
+				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.dst_pitch = 0,
+				.expected = { 0xE0 },
+			},
+		},
 	},
 	{
 		.name = "single_pixel_clip_rectangle",
 		.pitch = 2 * 4,
-		.dst_pitch = 0,
 		.clip = DRM_RECT_INIT(1, 1, 1, 1),
 		.xrgb8888 = {
 			0x00000000, 0x00000000,
 			0x00000000, 0x10FF0000,
 		},
-		.expected = { 0xE0 },
+		.results = {
+			{
+				.dst_format = DRM_FORMAT_RGB332,
+				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.dst_pitch = 0,
+				.expected = { 0xE0 },
+			},
+		},
 	},
 	{
 		/* Well known colors: White, black, red, green, blue, magenta,
@@ -52,7 +73,6 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 		 */
 		.name = "well_known_colors",
 		.pitch = 4 * 4,
-		.dst_pitch = 0,
 		.clip = DRM_RECT_INIT(1, 1, 2, 4),
 		.xrgb8888 = {
 			0x00000000, 0x00000000, 0x00000000, 0x00000000,
@@ -61,28 +81,41 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
 			0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
 		},
-		.expected = {
-			0xFF, 0x00,
-			0xE0, 0x1C,
-			0x03, 0xE3,
-			0xFC, 0x1F,
+		.results = {
+			{
+				.dst_format = DRM_FORMAT_RGB332,
+				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.dst_pitch = 0,
+				.expected = {
+					0xFF, 0x00,
+					0xE0, 0x1C,
+					0x03, 0xE3,
+					0xFC, 0x1F,
+				},
+			},
 		},
 	},
 	{
 		/* Randomly picked colors. Full buffer within the clip area. */
 		.name = "destination_pitch",
 		.pitch = 3 * 4,
-		.dst_pitch = 5,
 		.clip = DRM_RECT_INIT(0, 0, 3, 3),
 		.xrgb8888 = {
 			0xA10E449C, 0xB1114D05, 0xC1A80303,
 			0xD16C7073, 0xA20E449C, 0xB2114D05,
 			0xC2A80303, 0xD26C7073, 0xA30E449C,
 		},
-		.expected = {
-			0x0A, 0x08, 0xA0, 0x00, 0x00,
-			0x6D, 0x0A, 0x08, 0x00, 0x00,
-			0xA0, 0x6D, 0x0A, 0x00, 0x00,
+		.results = {
+			{
+				.dst_format = DRM_FORMAT_RGB332,
+				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.dst_pitch = 5,
+				.expected = {
+					0x0A, 0x08, 0xA0, 0x00, 0x00,
+					0x6D, 0x0A, 0x08, 0x00, 0x00,
+					0xA0, 0x6D, 0x0A, 0x00, 0x00,
+				},
+			},
 		},
 	},
 };
@@ -123,24 +156,31 @@ KUNIT_ARRAY_PARAM(convert_xrgb8888, convert_xrgb8888_cases,
 static void convert_xrgb8888_test(struct kunit *test)
 {
 	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_xrgb8888_result *result;
 	size_t dst_size;
 	__u8 *dst = NULL;
+	int n;
 
 	struct drm_framebuffer fb = {
 		.format = drm_format_info(DRM_FORMAT_XRGB8888),
 		.pitches = { params->pitch, 0, 0 },
 	};
 
-	dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
-				       &params->clip);
-	KUNIT_ASSERT_GT(test, dst_size, 0);
+	for (n = 0; n < ARRAY_SIZE(params->results); n++) {
+		result = &params->results[n];
+
+		dst_size = conversion_buf_size(result->dst_format,
+					       result->dst_pitch,
+					       &params->clip);
+		KUNIT_ASSERT_GT(test, dst_size, 0);
 
-	dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
+		dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
 
-	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
+		result->conv_func(dst, result->dst_pitch, params->xrgb8888,
 				  &fb, &params->clip);
-	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
+		KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
+	}
 }
 
 static struct kunit_case drm_format_helper_test_cases[] = {
-- 
2.25.1

[PATCH 3/4] drm/format-helper: Add support for conversion functions with swab

[José Expósito]

The RGB565 conversion functions take an extra parameter ("swab")
indicating whether the bytes should be swapped into the clip buffer or
not.

Create a union in the "convert_xrgb8888_result" structure holding the
value of the "swab" parameter as well as the conversion function
pointer.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 .../gpu/drm/tests/drm_format_helper_test.c    | 44 ++++++++++++++-----
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 732d945e7f4e..52dc41cc7c60 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -16,12 +16,29 @@
 
 #define TEST_BUF_SIZE 50
 
+struct convert_xrgb8888_func {
+	void (*func)(void *dst, unsigned int dst_pitch,
+		     const void *src,
+		     const struct drm_framebuffer *fb,
+		     const struct drm_rect *clip);
+};
+
+struct convert_xrgb8888_func_swab {
+	void (*func)(void *dst, unsigned int dst_pitch,
+		     const void *src,
+		     const struct drm_framebuffer *fb,
+		     const struct drm_rect *clip,
+		     bool swab);
+	bool swab;
+};
+
 struct convert_xrgb8888_result {
 	u32 dst_format;
-	void (*conv_func)(void *dst, unsigned int dst_pitch,
-			  const void *src,
-			  const struct drm_framebuffer *fb,
-			  const struct drm_rect *clip);
+	bool has_swab;
+	union {
+		struct convert_xrgb8888_func conv;
+		struct convert_xrgb8888_func_swab conv_swab;
+	};
 	unsigned int dst_pitch;
 	const u8 expected[4 * TEST_BUF_SIZE];
 };
@@ -43,7 +60,7 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 		.results = {
 			{
 				.dst_format = DRM_FORMAT_RGB332,
-				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.conv = { .func = drm_fb_xrgb8888_to_rgb332 },
 				.dst_pitch = 0,
 				.expected = { 0xE0 },
 			},
@@ -60,7 +77,7 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 		.results = {
 			{
 				.dst_format = DRM_FORMAT_RGB332,
-				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.conv = { .func = drm_fb_xrgb8888_to_rgb332 },
 				.dst_pitch = 0,
 				.expected = { 0xE0 },
 			},
@@ -84,7 +101,7 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 		.results = {
 			{
 				.dst_format = DRM_FORMAT_RGB332,
-				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.conv = { .func = drm_fb_xrgb8888_to_rgb332 },
 				.dst_pitch = 0,
 				.expected = {
 					0xFF, 0x00,
@@ -108,7 +125,7 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 		.results = {
 			{
 				.dst_format = DRM_FORMAT_RGB332,
-				.conv_func = drm_fb_xrgb8888_to_rgb332,
+				.conv = { .func = drm_fb_xrgb8888_to_rgb332 },
 				.dst_pitch = 5,
 				.expected = {
 					0x0A, 0x08, 0xA0, 0x00, 0x00,
@@ -177,8 +194,15 @@ static void convert_xrgb8888_test(struct kunit *test)
 		dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
 		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
 
-		result->conv_func(dst, result->dst_pitch, params->xrgb8888,
-				  &fb, &params->clip);
+		if (result->has_swab) {
+			result->conv_swab.func(dst, result->dst_pitch,
+					       params->xrgb8888, &fb,
+					       &params->clip,
+					       result->conv_swab.swab);
+		} else {
+			result->conv.func(dst, result->dst_pitch,
+					  params->xrgb8888, &fb, &params->clip);
+		}
 		KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
 	}
 }
-- 
2.25.1

[PATCH 4/4] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()

[José Expósito]

Extend the existing test cases to test the conversion from XRGB8888 to
RGB565.

The documentation and the color picker available on [1] are useful
resources to understand this patch and validate the values returned by
the conversion function.

[1] http://www.barth-dev.de/online/rgb565-color-picker/

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 .../gpu/drm/tests/drm_format_helper_test.c    | 100 +++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 52dc41cc7c60..3fbe8026bccc 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -48,7 +48,7 @@ struct convert_xrgb8888_case {
 	unsigned int pitch;
 	struct drm_rect clip;
 	const u32 xrgb8888[TEST_BUF_SIZE];
-	struct convert_xrgb8888_result results[1];
+	struct convert_xrgb8888_result results[3];
 };
 
 static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
@@ -64,6 +64,26 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				.dst_pitch = 0,
 				.expected = { 0xE0 },
 			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = false,
+				},
+				.dst_pitch = 0,
+				.expected = { 0x00, 0xF8 },
+			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = true,
+				},
+				.dst_pitch = 0,
+				.expected = { 0xF8, 0x00 },
+			},
 		},
 	},
 	{
@@ -81,6 +101,26 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				.dst_pitch = 0,
 				.expected = { 0xE0 },
 			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = false,
+				},
+				.dst_pitch = 0,
+				.expected = { 0x00, 0xF8 },
+			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = true,
+				},
+				.dst_pitch = 0,
+				.expected = { 0xF8, 0x00 },
+			},
 		},
 	},
 	{
@@ -110,6 +150,36 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 					0xFC, 0x1F,
 				},
 			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = false,
+				},
+				.dst_pitch = 0,
+				.expected = {
+					0xFF, 0xFF, 0x00, 0x00,
+					0x00, 0xF8, 0xE0, 0x07,
+					0x1F, 0x00, 0x1F, 0xF8,
+					0xE0, 0xFF, 0xFF, 0x07,
+				},
+			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = true,
+				},
+				.dst_pitch = 0,
+				.expected = {
+					0xFF, 0xFF, 0x00, 0x00,
+					0xF8, 0x00, 0x07, 0xE0,
+					0x00, 0x1F, 0xF8, 0x1F,
+					0xFF, 0xE0, 0x07, 0xFF,
+				},
+			},
 		},
 	},
 	{
@@ -133,6 +203,34 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 					0xA0, 0x6D, 0x0A, 0x00, 0x00,
 				},
 			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = false,
+				},
+				.dst_pitch = 10,
+				.expected = {
+					0x33, 0x0A, 0x60, 0x12, 0x00, 0xA8, 0x00, 0x00, 0x00, 0x00,
+					0x8E, 0x6B, 0x33, 0x0A, 0x60, 0x12, 0x00, 0x00, 0x00, 0x00,
+					0x00, 0xA8, 0x8E, 0x6B, 0x33, 0x0A, 0x00, 0x00, 0x00, 0x00,
+				},
+			},
+			{
+				.dst_format = DRM_FORMAT_RGB565,
+				.has_swab = true,
+				.conv_swab = {
+					.func = drm_fb_xrgb8888_to_rgb565,
+					.swab = true,
+				},
+				.dst_pitch = 10,
+				.expected = {
+					0x0A, 0x33, 0x12, 0x60, 0xA8, 0x00, 0x00, 0x00, 0x00, 0x00,
+					0x6B, 0x8E, 0x0A, 0x33, 0x12, 0x60, 0x00, 0x00, 0x00, 0x00,
+					0xA8, 0x00, 0x6B, 0x8E, 0x0A, 0x33, 0x00, 0x00, 0x00, 0x00,
+				},
+			},
 		},
 	},
 };
-- 
2.25.1

Re: [PATCH 0/4] KUnit tests for RGB565 conversion

[Tales]

Em seg., 27 de jun. de 2022 às 13:13, José Expósito
<jose.exposito89@gmail.com> escreveu:
>
> Hello everyone,
>
> This series is a follow up of the XRGB8888 to RGB332 conversion KUnit tests.
>
> The first 3 patches refactor the existing test to make them agnostic of the target format and add support for "swab".
>
> The last patch adds the RGB565 conversion values, and shows how more formats will be easily added in the future.
>
> Thank you very much in advance for your feedback,
> José Expósito
>
> José Expósito (4):
>   drm/format-helper: Rename test cases to make them more generic
>   drm/format-helper: Transform tests to be agnostic of target format
>   drm/format-helper: Add support for conversion functions with swab
>   drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()
>
>  .../gpu/drm/tests/drm_format_helper_test.c    | 231 +++++++++++++++---
>  1 file changed, 196 insertions(+), 35 deletions(-)
>
>
> base-commit: 6fde8eec71796f3534f0c274066862829813b21f
> prerequisite-patch-id: 8a16f4c8004d6161035eaea275c8eafaa0ac927e
> prerequisite-patch-id: 53fded2a49e6212b546db76ec52563a683752e65
> prerequisite-patch-id: 294b0ca27a6ee57096c8f097c0572336b8a2d583
> prerequisite-patch-id: 5e05bfc5287d16c207bfc616b2776ad72eb4ab29
> prerequisite-patch-id: e94560be85dffb62a5b3cf58d1f0fc3d278ad806
> prerequisite-patch-id: a471df39c7b32c69dd2b138a7d0af015ea42e00a
> --
> 2.25.1


Tested with "./tools/testing/kunit/kunit.py run
--kunitconfig=drivers/gpu/drm/tests --arch=x86_64", "... --arch=i386"
and baremetal on x86_64 to be sure; everything looks fine, but I feel
like some patches could be squashed, though.

Tested-by: Tales L. Aparecida <tales.aparecida@gmail.com>

Inspiring work, José, keep it up!
Best regards, Tales

Re: [PATCH 0/4] KUnit tests for RGB565 conversion

[David Gow]

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

On Tue, Jun 28, 2022 at 12:13 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Hello everyone,
>
> This series is a follow up of the XRGB8888 to RGB332 conversion KUnit tests.
>
> The first 3 patches refactor the existing test to make them agnostic of the target format and add support for "swab".
>
> The last patch adds the RGB565 conversion values, and shows how more formats will be easily added in the future.
>
> Thank you very much in advance for your feedback,
> José Expósito
>
> José Expósito (4):
>   drm/format-helper: Rename test cases to make them more generic
>   drm/format-helper: Transform tests to be agnostic of target format
>   drm/format-helper: Add support for conversion functions with swab
>   drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()
>
>  .../gpu/drm/tests/drm_format_helper_test.c    | 231 +++++++++++++++---
>  1 file changed, 196 insertions(+), 35 deletions(-)
>
>
> base-commit: 6fde8eec71796f3534f0c274066862829813b21f
> prerequisite-patch-id: 8a16f4c8004d6161035eaea275c8eafaa0ac927e
> prerequisite-patch-id: 53fded2a49e6212b546db76ec52563a683752e65
> prerequisite-patch-id: 294b0ca27a6ee57096c8f097c0572336b8a2d583
> prerequisite-patch-id: 5e05bfc5287d16c207bfc616b2776ad72eb4ab29
> prerequisite-patch-id: e94560be85dffb62a5b3cf58d1f0fc3d278ad806
> prerequisite-patch-id: a471df39c7b32c69dd2b138a7d0af015ea42e00a
> --
> 2.25.1
>

These look pretty good overall to me, but there is one big issue
(which is actually with the previous series -- oops!), and a few small
stylistic thoughts.

For the big issue: these tests don't work on big-endian systems. The
'swab' bit in this series reminded me to check, and sure enough, all
of the tests fail (including the rgb332 ones).

I tested it on PowerPC with:
 ./tools/testing/kunit/kunit.py run
--kunitconfig=drivers/gpu/drm/tests --arch=powerpc
--cross_compile=powerpc64-linux-gnu-

So that's something which needs to be fixed.

The smaller stylistic thoughts basically all revolve around the
complexity of convert_xrgb8888_cases: there are arrays of structs with
arrays of unions of structs (with function pointers in them). This
isn't a problem: it's actually a lot more readable than that
description implies, but there are other ways it could be tackled
(which have their own tradeoffs, of course).

One possibility would be to split up the test into a separate test per
destination format. They could reuse the convert_xrgb8888_cases array,
and just each access a different result. You could make things even
clearer (IMO) by replacing the results[] array with a separate, named,
member (since you don't need to iterate over them any more), and
remove the need to have the function pointer and swab union/members by
just hardcoding those in the separate test functions. It'd also make
the results a bit clearer, as each destination format would get its
own separate set of results.

Of course, that's just an idea: I don't actually have a problem with
the existing design either (other than the endianness issue, of
course).

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 4/4] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4930 bytes --]

Hi

Am 27.06.22 um 18:11 schrieb José Expósito:
> Extend the existing test cases to test the conversion from XRGB8888 to
> RGB565.
> 
> The documentation and the color picker available on [1] are useful
> resources to understand this patch and validate the values returned by
> the conversion function.
> 
> [1] http://www.barth-dev.de/online/rgb565-color-picker/

URLs in commit messages are usually given as Link tag like this:

Link: http://www.barth-dev.de/online/rgb565-color-picker/ # [1]

Put this somewhere below your signed-off-by tag.

Best regards
Thomas

> 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   .../gpu/drm/tests/drm_format_helper_test.c    | 100 +++++++++++++++++-
>   1 file changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 52dc41cc7c60..3fbe8026bccc 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -48,7 +48,7 @@ struct convert_xrgb8888_case {
>   	unsigned int pitch;
>   	struct drm_rect clip;
>   	const u32 xrgb8888[TEST_BUF_SIZE];
> -	struct convert_xrgb8888_result results[1];
> +	struct convert_xrgb8888_result results[3];
>   };
>   
>   static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> @@ -64,6 +64,26 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   				.dst_pitch = 0,
>   				.expected = { 0xE0 },
>   			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = false,
> +				},
> +				.dst_pitch = 0,
> +				.expected = { 0x00, 0xF8 },
> +			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = true,
> +				},
> +				.dst_pitch = 0,
> +				.expected = { 0xF8, 0x00 },
> +			},
>   		},
>   	},
>   	{
> @@ -81,6 +101,26 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   				.dst_pitch = 0,
>   				.expected = { 0xE0 },
>   			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = false,
> +				},
> +				.dst_pitch = 0,
> +				.expected = { 0x00, 0xF8 },
> +			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = true,
> +				},
> +				.dst_pitch = 0,
> +				.expected = { 0xF8, 0x00 },
> +			},
>   		},
>   	},
>   	{
> @@ -110,6 +150,36 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   					0xFC, 0x1F,
>   				},
>   			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = false,
> +				},
> +				.dst_pitch = 0,
> +				.expected = {
> +					0xFF, 0xFF, 0x00, 0x00,
> +					0x00, 0xF8, 0xE0, 0x07,
> +					0x1F, 0x00, 0x1F, 0xF8,
> +					0xE0, 0xFF, 0xFF, 0x07,
> +				},
> +			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = true,
> +				},
> +				.dst_pitch = 0,
> +				.expected = {
> +					0xFF, 0xFF, 0x00, 0x00,
> +					0xF8, 0x00, 0x07, 0xE0,
> +					0x00, 0x1F, 0xF8, 0x1F,
> +					0xFF, 0xE0, 0x07, 0xFF,
> +				},
> +			},
>   		},
>   	},
>   	{
> @@ -133,6 +203,34 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>   					0xA0, 0x6D, 0x0A, 0x00, 0x00,
>   				},
>   			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = false,
> +				},
> +				.dst_pitch = 10,
> +				.expected = {
> +					0x33, 0x0A, 0x60, 0x12, 0x00, 0xA8, 0x00, 0x00, 0x00, 0x00,
> +					0x8E, 0x6B, 0x33, 0x0A, 0x60, 0x12, 0x00, 0x00, 0x00, 0x00,
> +					0x00, 0xA8, 0x8E, 0x6B, 0x33, 0x0A, 0x00, 0x00, 0x00, 0x00,
> +				},
> +			},
> +			{
> +				.dst_format = DRM_FORMAT_RGB565,
> +				.has_swab = true,
> +				.conv_swab = {
> +					.func = drm_fb_xrgb8888_to_rgb565,
> +					.swab = true,
> +				},
> +				.dst_pitch = 10,
> +				.expected = {
> +					0x0A, 0x33, 0x12, 0x60, 0xA8, 0x00, 0x00, 0x00, 0x00, 0x00,
> +					0x6B, 0x8E, 0x0A, 0x33, 0x12, 0x60, 0x00, 0x00, 0x00, 0x00,
> +					0xA8, 0x00, 0x6B, 0x8E, 0x0A, 0x33, 0x00, 0x00, 0x00, 0x00,
> +				},
> +			},
>   		},
>   	},
>   };

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] KUnit tests for RGB565 conversion

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1777 bytes --]

Hi

Am 27.06.22 um 18:11 schrieb José Expósito:
> Hello everyone,
> 
> This series is a follow up of the XRGB8888 to RGB332 conversion KUnit tests.
> 
> The first 3 patches refactor the existing test to make them agnostic of the target format and add support for "swab".
> 
> The last patch adds the RGB565 conversion values, and shows how more formats will be easily added in the future.

Thanks for your patches. I had one comment for the final one. With this 
fixed, you can add

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

to the series.

Best regards
Thomas

> 
> Thank you very much in advance for your feedback,
> José Expósito
> 
> José Expósito (4):
>    drm/format-helper: Rename test cases to make them more generic
>    drm/format-helper: Transform tests to be agnostic of target format
>    drm/format-helper: Add support for conversion functions with swab
>    drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()
> 
>   .../gpu/drm/tests/drm_format_helper_test.c    | 231 +++++++++++++++---
>   1 file changed, 196 insertions(+), 35 deletions(-)
> 
> 
> base-commit: 6fde8eec71796f3534f0c274066862829813b21f
> prerequisite-patch-id: 8a16f4c8004d6161035eaea275c8eafaa0ac927e
> prerequisite-patch-id: 53fded2a49e6212b546db76ec52563a683752e65
> prerequisite-patch-id: 294b0ca27a6ee57096c8f097c0572336b8a2d583
> prerequisite-patch-id: 5e05bfc5287d16c207bfc616b2776ad72eb4ab29
> prerequisite-patch-id: e94560be85dffb62a5b3cf58d1f0fc3d278ad806
> prerequisite-patch-id: a471df39c7b32c69dd2b138a7d0af015ea42e00a

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] KUnit tests for RGB565 conversion

[José Expósito]

Hi David,

Sorry for not getting back to you sooner, I've been swamped with work
this week.

On Wed, Jun 29, 2022 at 03:27:44PM +0800, David Gow wrote:
> These look pretty good overall to me, but there is one big issue
> (which is actually with the previous series -- oops!), and a few small
> stylistic thoughts.
> 
> For the big issue: these tests don't work on big-endian systems. The
> 'swab' bit in this series reminded me to check, and sure enough, all
> of the tests fail (including the rgb332 ones).
> 
> I tested it on PowerPC with:
>  ./tools/testing/kunit/kunit.py run
> --kunitconfig=drivers/gpu/drm/tests --arch=powerpc
> --cross_compile=powerpc64-linux-gnu-
> 
> So that's something which needs to be fixed.

Oops, yes, definitely something that I need to fix!
I'll include an extra patch at the beginning of v2 fixing this bug.

> The smaller stylistic thoughts basically all revolve around the
> complexity of convert_xrgb8888_cases: there are arrays of structs with
> arrays of unions of structs (with function pointers in them). This
> isn't a problem: it's actually a lot more readable than that
> description implies, but there are other ways it could be tackled
> (which have their own tradeoffs, of course).
> 
> One possibility would be to split up the test into a separate test per
> destination format. They could reuse the convert_xrgb8888_cases array,
> and just each access a different result. You could make things even
> clearer (IMO) by replacing the results[] array with a separate, named,
> member (since you don't need to iterate over them any more), and
> remove the need to have the function pointer and swab union/members by
> just hardcoding those in the separate test functions. It'd also make
> the results a bit clearer, as each destination format would get its
> own separate set of results.
> 
> Of course, that's just an idea: I don't actually have a problem with
> the existing design either (other than the endianness issue, of
> course).

I like from your approach that the output of the tests would be easier
to understand. At the moment, if a test fails, there is not enough
context to know which target format failed. I'll explore this approach
and see how it looks like.

Thanks,
Jose

> Cheers,
> -- David