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

[Javier Martinez Canillas <javierm@redhat.com>]

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

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat