2022-07-09 12:02:37

by José Expósito

[permalink] [raw]
Subject: [PATCH v2 0/4] KUnit tests for RGB565 conversion

Hello everyone,

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

The first patch fixes a bug reported by David Gow in the XRGB8888 to
RGB332 tests.
Note that the fix is required for this format because internally, the
drm_fb_xrgb8888_to_rgb332_line() function, calls le32_to_cpu().
The other *_line() functiones don't change the endian. That's why the
RGB565 tests don't need to make any endian conversions.

I'm not sure whether this inconsistency handling the endian could be
considered a bug or not, but at list it is confusing. It'd be
interesting to hear the opinion of the maintainers on this topic.

Patches 2 and 3 make the test generic and the last one tests
drm_fb_xrgb8888_to_rgb565().

Best wishes,
José Expósito

Changes since v1:

- Fix a bug reported by David Gow in the XRGB8888 to RGB332 tests
- Simplify the test structure as suggested by David Gow
- Add Tested-by Tales L. Aparecida and Acked-by Thomas Zimmermann
- Fix link in the last patch (Thomas Zimmermann)

José Expósito (4):
drm/format-helper: Fix test on big endian architectures
drm/format-helper: Rename test cases to make them more generic
drm/format-helper: Support multiple target formats results
drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb565()

.../gpu/drm/tests/drm_format_helper_test.c | 167 ++++++++++++++----
1 file changed, 136 insertions(+), 31 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
prerequisite-patch-id: 04e8b96dd1319088e45d4b4dc4beb29889b30941
prerequisite-patch-id: bda9f93309910ca6d9528c9c560a3ccb7c388bfa
prerequisite-patch-id: f45a301abf3874302b47751378d1cb4da01a1e2a
prerequisite-patch-id: 7e8d74a4769f5bc8f2a1ced1de64aa13ea7b9d6c
prerequisite-patch-id: cfa9be5a3b48cada74c681ebe967ef2a3ac572a0
prerequisite-patch-id: f4d0b5ba40437486248a6df6ab022a3428898273
prerequisite-patch-id: f6380c88b6640cd57b867a59ed42a2fc754fa9e4
prerequisite-patch-id: 112685a3c337b3bc8ff9507ab832c6322a77a2cc
prerequisite-patch-id: 3f69109f64a8b7ac85a0332d177c8b2216cc747c
prerequisite-patch-id: 7f0a3edb0b94bd54681d67c50a23008ccdb18c00
prerequisite-patch-id: bfb6024a0e542cdc89f1f2a98cc249a2c071c68f
prerequisite-patch-id: b569ec32d7a4a9c7be317e8a8f0177ad4f696a10
prerequisite-patch-id: f1f7adf96de1501469c223a50d6405dc4b378982
prerequisite-patch-id: ce194443e4d40ded9bcec371717566398333e1db
prerequisite-patch-id: 253ba111d18eea436c45fc43216b7d3ddba41388
prerequisite-patch-id: fac3f1b822c5154bc16eae9eb97b95fa19c178ad
prerequisite-patch-id: 7eeca32e531b0d97d67285c106c1e721158da931
prerequisite-patch-id: 2b1b9ce615e8856f7e236f5fbaf7dfcd625da2ea
prerequisite-patch-id: 6e45198758680c3ed23f58ee049d93f63d316d88
prerequisite-patch-id: 58c3a87ebf0a31d65a3a787121a066013e1da82e
prerequisite-patch-id: e62f9ef9e892d075778094ee5b4e4e9d9cc76c86
prerequisite-patch-id: 3a7abafa7b011cbeca304f8af85b88aefc55cf64
prerequisite-patch-id: 1146b840a8ff43a4fdc8382c6c4d133aae6ec2fa
prerequisite-patch-id: 1455f1117f208d983759d89b750440d349a880c2
prerequisite-patch-id: 92d0eb8a4e06da97b18a5cf28a81fc90158c444d
prerequisite-patch-id: 2ab38ae9cea6d979bff04345550a1a848b55a091
prerequisite-patch-id: 25b591d59400b972ed1b709d932feaba0d5f642e
prerequisite-patch-id: 003dca01353bba78d1a05baa5852acb1b313a154
prerequisite-patch-id: f7beda6d5b2aa56ecd5ce610f36db704d1fbd653
prerequisite-patch-id: f641abe09200a8b9507124949bab294a05529175
prerequisite-patch-id: 1fa4294e769b40dca3b89fa160c4ae1ff9eff8b7
prerequisite-patch-id: 9d04c01b752031e874286e121a64844891008a14
prerequisite-patch-id: f34a698723d7df6b6a4d17686395af8b694186e2
prerequisite-patch-id: 488ab7cec987c0a6c8c032fc74ad79a40bf546f9
prerequisite-patch-id: a5a4c847fb9b55f53eaef99b353749f61dd1f72f
prerequisite-patch-id: 443d79ee1834963cb1f2a4e0007d02e419a2f5ff
prerequisite-patch-id: 15add4faa533058e2ef962aeb7563835c6cbd82a
prerequisite-patch-id: 6afb078b02085c4b24136bff54941a11090ea738
prerequisite-patch-id: da36728d198481270fb9695c7d86d210111353c2
prerequisite-patch-id: eb412a9ebab86d80a96f3c32690e5a96ed5a9ebc
prerequisite-patch-id: e33a4314f9bb9caf819f5e350c50b5b571d68e37
prerequisite-patch-id: 10ec9def77f602a29e20ac1b4193bbc7e932a7f5
prerequisite-patch-id: 970edbb64a43f50ac7e80f2f0258767a08b8294c
prerequisite-patch-id: f089485485d6e374fca925da2b904867c8e60a74
prerequisite-patch-id: b60e812dbb263d1b9c27f46bf487f4f73568a9d0
prerequisite-patch-id: 6344323c6242f0fa4402003f63cac9bc63fc7752
prerequisite-patch-id: b4d1a179227563da227593d7588306c9e676c6af
prerequisite-patch-id: 48dbb90b0dbd77ad1e6ae8101804d23bb855645f
prerequisite-patch-id: f104eb773c8c0b0ead12b8576ce0052c646cf598
prerequisite-patch-id: 4e6659a16896cb1546dc272a7db2893873d3d7c0
prerequisite-patch-id: ea56ba8675748e2fb5a942244a77a17ca1650de5
prerequisite-patch-id: df4cd3120fe4bb1ae6322ae908739fb1f620e660
prerequisite-patch-id: 533aee0b212f50f0dcf705b7e04c626340f4bd36
prerequisite-patch-id: 599329c73d4c591a95c8031a41c080115a6d602b
prerequisite-patch-id: 0f27a6299d98e481e523a93f2c494079b4c2ad4a
prerequisite-patch-id: 7621b25a47ee43ae20862c285e0b2d236311788b
prerequisite-patch-id: 72b2b9c39cf28cf2c2282231838184caba0b250d
prerequisite-patch-id: 80bf2499edfee73e4e4672619ee4da47df53d4c5
prerequisite-patch-id: 97745925e55ae5c66e379a637f0c432b5cd4c5a9
prerequisite-patch-id: 76d682950bf55d9b6a6e4df9dcd52278f534d41c
prerequisite-patch-id: 4bf1847249b002130e4e59d8a83ad6af0b27508f
prerequisite-patch-id: 1760ca541aa2cf8e892d7e6b8620b566f5d9bd05
prerequisite-patch-id: f63faffa66e94f9f330fda71dfc9f4a55b4f3d07
prerequisite-patch-id: cb8e14eefb9813f539ba1a5b5f78be5df351888d
prerequisite-patch-id: d42116fc0ae5898cdcf0d47d99866a0e328d27c7
prerequisite-patch-id: bc81a5b598bdc0e91286cef08957f5ed627b6a41
prerequisite-patch-id: 693319f739fbffa8be024f7a6ef8265caa48ef1b
prerequisite-patch-id: e10dc3d38993222d15a095f33f5150c1998d2c62
prerequisite-patch-id: 6b2a39c25f3938b86cb71ed70b7330ded34e0a2e
prerequisite-patch-id: cde017ae3f0c1691dcb437095804ef5207520f9a
prerequisite-patch-id: 1c79ba2de78c4fbf91a4371c32f8e31bcffe5493
prerequisite-patch-id: 00079a82e7089c6aacb7610953b6fbe99167dba1
prerequisite-patch-id: 609e49dc054b83b9e4b5961e9a7c956a3d54ab51
prerequisite-patch-id: a8394a54421133ddfaeb486d13a135479beb12ad
prerequisite-patch-id: 25eaeb161a40979bfa8ca4ee6246a5338d61e972
prerequisite-patch-id: be89047878944cfb969e544179f42008a3b9dae4
prerequisite-patch-id: 45bc4815468fc4c240b9291e87195ba6a56c8ced
prerequisite-patch-id: df771a5eda31e2bf8a1d46f6d38049d76fd8fa3c
prerequisite-patch-id: 3031536f5d0e359d3ff190b08e8dd8b0a1cd9bff
prerequisite-patch-id: 4c241d97f6c49494f73978ec24691b91e5ef96af
prerequisite-patch-id: a0c4a7df919ffcfd2698d7a8aaf8ae08e5005153
prerequisite-patch-id: dd85b9081eb9bc7e15a54ac958a844afc88f6716
prerequisite-patch-id: fdee5e717d385329c229a42b7aea83d29ab6d2b6
prerequisite-patch-id: 7caeb6f71b1d618fe310bb05cc5f3e6b48bd16e9
prerequisite-patch-id: ed364b5c9a5014df39654a6ba626baa7f67cf99a
prerequisite-patch-id: 56d211f7e8c7601370370e07bedff01f9f3cd4df
prerequisite-patch-id: 1c43718b51bf2e0ae545367b48b2a353f0f7289f
--
2.25.1


2022-07-09 12:18:36

by José Expósito

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

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.

Tested-by: Tales L. Aparecida <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
Signed-off-by: José Expósito <[email protected]>
Link: http://www.barth-dev.de/online/rgb565-color-picker/ # [1]
---
.../gpu/drm/tests/drm_format_helper_test.c | 76 ++++++++++++++++++-
1 file changed, 75 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 0a490ad4fd32..c0592c1235cf 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -21,12 +21,19 @@ struct convert_to_rgb332_result {
const u8 expected[TEST_BUF_SIZE];
};

+struct convert_to_rgb565_result {
+ unsigned int dst_pitch;
+ const u16 expected[TEST_BUF_SIZE];
+ const u16 expected_swab[TEST_BUF_SIZE];
+};
+
struct convert_xrgb8888_case {
const char *name;
unsigned int pitch;
struct drm_rect clip;
const u32 xrgb8888[TEST_BUF_SIZE];
struct convert_to_rgb332_result rgb332_result;
+ struct convert_to_rgb565_result rgb565_result;
};

static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
@@ -39,6 +46,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
.dst_pitch = 0,
.expected = { 0xE0 },
},
+ .rgb565_result = {
+ .dst_pitch = 0,
+ .expected = { 0xF800 },
+ .expected_swab = { 0x00F8 },
+ },
},
{
.name = "single_pixel_clip_rectangle",
@@ -52,6 +64,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
.dst_pitch = 0,
.expected = { 0xE0 },
},
+ .rgb565_result = {
+ .dst_pitch = 0,
+ .expected = { 0xF800 },
+ .expected_swab = { 0x00F8 },
+ },
},
{
/* Well known colors: White, black, red, green, blue, magenta,
@@ -77,6 +94,21 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
0xFC, 0x1F,
},
},
+ .rgb565_result = {
+ .dst_pitch = 0,
+ .expected = {
+ 0xFFFF, 0x0000,
+ 0xF800, 0x07E0,
+ 0x001F, 0xF81F,
+ 0xFFE0, 0x07FF,
+ },
+ .expected_swab = {
+ 0xFFFF, 0x0000,
+ 0x00F8, 0xE007,
+ 0x1F00, 0x1FF8,
+ 0xE0FF, 0xFF07,
+ },
+ },
},
{
/* Randomly picked colors. Full buffer within the clip area. */
@@ -96,6 +128,19 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
0xA0, 0x6D, 0x0A, 0x00, 0x00,
},
},
+ .rgb565_result = {
+ .dst_pitch = 10,
+ .expected = {
+ 0x0A33, 0x1260, 0xA800, 0x0000, 0x0000,
+ 0x6B8E, 0x0A33, 0x1260, 0x0000, 0x0000,
+ 0xA800, 0x6B8E, 0x0A33, 0x0000, 0x0000,
+ },
+ .expected_swab = {
+ 0x330A, 0x6012, 0x00A8, 0x0000, 0x0000,
+ 0x8E6B, 0x330A, 0x6012, 0x0000, 0x0000,
+ 0x00A8, 0x8E6B, 0x330A, 0x0000, 0x0000,
+ },
+ },
},
};

@@ -120,7 +165,7 @@ static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
if (!dst_pitch)
dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];

- return dst_pitch * drm_rect_height(clip);
+ return (dst_pitch * drm_rect_height(clip)) / (dst_fi->depth / 8);
}

static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
@@ -175,8 +220,37 @@ static void xrgb8888_to_rgb332_test(struct kunit *test)
KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
}

+static void xrgb8888_to_rgb565_test(struct kunit *test)
+{
+ const struct convert_xrgb8888_case *params = test->param_value;
+ const struct convert_to_rgb565_result *result = &params->rgb565_result;
+ size_t dst_size;
+ __u16 *dst = NULL;
+
+ struct drm_framebuffer fb = {
+ .format = drm_format_info(DRM_FORMAT_XRGB8888),
+ .pitches = { params->pitch, 0, 0 },
+ };
+
+ dst_size = conversion_buf_size(DRM_FORMAT_RGB565, 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);
+
+ drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
+ &params->clip, false);
+ KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
+
+ drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
+ &params->clip, true);
+ KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected_swab, dst_size), 0);
+}
+
static struct kunit_case drm_format_helper_test_cases[] = {
KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test, convert_xrgb8888_gen_params),
+ KUNIT_CASE_PARAM(xrgb8888_to_rgb565_test, convert_xrgb8888_gen_params),
{}
};

--
2.25.1

2022-07-16 09:35:42

by David Gow

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

On Sat, Jul 9, 2022 at 7:58 PM José Expósito <[email protected]> wrote:
>
> 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.
>
> Tested-by: Tales L. Aparecida <[email protected]>
> Acked-by: Thomas Zimmermann <[email protected]>
> Signed-off-by: José Expósito <[email protected]>
> Link: http://www.barth-dev.de/online/rgb565-color-picker/ # [1]
> ---

Looks good and passes here.

Reviewed-by: David Gow <[email protected]>

Thanks,
-- David


> .../gpu/drm/tests/drm_format_helper_test.c | 76 ++++++++++++++++++-
> 1 file changed, 75 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 0a490ad4fd32..c0592c1235cf 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -21,12 +21,19 @@ struct convert_to_rgb332_result {
> const u8 expected[TEST_BUF_SIZE];
> };
>
> +struct convert_to_rgb565_result {
> + unsigned int dst_pitch;
> + const u16 expected[TEST_BUF_SIZE];
> + const u16 expected_swab[TEST_BUF_SIZE];
> +};
> +
> struct convert_xrgb8888_case {
> const char *name;
> unsigned int pitch;
> struct drm_rect clip;
> const u32 xrgb8888[TEST_BUF_SIZE];
> struct convert_to_rgb332_result rgb332_result;
> + struct convert_to_rgb565_result rgb565_result;
> };
>
> static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> @@ -39,6 +46,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xE0 },
> },
> + .rgb565_result = {
> + .dst_pitch = 0,
> + .expected = { 0xF800 },
> + .expected_swab = { 0x00F8 },
> + },
> },
> {
> .name = "single_pixel_clip_rectangle",
> @@ -52,6 +64,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> .dst_pitch = 0,
> .expected = { 0xE0 },
> },
> + .rgb565_result = {
> + .dst_pitch = 0,
> + .expected = { 0xF800 },
> + .expected_swab = { 0x00F8 },
> + },
> },
> {
> /* Well known colors: White, black, red, green, blue, magenta,
> @@ -77,6 +94,21 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> 0xFC, 0x1F,
> },
> },
> + .rgb565_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0xFFFF, 0x0000,
> + 0xF800, 0x07E0,
> + 0x001F, 0xF81F,
> + 0xFFE0, 0x07FF,
> + },
> + .expected_swab = {
> + 0xFFFF, 0x0000,
> + 0x00F8, 0xE007,
> + 0x1F00, 0x1FF8,
> + 0xE0FF, 0xFF07,
> + },
> + },
> },
> {
> /* Randomly picked colors. Full buffer within the clip area. */
> @@ -96,6 +128,19 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> 0xA0, 0x6D, 0x0A, 0x00, 0x00,
> },
> },
> + .rgb565_result = {
> + .dst_pitch = 10,
> + .expected = {
> + 0x0A33, 0x1260, 0xA800, 0x0000, 0x0000,
> + 0x6B8E, 0x0A33, 0x1260, 0x0000, 0x0000,
> + 0xA800, 0x6B8E, 0x0A33, 0x0000, 0x0000,
> + },
> + .expected_swab = {
> + 0x330A, 0x6012, 0x00A8, 0x0000, 0x0000,
> + 0x8E6B, 0x330A, 0x6012, 0x0000, 0x0000,
> + 0x00A8, 0x8E6B, 0x330A, 0x0000, 0x0000,
> + },
> + },
> },
> };
>
> @@ -120,7 +165,7 @@ static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> if (!dst_pitch)
> dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
>
> - return dst_pitch * drm_rect_height(clip);
> + return (dst_pitch * drm_rect_height(clip)) / (dst_fi->depth / 8);
> }
>
> static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
> @@ -175,8 +220,37 @@ static void xrgb8888_to_rgb332_test(struct kunit *test)
> KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
> }
>
> +static void xrgb8888_to_rgb565_test(struct kunit *test)
> +{
> + const struct convert_xrgb8888_case *params = test->param_value;
> + const struct convert_to_rgb565_result *result = &params->rgb565_result;
> + size_t dst_size;
> + __u16 *dst = NULL;
> +
> + struct drm_framebuffer fb = {
> + .format = drm_format_info(DRM_FORMAT_XRGB8888),
> + .pitches = { params->pitch, 0, 0 },
> + };
> +
> + dst_size = conversion_buf_size(DRM_FORMAT_RGB565, 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);
> +
> + drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
> + &params->clip, false);
> + KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
> +
> + drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
> + &params->clip, true);
> + KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected_swab, dst_size), 0);
> +}
> +
> static struct kunit_case drm_format_helper_test_cases[] = {
> KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test, convert_xrgb8888_gen_params),
> + KUNIT_CASE_PARAM(xrgb8888_to_rgb565_test, convert_xrgb8888_gen_params),
> {}
> };
>
> --
> 2.25.1
>


Attachments:
smime.p7s (3.91 kB)
S/MIME Cryptographic Signature

2022-07-17 17:11:59

by José Expósito

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

Jos? Exp?sito <[email protected]> wrote:
> I already fixed the warning and added the reviewed by tags, however, I
> noticed that rebasing the series on the latest drm-misc-next show this
> error:
> [...]

Sorry for the extra email. I forgot to mention that the error is only
present in UML. Running in other architectures works as expected.
Tested on x86_64 and PowerPC.

Jose

2022-07-17 17:15:52

by José Expósito

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

Hi David,

On Sat, Jul 16, 2022 at 05:32:51PM +0800, David Gow wrote:
> On Sat, Jul 9, 2022 at 7:58 PM Jos? Exp?sito <[email protected]> wrote:
> >
> > 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.
> >
> > Tested-by: Tales L. Aparecida <[email protected]>
> > Acked-by: Thomas Zimmermann <[email protected]>
> > Signed-off-by: Jos? Exp?sito <[email protected]>
> > Link: http://www.barth-dev.de/online/rgb565-color-picker/ # [1]
> > ---
>
> Looks good and passes here.
>
> Reviewed-by: David Gow <[email protected]>
>
> Thanks,
> -- David

Thanks a lot for reviewing the series and for pointing out the Sparse
warning.

I already fixed the warning and added the reviewed by tags, however, I
noticed that rebasing the series on the latest drm-misc-next show this
error:

[18:49:32] ============================================================
[18:49:33] =========== drm_format_helper_test (2 subtests) ============
[18:49:33] ================= xrgb8888_to_rgb332_test ==================
[18:49:33] [ERROR] Test: xrgb8888_to_rgb332_test: missing subtest result line!
[18:49:33] [ERROR] Test: xrgb8888_to_rgb332_test: 0 tests run!
[18:49:33] ========== [NO TESTS RUN] xrgb8888_to_rgb332_test ==========
[18:49:33] [ERROR] Test: drm_format_helper_test: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: drm_format_helper_test: missing subtest result line!
[18:49:33] # Subtest: drm_format_helper_test
[18:49:33] 1..2
[18:49:33] ============= [CRASHED] drm_format_helper_test =============
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] [ERROR] Test: main: missing expected subtest!
[18:49:33] [CRASHED]
[18:49:33] ============================================================
[18:49:33] Testing complete. Ran 10 tests: crashed: 10, errors: 13

I bisected drm-misc-next to find out that the first bad commit is:
e23a5e14aa278858c2e3d81ec34e83aa9a4177c5

Not very usefull, because that commit merges v5.19-rc6 into misc.

I tested on the latest kselftest-master branch and the error is not
present.

Are you aware of any change that could cause this issue?

Jose


> > .../gpu/drm/tests/drm_format_helper_test.c | 76 ++++++++++++++++++-
> > 1 file changed, 75 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 0a490ad4fd32..c0592c1235cf 100644
> > --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> > +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> > @@ -21,12 +21,19 @@ struct convert_to_rgb332_result {
> > const u8 expected[TEST_BUF_SIZE];
> > };
> >
> > +struct convert_to_rgb565_result {
> > + unsigned int dst_pitch;
> > + const u16 expected[TEST_BUF_SIZE];
> > + const u16 expected_swab[TEST_BUF_SIZE];
> > +};
> > +
> > struct convert_xrgb8888_case {
> > const char *name;
> > unsigned int pitch;
> > struct drm_rect clip;
> > const u32 xrgb8888[TEST_BUF_SIZE];
> > struct convert_to_rgb332_result rgb332_result;
> > + struct convert_to_rgb565_result rgb565_result;
> > };
> >
> > static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> > @@ -39,6 +46,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> > .dst_pitch = 0,
> > .expected = { 0xE0 },
> > },
> > + .rgb565_result = {
> > + .dst_pitch = 0,
> > + .expected = { 0xF800 },
> > + .expected_swab = { 0x00F8 },
> > + },
> > },
> > {
> > .name = "single_pixel_clip_rectangle",
> > @@ -52,6 +64,11 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> > .dst_pitch = 0,
> > .expected = { 0xE0 },
> > },
> > + .rgb565_result = {
> > + .dst_pitch = 0,
> > + .expected = { 0xF800 },
> > + .expected_swab = { 0x00F8 },
> > + },
> > },
> > {
> > /* Well known colors: White, black, red, green, blue, magenta,
> > @@ -77,6 +94,21 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> > 0xFC, 0x1F,
> > },
> > },
> > + .rgb565_result = {
> > + .dst_pitch = 0,
> > + .expected = {
> > + 0xFFFF, 0x0000,
> > + 0xF800, 0x07E0,
> > + 0x001F, 0xF81F,
> > + 0xFFE0, 0x07FF,
> > + },
> > + .expected_swab = {
> > + 0xFFFF, 0x0000,
> > + 0x00F8, 0xE007,
> > + 0x1F00, 0x1FF8,
> > + 0xE0FF, 0xFF07,
> > + },
> > + },
> > },
> > {
> > /* Randomly picked colors. Full buffer within the clip area. */
> > @@ -96,6 +128,19 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> > 0xA0, 0x6D, 0x0A, 0x00, 0x00,
> > },
> > },
> > + .rgb565_result = {
> > + .dst_pitch = 10,
> > + .expected = {
> > + 0x0A33, 0x1260, 0xA800, 0x0000, 0x0000,
> > + 0x6B8E, 0x0A33, 0x1260, 0x0000, 0x0000,
> > + 0xA800, 0x6B8E, 0x0A33, 0x0000, 0x0000,
> > + },
> > + .expected_swab = {
> > + 0x330A, 0x6012, 0x00A8, 0x0000, 0x0000,
> > + 0x8E6B, 0x330A, 0x6012, 0x0000, 0x0000,
> > + 0x00A8, 0x8E6B, 0x330A, 0x0000, 0x0000,
> > + },
> > + },
> > },
> > };
> >
> > @@ -120,7 +165,7 @@ static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> > if (!dst_pitch)
> > dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> >
> > - return dst_pitch * drm_rect_height(clip);
> > + return (dst_pitch * drm_rect_height(clip)) / (dst_fi->depth / 8);
> > }
> >
> > static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
> > @@ -175,8 +220,37 @@ static void xrgb8888_to_rgb332_test(struct kunit *test)
> > KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
> > }
> >
> > +static void xrgb8888_to_rgb565_test(struct kunit *test)
> > +{
> > + const struct convert_xrgb8888_case *params = test->param_value;
> > + const struct convert_to_rgb565_result *result = &params->rgb565_result;
> > + size_t dst_size;
> > + __u16 *dst = NULL;
> > +
> > + struct drm_framebuffer fb = {
> > + .format = drm_format_info(DRM_FORMAT_XRGB8888),
> > + .pitches = { params->pitch, 0, 0 },
> > + };
> > +
> > + dst_size = conversion_buf_size(DRM_FORMAT_RGB565, 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);
> > +
> > + drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
> > + &params->clip, false);
> > + KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected, dst_size), 0);
> > +
> > + drm_fb_xrgb8888_to_rgb565(dst, result->dst_pitch, params->xrgb8888, &fb,
> > + &params->clip, true);
> > + KUNIT_EXPECT_EQ(test, memcmp(dst, result->expected_swab, dst_size), 0);
> > +}
> > +
> > static struct kunit_case drm_format_helper_test_cases[] = {
> > KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test, convert_xrgb8888_gen_params),
> > + KUNIT_CASE_PARAM(xrgb8888_to_rgb565_test, convert_xrgb8888_gen_params),
> > {}
> > };
> >
> > --
> > 2.25.1
> >


2022-08-10 16:50:45

by Daniel Vetter

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

On Sun, Jul 17, 2022 at 07:00:54PM +0200, Jos? Exp?sito wrote:
> Jos? Exp?sito <[email protected]> wrote:
> > I already fixed the warning and added the reviewed by tags, however, I
> > noticed that rebasing the series on the latest drm-misc-next show this
> > error:
> > [...]
>
> Sorry for the extra email. I forgot to mention that the error is only
> present in UML. Running in other architectures works as expected.
> Tested on x86_64 and PowerPC.

Maybe a regression in the kunit infrastructure? Just guessing here ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2022-08-10 16:51:46

by Daniel Latypov

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

On Sun, Jul 17, 2022 at 10:01 AM José Expósito
<[email protected]> wrote:
>
> José Expósito <[email protected]> wrote:
> > I already fixed the warning and added the reviewed by tags, however, I
> > noticed that rebasing the series on the latest drm-misc-next show this
> > error:
> > [...]
>
> Sorry for the extra email. I forgot to mention that the error is only
> present in UML. Running in other architectures works as expected.
> Tested on x86_64 and PowerPC.

Can you take a look at the raw output?

It would be .kunit/test.log (or replace .kunit with w/e --build_dir
you're using).
You could also run the test with --raw_output to have kunit.py print
that out instead.
We might want to compare the output on UML vs x86 and see what looks suspicious.

These errors
[ERROR] Test: xrgb8888_to_rgb332_test: missing subtest result line!
means that kunit.py can't parse the KTAP output.

It could be that the output is mangled for some reason.
I recall running into a UML-specific issue with output mangling on an
old kernel fork. I doubt it's related to this one, but it shows that
it's possible there could be something going on.

-Daniel

2022-08-10 17:45:11

by Daniel Latypov

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

On Wed, Aug 10, 2022 at 9:31 AM Daniel Vetter <[email protected]> wrote:
>
> On Sun, Jul 17, 2022 at 07:00:54PM +0200, José Expósito wrote:
> > José Expósito <[email protected]> wrote:
> > > I already fixed the warning and added the reviewed by tags, however, I
> > > noticed that rebasing the series on the latest drm-misc-next show this
> > > error:
> > > [...]
> >
> > Sorry for the extra email. I forgot to mention that the error is only
> > present in UML. Running in other architectures works as expected.
> > Tested on x86_64 and PowerPC.
>
> Maybe a regression in the kunit infrastructure? Just guessing here ...
> -Daniel

As noted elsewhere on the thread, these errors means that kunit.py
can't parse the KTAP output coming from the kernel.
There shouldn't be any recent changes in either the python-side parser
or the kernel-side output logic.

So I can't think of a kunit-specific change that would trigger this.
There could be some other issue causing output mangling.

We'd want to look at what output the UML kernel produces (stored in
.kunit/test.log, or visible via kunit.py run --raw_output).

-A different Daniel

2022-08-13 11:08:37

by José Expósito

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

On Wed, Aug 10, 2022 at 09:41:18AM -0700, Daniel Latypov wrote:
> On Sun, Jul 17, 2022 at 10:01 AM Jos? Exp?sito
> <[email protected]> wrote:
> >
> > Jos? Exp?sito <[email protected]> wrote:
> > > I already fixed the warning and added the reviewed by tags, however, I
> > > noticed that rebasing the series on the latest drm-misc-next show this
> > > error:
> > > [...]
> >
> > Sorry for the extra email. I forgot to mention that the error is only
> > present in UML. Running in other architectures works as expected.
> > Tested on x86_64 and PowerPC.
>
> Can you take a look at the raw output?
>
> It would be .kunit/test.log (or replace .kunit with w/e --build_dir
> you're using).
> You could also run the test with --raw_output to have kunit.py print
> that out instead.
> We might want to compare the output on UML vs x86 and see what looks suspicious.
>
> These errors
> [ERROR] Test: xrgb8888_to_rgb332_test: missing subtest result line!
> means that kunit.py can't parse the KTAP output.
>
> It could be that the output is mangled for some reason.
> I recall running into a UML-specific issue with output mangling on an
> old kernel fork. I doubt it's related to this one, but it shows that
> it's possible there could be something going on.
>
> -Daniel

Hi!

Sorry for not clarifying the error in this thread.
I fixed it in v3 (already merged).

The issue was in my implementation. I was overwriting a few bytes of
memory due to an out of bounds bug. Thanks to the crash I mentioned,
I detected it before the code got merged.

Thanks a lot for the pointers Daniel, the next time I'll check
.kunit/test.log for usefull information.

Jose