Hi Thomas,
Thanks for CCing me.
There are some issues with this helpers on big endian architectures, you can
run the test on big endian with this command:
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
--arch=powerpc --cross_compile=powerpc64-linux-gnu- drm_format_helper_test
The problem is in the drm_fb_xrgb8888_to_*_line() functions, see the fixes
inlined:
On Tue, Dec 13, 2022 at 09:12:29PM +0100, Thomas Zimmermann wrote:
> Add conversion from XRGB8888 to XRGB1555, ARGB1555 and RGBA5551, which
> are the formats currently supported by the simplefb infrastructure. The
> new helpers allow the output of XRGB8888 framebuffers to firmware
> scanout buffers in one of the 15-bit formats.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> drivers/gpu/drm/drm_format_helper.c | 164 +++++++++++++++
> .../gpu/drm/tests/drm_format_helper_test.c | 186 ++++++++++++++++++
> include/drm/drm_format_helper.h | 9 +
> 3 files changed, 359 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index e562a3cefb89..aa6138756458 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -395,6 +395,161 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
> }
> EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>
> +static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +{
> + u16 *dbuf16 = dbuf;
> + const __le32 *sbuf32 = sbuf;
> + unsigned int x;
> + u16 val16;
> + u32 pix;
> +
> + for (x = 0; x < pixels; x++) {
> + pix = le32_to_cpu(sbuf32[x]);
> + val16 = ((pix & 0x00f80000) >> 9) |
> + ((pix & 0x0000f800) >> 6) |
> + ((pix & 0x000000f8) >> 3);
> + dbuf16[x] = cpu_to_le16(val16);
You don't need the extra cpu_to_le16() here:
- dbuf16[x] = cpu_to_le16(val16);
+ dbuf16[x] = val16;
> + }
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
> + * @dst: Array of XRGB1555 destination buffers
> + * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
> + * within @dst; can be NULL if scanlines are stored next to each other.
> + * @src: Array of XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * This function copies parts of a framebuffer to display memory and converts
> + * the color format during the process. The parameters @dst, @dst_pitch and
> + * @src refer to arrays. Each array must have at least as many entries as
> + * there are planes in @fb's format. Each entry stores the value for the
> + * format's respective color plane at the same index.
> + *
> + * This function does not apply clipping on @dst (i.e. the destination is at the
> + * top-left corner).
> + *
> + * Drivers can use this function for XRGB1555 devices that don't support
> + * XRGB8888 natively.
> + */
> +void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip)
> +{
> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> + 2,
> + };
> +
> + drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
> + drm_fb_xrgb8888_to_xrgb1555_line);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
> +
> +static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +{
> + u16 *dbuf16 = dbuf;
> + const __le32 *sbuf32 = sbuf;
> + unsigned int x;
> + u16 val16;
> + u32 pix;
> +
> + for (x = 0; x < pixels; x++) {
> + pix = le32_to_cpu(sbuf32[x]);
> + val16 = BIT(15) | /* set alpha bit */
> + ((pix & 0x00f80000) >> 9) |
> + ((pix & 0x0000f800) >> 6) |
> + ((pix & 0x000000f8) >> 3);
> + dbuf16[x] = cpu_to_le16(val16);
The same here:
- dbuf16[x] = cpu_to_le16(val16);
+ dbuf16[x] = val16;
> + }
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip buffer
> + * @dst: Array of ARGB1555 destination buffers
> + * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
> + * within @dst; can be NULL if scanlines are stored next to each other.
> + * @src: Array of XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * This function copies parts of a framebuffer to display memory and converts
> + * the color format during the process. The parameters @dst, @dst_pitch and
> + * @src refer to arrays. Each array must have at least as many entries as
> + * there are planes in @fb's format. Each entry stores the value for the
> + * format's respective color plane at the same index.
> + *
> + * This function does not apply clipping on @dst (i.e. the destination is at the
> + * top-left corner).
> + *
> + * Drivers can use this function for ARGB1555 devices that don't support
> + * XRGB8888 natively. It sets an opaque alpha channel as part of the conversion.
> + */
> +void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip)
> +{
> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> + 2,
> + };
> +
> + drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
> + drm_fb_xrgb8888_to_argb1555_line);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
> +
> +static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +{
> + u16 *dbuf16 = dbuf;
> + const __le32 *sbuf32 = sbuf;
> + unsigned int x;
> + u16 val16;
> + u32 pix;
> +
> + for (x = 0; x < pixels; x++) {
> + pix = le32_to_cpu(sbuf32[x]);
> + val16 = ((pix & 0x00f80000) >> 8) |
> + ((pix & 0x0000f800) >> 5) |
> + ((pix & 0x000000f8) >> 2) |
> + BIT(0); /* set alpha bit */
> + dbuf16[x] = cpu_to_le16(val16);
And this is the last fix:
- dbuf16[x] = cpu_to_le16(val16);
+ dbuf16[x] = val16;
Other than that, this patch looks good:
Reviewed-by: José Expósito <jose.exposito89@xxxxxxxxx>
Best wishes,
Jose
> + }
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip buffer
> + * @dst: Array of RGBA5551 destination buffers
> + * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
> + * within @dst; can be NULL if scanlines are stored next to each other.
> + * @src: Array of XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * This function copies parts of a framebuffer to display memory and converts
> + * the color format during the process. The parameters @dst, @dst_pitch and
> + * @src refer to arrays. Each array must have at least as many entries as
> + * there are planes in @fb's format. Each entry stores the value for the
> + * format's respective color plane at the same index.
> + *
> + * This function does not apply clipping on @dst (i.e. the destination is at the
> + * top-left corner).
> + *
> + * Drivers can use this function for RGBA5551 devices that don't support
> + * XRGB8888 natively. It sets an opaque alpha channel as part of the conversion.
> + */
> +void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip)
> +{
> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> + 2,
> + };
> +
> + drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
> + drm_fb_xrgb8888_to_rgba5551_line);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
> +
> static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> {
> u8 *dbuf8 = dbuf;
> @@ -761,6 +916,15 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
> if (dst_format == DRM_FORMAT_RGB565) {
> drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
> return 0;
> + } else if (dst_format == DRM_FORMAT_XRGB1555) {
> + drm_fb_xrgb8888_to_xrgb1555(dst, dst_pitch, src, fb, clip);
> + return 0;
> + } else if (dst_format == DRM_FORMAT_ARGB1555) {
> + drm_fb_xrgb8888_to_argb1555(dst, dst_pitch, src, fb, clip);
> + return 0;
> + } else if (dst_format == DRM_FORMAT_RGBA5551) {
> + drm_fb_xrgb8888_to_rgba5551(dst, dst_pitch, src, fb, clip);
> + return 0;
> } else if (dst_format == DRM_FORMAT_RGB888) {
> drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
> return 0;
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index b15dc7c1eb96..e01695c70bb6 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -32,6 +32,21 @@ struct convert_to_rgb565_result {
> const u16 expected_swab[TEST_BUF_SIZE];
> };
>
> +struct convert_to_xrgb1555_result {
> + unsigned int dst_pitch;
> + const u16 expected[TEST_BUF_SIZE];
> +};
> +
> +struct convert_to_argb1555_result {
> + unsigned int dst_pitch;
> + const u16 expected[TEST_BUF_SIZE];
> +};
> +
> +struct convert_to_rgba5551_result {
> + unsigned int dst_pitch;
> + const u16 expected[TEST_BUF_SIZE];
> +};
> +
> struct convert_to_rgb888_result {
> unsigned int dst_pitch;
> const u8 expected[TEST_BUF_SIZE];
> @@ -60,6 +75,9 @@ struct convert_xrgb8888_case {
> struct convert_to_gray8_result gray8_result;
> struct convert_to_rgb332_result rgb332_result;
> struct convert_to_rgb565_result rgb565_result;
> + struct convert_to_xrgb1555_result xrgb1555_result;
> + struct convert_to_argb1555_result argb1555_result;
> + struct convert_to_rgba5551_result rgba5551_result;
> struct convert_to_rgb888_result rgb888_result;
> struct convert_to_argb8888_result argb8888_result;
> struct convert_to_xrgb2101010_result xrgb2101010_result;
> @@ -85,6 +103,18 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> .expected = { 0xF800 },
> .expected_swab = { 0x00F8 },
> },
> + .xrgb1555_result = {
> + .dst_pitch = 0,
> + .expected = { 0x7C00 },
> + },
> + .argb1555_result = {
> + .dst_pitch = 0,
> + .expected = { 0xFC00 },
> + },
> + .rgba5551_result = {
> + .dst_pitch = 0,
> + .expected = { 0xF801 },
> + },
> .rgb888_result = {
> .dst_pitch = 0,
> .expected = { 0x00, 0x00, 0xFF },
> @@ -123,6 +153,18 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> .expected = { 0xF800 },
> .expected_swab = { 0x00F8 },
> },
> + .xrgb1555_result = {
> + .dst_pitch = 0,
> + .expected = { 0x7C00 },
> + },
> + .argb1555_result = {
> + .dst_pitch = 0,
> + .expected = { 0xFC00 },
> + },
> + .rgba5551_result = {
> + .dst_pitch = 0,
> + .expected = { 0xF801 },
> + },
> .rgb888_result = {
> .dst_pitch = 0,
> .expected = { 0x00, 0x00, 0xFF },
> @@ -188,6 +230,33 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> 0xE0FF, 0xFF07,
> },
> },
> + .xrgb1555_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0x7FFF, 0x0000,
> + 0x7C00, 0x03E0,
> + 0x001F, 0x7C1F,
> + 0x7FE0, 0x03FF,
> + },
> + },
> + .argb1555_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0xFFFF, 0x8000,
> + 0xFC00, 0x83E0,
> + 0x801F, 0xFC1F,
> + 0xFFE0, 0x83FF,
> + },
> + },
> + .rgba5551_result = {
> + .dst_pitch = 0,
> + .expected = {
> + 0xFFFF, 0x0001,
> + 0xF801, 0x07C1,
> + 0x003F, 0xF83F,
> + 0xFFC1, 0x07FF,
> + },
> + },
> .rgb888_result = {
> .dst_pitch = 0,
> .expected = {
> @@ -264,6 +333,30 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
> 0x00A8, 0x8E6B, 0x330A, 0x0000, 0x0000,
> },
> },
> + .xrgb1555_result = {
> + .dst_pitch = 10,
> + .expected = {
> + 0x0513, 0x0920, 0x5400, 0x0000, 0x0000,
> + 0x35CE, 0x0513, 0x0920, 0x0000, 0x0000,
> + 0x5400, 0x35CE, 0x0513, 0x0000, 0x0000,
> + },
> + },
> + .argb1555_result = {
> + .dst_pitch = 10,
> + .expected = {
> + 0x8513, 0x8920, 0xD400, 0x0000, 0x0000,
> + 0xB5CE, 0x8513, 0x8920, 0x0000, 0x0000,
> + 0xD400, 0xB5CE, 0x8513, 0x0000, 0x0000,
> + },
> + },
> + .rgba5551_result = {
> + .dst_pitch = 10,
> + .expected = {
> + 0x0A27, 0x1241, 0xA801, 0x0000, 0x0000,
> + 0x6B9D, 0x0A27, 0x1241, 0x0000, 0x0000,
> + 0xA801, 0x6B9D, 0x0A27, 0x0000, 0x0000,
> + },
> + },
> .rgb888_result = {
> .dst_pitch = 15,
> .expected = {
> @@ -443,6 +536,96 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
> KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected_swab, dst_size), 0);
> }
>
> +static void drm_test_fb_xrgb8888_to_xrgb1555(struct kunit *test)
> +{
> + const struct convert_xrgb8888_case *params = test->param_value;
> + const struct convert_to_xrgb1555_result *result = ¶ms->xrgb1555_result;
> + size_t dst_size;
> + __u16 *buf = NULL;
> + __u32 *xrgb8888 = NULL;
> + struct iosys_map dst, src;
> +
> + struct drm_framebuffer fb = {
> + .format = drm_format_info(DRM_FORMAT_XRGB8888),
> + .pitches = { params->pitch, 0, 0 },
> + };
> +
> + dst_size = conversion_buf_size(DRM_FORMAT_XRGB1555, result->dst_pitch,
> + ¶ms->clip);
> + KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> + buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> + iosys_map_set_vaddr(&dst, buf);
> +
> + xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
> + iosys_map_set_vaddr(&src, xrgb8888);
> +
> + drm_fb_xrgb8888_to_xrgb1555(&dst, &result->dst_pitch, &src, &fb, ¶ms->clip);
> + KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
> +}
> +
> +static void drm_test_fb_xrgb8888_to_argb1555(struct kunit *test)
> +{
> + const struct convert_xrgb8888_case *params = test->param_value;
> + const struct convert_to_argb1555_result *result = ¶ms->argb1555_result;
> + size_t dst_size;
> + __u16 *buf = NULL;
> + __u32 *xrgb8888 = NULL;
> + struct iosys_map dst, src;
> +
> + struct drm_framebuffer fb = {
> + .format = drm_format_info(DRM_FORMAT_XRGB8888),
> + .pitches = { params->pitch, 0, 0 },
> + };
> +
> + dst_size = conversion_buf_size(DRM_FORMAT_ARGB1555, result->dst_pitch,
> + ¶ms->clip);
> + KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> + buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> + iosys_map_set_vaddr(&dst, buf);
> +
> + xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
> + iosys_map_set_vaddr(&src, xrgb8888);
> +
> + drm_fb_xrgb8888_to_argb1555(&dst, &result->dst_pitch, &src, &fb, ¶ms->clip);
> + KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
> +}
> +
> +static void drm_test_fb_xrgb8888_to_rgba5551(struct kunit *test)
> +{
> + const struct convert_xrgb8888_case *params = test->param_value;
> + const struct convert_to_rgba5551_result *result = ¶ms->rgba5551_result;
> + size_t dst_size;
> + __u16 *buf = NULL;
> + __u32 *xrgb8888 = NULL;
> + struct iosys_map dst, src;
> +
> + struct drm_framebuffer fb = {
> + .format = drm_format_info(DRM_FORMAT_XRGB8888),
> + .pitches = { params->pitch, 0, 0 },
> + };
> +
> + dst_size = conversion_buf_size(DRM_FORMAT_RGBA5551, result->dst_pitch,
> + ¶ms->clip);
> + KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> + buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> + iosys_map_set_vaddr(&dst, buf);
> +
> + xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
> + iosys_map_set_vaddr(&src, xrgb8888);
> +
> + drm_fb_xrgb8888_to_rgba5551(&dst, &result->dst_pitch, &src, &fb, ¶ms->clip);
> + KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
> +}
> +
> static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
> {
> const struct convert_xrgb8888_case *params = test->param_value;
> @@ -570,6 +753,9 @@ static struct kunit_case drm_format_helper_test_cases[] = {
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_gray8, convert_xrgb8888_gen_params),
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb332, convert_xrgb8888_gen_params),
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb565, convert_xrgb8888_gen_params),
> + KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb1555, convert_xrgb8888_gen_params),
> + KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb1555, convert_xrgb8888_gen_params),
> + KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgba5551, convert_xrgb8888_gen_params),
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
> KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 10b2d19cdb66..2d04f5863722 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -30,6 +30,15 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
> void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
> const struct iosys_map *src, const struct drm_framebuffer *fb,
> const struct drm_rect *clip, bool swab);
> +void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
> + const struct iosys_map *src, const struct drm_framebuffer *fb,
> + const struct drm_rect *clip);
> void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
> const struct iosys_map *src, const struct drm_framebuffer *fb,
> const struct drm_rect *clip);
> --
> 2.38.1
>
