Message-ID: <0236d6a0-a8c5-47bb-a3f2-c4022b49b09d@suse.de>
Date: Tue, 9 Jul 2024 09:14:47 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
 "mripard@...nel.org" <mripard@...nel.org>,
 "airlied@...il.com" <airlied@...il.com>, "daniel@...ll.ch"
 <daniel@...ll.ch>, Orlando Chamberlain <orlandoch.dev@...il.com>,
 Kerem Karabay <kekrby@...il.com>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/format-helper: Add conversion from XRGB8888 to BGR888

Hi

Am 08.07.24 um 10:37 schrieb Aditya Garg:
>
>> On 8 Jul 2024, at 1:59 PM, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>>
>> Hi
>>
>>> Am 08.07.24 um 10:10 schrieb Aditya Garg:
>>> Hi
>>>> I see. Best would be to submit this patch together with the driver for review.
>>>>
>>> Although it’s your take, but I really doubt whether the driver would make it upstream. It shall most likely be provided as a DKMS module.
>> What's wrong with upstreaming the driver?
> 1. The author of the driver seems to have abandoned the driver.
> 2. It currently supports only T2 Macs. T1 Mac support is uncertain.
> 3. It only handles the DRM part. 2 additional HID drivers are also needed for the touch bar to work, which again work only for T2 Macs.
> 4. Although I can legally submit it since the author signed the commit and it's GPL2, I would prefer getting consent from the author for a bigger patch like adding the driver.
>
> Do you still think it will be able to make upstream?

It's up to you. But you'd effectively sign up for maintainership.

Best regards
Thomas

>
>>> What should be the best case scenario then?
>> We only add helpers that have callers. So the conversion routine would have to remain in the driver module. If we have have an upstream user of that helper, we can pick up your patch later.
>>
> Makes sense. Thanks for clarifying!
>
>> Best regards
>> Thomas
>>
>>
>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>> Signed-off-by: Kerem Karabay <kekrby@...il.com>
>>>>>>> Signed-off-by: Aditya Garg <gargaditya08@...e.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/drm_format_helper.c           | 54 +++++++++++++
>>>>>>>   .../gpu/drm/tests/drm_format_helper_test.c    | 81 +++++++++++++++++++
>>>>>>>   include/drm/drm_format_helper.h               |  3 +
>>>>>>>   3 files changed, 138 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>>>>>> index b1be458ed..28c0e76a1 100644
>>>>>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>>>>>> @@ -702,6 +702,57 @@ void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pi
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
>>>>>>>   +static void drm_fb_xrgb8888_to_bgr888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>>>>>>> +{
>>>>>>> + u8 *dbuf8 = dbuf;
>>>>>>> + const __le32 *sbuf32 = sbuf;
>>>>>>> + unsigned int x;
>>>>>>> + u32 pix;
>>>>>>> +
>>>>>>> + for (x = 0; x < pixels; x++) {
>>>>>>> + pix = le32_to_cpu(sbuf32[x]);
>>>>>>> + /* write red-green-blue to output in little endianness */
>>>>>>> + *dbuf8++ = (pix & 0x00FF0000) >> 16;
>>>>>>> + *dbuf8++ = (pix & 0x0000FF00) >> 8;
>>>>>>> + *dbuf8++ = (pix & 0x000000FF) >> 0;
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_fb_xrgb8888_to_bgr888 - Convert XRGB8888 to BGR888 clip buffer
>>>>>>> + * @dst: Array of BGR888 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 buffers
>>>>>>> + * @fb: DRM framebuffer
>>>>>>> + * @clip: Clip rectangle area to copy
>>>>>>> + * @state: Transform and conversion state
>>>>>>> + *
>>>>>>> + * This function copies parts of a framebuffer to display memory and converts the
>>>>>>> + * color format during the process. Destination and framebuffer formats must match. 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 BGR888 devices that don't natively
>>>>>>> + * support XRGB8888.
>>>>>>> + */
>>>>>>> +void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
>>>>>>> +        const struct iosys_map *src, const struct drm_framebuffer *fb,
>>>>>>> +        const struct drm_rect *clip, struct drm_format_conv_state *state)
>>>>>>> +{
>>>>>>> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
>>>>>>> + 3,
>>>>>>> + };
>>>>>>> +
>>>>>>> + drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
>>>>>>> +     drm_fb_xrgb8888_to_bgr888_line);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_bgr888);
>>>>>>> +
>>>>>>>   static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>>>>>>>   {
>>>>>>>    __le32 *dbuf32 = dbuf;
>>>>>>> @@ -1035,6 +1086,9 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>>>>>>>    } else if (dst_format == DRM_FORMAT_RGB888) {
>>>>>>>    drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip, state);
>>>>>>>    return 0;
>>>>>>> + } else if (dst_format == DRM_FORMAT_BGR888) {
>>>>>>> + drm_fb_xrgb8888_to_bgr888(dst, dst_pitch, src, fb, clip, state);
>>>>>>> + return 0;
>>>>>>>    } else if (dst_format == DRM_FORMAT_ARGB8888) {
>>>>>>>    drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip, state);
>>>>>>>    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 08992636e..e54f0f6e7 100644
>>>>>>> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
>>>>>>> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
>>>>>>> @@ -60,6 +60,11 @@ struct convert_to_rgb888_result {
>>>>>>>    const u8 expected[TEST_BUF_SIZE];
>>>>>>>   };
>>>>>>>   +struct convert_to_bgr888_result {
>>>>>>> + unsigned int dst_pitch;
>>>>>>> + const u8 expected[TEST_BUF_SIZE];
>>>>>>> +};
>>>>>>> +
>>>>>>>   struct convert_to_argb8888_result {
>>>>>>>    unsigned int dst_pitch;
>>>>>>>    const u32 expected[TEST_BUF_SIZE];
>>>>>>> @@ -107,6 +112,7 @@ struct convert_xrgb8888_case {
>>>>>>>    struct convert_to_argb1555_result argb1555_result;
>>>>>>>    struct convert_to_rgba5551_result rgba5551_result;
>>>>>>>    struct convert_to_rgb888_result rgb888_result;
>>>>>>> + struct convert_to_bgr888_result bgr888_result;
>>>>>>>    struct convert_to_argb8888_result argb8888_result;
>>>>>>>    struct convert_to_xrgb2101010_result xrgb2101010_result;
>>>>>>>    struct convert_to_argb2101010_result argb2101010_result;
>>>>>>> @@ -151,6 +157,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>>>>>>>    .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>>    .expected = { 0x00, 0x00, 0xFF },
>>>>>>>    },
>>>>>>> + .bgr888_result = {
>>>>>>> + .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>> + .expected = { 0xFF, 0x00, 0x00 },
>>>>>>> + },
>>>>>>>    .argb8888_result = {
>>>>>>>    .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>>    .expected = { 0xFFFF0000 },
>>>>>>> @@ -217,6 +227,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>>>>>>>    .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>>    .expected = { 0x00, 0x00, 0xFF },
>>>>>>>    },
>>>>>>> + .bgr888_result = {
>>>>>>> + .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>> + .expected = { 0xFF, 0x00, 0x00 },
>>>>>>> + },
>>>>>>>    .argb8888_result = {
>>>>>>>    .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>>    .expected = { 0xFFFF0000 },
>>>>>>> @@ -330,6 +344,15 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>>>>>>>    0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x00,
>>>>>>>    },
>>>>>>>    },
>>>>>>> + .bgr888_result = {
>>>>>>> + .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>> + .expected = {
>>>>>>> + 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00,
>>>>>>> + 0xFF, 0x00, 0x00, 0x00, 0xFF, 0x00,
>>>>>>> + 0x00, 0x00, 0xFF, 0xFF, 0x00, 0xFF,
>>>>>>> + 0xFF, 0xFF, 0x00, 0x00, 0xFF, 0xFF,
>>>>>>> + },
>>>>>>> + },
>>>>>>>    .argb8888_result = {
>>>>>>>    .dst_pitch = TEST_USE_DEFAULT_PITCH,
>>>>>>>    .expected = {
>>>>>>> @@ -468,6 +491,17 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
>>>>>>>    0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>>>>>>>    },
>>>>>>>    },
>>>>>>> + .bgr888_result = {
>>>>>>> + .dst_pitch = 15,
>>>>>>> + .expected = {
>>>>>>> + 0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05, 0xA8, 0xF3, 0x03,
>>>>>>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>>>>>>> + 0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C, 0x11, 0x4D, 0x05,
>>>>>>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>>>>>>> + 0xA8, 0x03, 0x03, 0x6C, 0xF0, 0x73, 0x0E, 0x44, 0x9C,
>>>>>>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>>>>>>> + },
>>>>>>> + },
>>>>>>>    .argb8888_result = {
>>>>>>>    .dst_pitch = 20,
>>>>>>>    .expected = {
>>>>>>> @@ -914,6 +948,52 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
>>>>>>>    KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
>>>>>>>   }
>>>>>>>   +static void drm_test_fb_xrgb8888_to_bgr888(struct kunit *test)
>>>>>>> +{
>>>>>>> + const struct convert_xrgb8888_case *params = test->param_value;
>>>>>>> + const struct convert_to_bgr888_result *result = &params->bgr888_result;
>>>>>>> + size_t dst_size;
>>>>>>> + u8 *buf = NULL;
>>>>>>> + __le32 *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_BGR888, result->dst_pitch,
>>>>>>> +        &params->clip, 0);
>>>>>>> + 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 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
>>>>>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
>>>>>>> + iosys_map_set_vaddr(&src, xrgb8888);
>>>>>>> +
>>>>>>> + /*
>>>>>>> +  * BGR888 expected results are already in little-endian
>>>>>>> +  * order, so there's no need to convert the test output.
>>>>>>> +  */
>>>>>>> + drm_fb_xrgb8888_to_bgr888(&dst, &result->dst_pitch, &src, &fb, &params->clip,
>>>>>>> +   &fmtcnv_state);
>>>>>>> + KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
>>>>>>> +
>>>>>>> + buf = dst.vaddr; /* restore original value of buf */
>>>>>>> + memset(buf, 0, dst_size);
>>>>>>> +
>>>>>>> + int blit_result = 0;
>>>>>>> +
>>>>>>> + blit_result = drm_fb_blit(&dst, &result->dst_pitch, DRM_FORMAT_BGR888, &src, &fb, &params->clip,
>>>>>>> +   &fmtcnv_state);
>>>>>>> +
>>>>>>> + KUNIT_EXPECT_FALSE(test, blit_result);
>>>>>>> + KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
>>>>>>> +}
>>>>>>> +
>>>>>>>   static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
>>>>>>>   {
>>>>>>>    const struct convert_xrgb8888_case *params = test->param_value;
>>>>>>> @@ -1851,6 +1931,7 @@ static struct kunit_case drm_format_helper_test_cases[] = {
>>>>>>>    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_bgr888, 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),
>>>>>>>    KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb2101010, convert_xrgb8888_gen_params),
>>>>>>> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
>>>>>>> index f13b34e0b..b53cf85ca 100644
>>>>>>> --- a/include/drm/drm_format_helper.h
>>>>>>> +++ b/include/drm/drm_format_helper.h
>>>>>>> @@ -95,6 +95,9 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
>>>>>>>   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, struct drm_format_conv_state *state);
>>>>>>> +void drm_fb_xrgb8888_to_bgr888(struct iosys_map *dst, const unsigned int *dst_pitch,
>>>>>>> +        const struct iosys_map *src, const struct drm_framebuffer *fb,
>>>>>>> +        const struct drm_rect *clip, struct drm_format_conv_state *state);
>>>>>>>   void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
>>>>>>>     const struct iosys_map *src, const struct drm_framebuffer *fb,
>>>>>>>     const struct drm_rect *clip, struct drm_format_conv_state *state);
>>>>>> --
>>>>>> --
>>>>>> Thomas Zimmermann
>>>>>> Graphics Driver Developer
>>>>>> SUSE Software Solutions Germany GmbH
>>>>>> Frankenstrasse 146, 90461 Nuernberg, Germany
>>>>>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>>>>>> HRB 36809 (AG Nuernberg)
>>>> --
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Frankenstrasse 146, 90461 Nuernberg, Germany
>>>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>>>> HRB 36809 (AG Nuernberg)
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Powered by blists - more mailing lists