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

[Thomas Zimmermann <tzimmermann@suse.de>]

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.

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)