Re: [PATCH 5/9] drm/color-mgmt: Prepare for RGB332 palettes

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 11.07.25 um 14:30 schrieb Thomas Zimmermann:
> Hi
>
> Am 11.07.25 um 14:24 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>>
>>> Add helper drm_crtc_fill_palette_332(), which fills palettes with
>>> RGB332 color data. Each color in RGB332 format serves as an index
>>> into an 8-bit palette that stores the corresponding component-based
>>> colors.
>>>
>>> Vesadrm will use the new helper to emulate RGB formats on top of
>>> framebuffers in C8 format.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/drm_color_mgmt.c | 32 
>>> ++++++++++++++++++++++++++++++++
>>>   include/drm/drm_color_mgmt.h     |  1 +
>>>   2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c 
>>> b/drivers/gpu/drm/drm_color_mgmt.c
>>> index 37a3270bc3c2..7ef214848313 100644
>>> --- a/drivers/gpu/drm/drm_color_mgmt.c
>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c
>>> @@ -817,6 +817,38 @@ void drm_crtc_load_palette_8(struct drm_crtc 
>>> *crtc, const struct drm_color_lut *
>>>   }
>>>   EXPORT_SYMBOL(drm_crtc_load_palette_8);
>>>   +static void fill_palette_332(struct drm_crtc *crtc, u16 r, u16 g, 
>>> u16 b,
>>> +                 drm_crtc_set_lut_func set_palette)
>>> +{
>>> +    unsigned int i = (r << 5) | (g << 2) | b;
>>> +
>>> +    r = (r << 13) | (r << 10) | (r << 7) | (r << 4) | (r << 1) | (r 
>>> >> 2);
>>> +    g = (g << 13) | (g << 10) | (g << 7) | (g << 4) | (g << 1) | (g 
>>> >> 2);
>>> +    b = (b << 14) | (b << 12) | (b << 10) | (b << 8) | (b << 6) | 
>>> (b << 4) | (b << 2) | b;
>>> +
>>> +    set_palette(crtc, i, r, g, b);
>>> +}
>> I think this helper can benefit of having a kernel-doc or some code
>> comments, e.g:
>>
>>            /* Calculate the 8-bit palette index from the color 
>> components */
>>            unsigned int i = (r << 5) | (g << 2) | b;
>>
>>            /* Expand R (3-bit) G (3-bit) and B (2-bit) values to 
>> 16-bit depth colors */
>>            r = (r << 13) | (r << 10) | (r << 7) | (r << 4) | (r << 1) 
>> | (r >> 2);
>>            g = (g << 13) | (g << 10) | (g << 7) | (g << 4) | (g << 1) 
>> | (g >> 2);
>>            b = (b << 14) | (b << 12) | (b << 10) | (b << 8) | (b << 
>> 6) | (b << 4) | (b << 2) | b;
>>
>>            /* Call the drivers' specific callback to program the 
>> hardware LUT */
>>            set_palette(crtc, i, r, g, b);
>>
>> It might be evident to you, but I don't think it will be for others 
>> looking
>> at the code later.
>>
>> The code itself looks good to me, if I understood it correctly :)
>
> Thanks for reviewing. Commenting this code makes sense.  I just looked 
> again at the r-g-b for loop and the limits are incorrect. Needs to by 
> 3-3-2 instead of 8-8-4. IDK how that happened, as the screenshot from 
> the test looks correct.
>
> https://imgur.com/a/vesadrm-day4-z2XQ4wA
>
> Anyway that is another fix here.

And looking at it again, 8-8-4 is actually correct. It's just 
non-obvious even to the code's author. Hence some docs/changes are needed.

>
> Best regards
> Thomas
>
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>

-- 
--
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)