Re: [PATCH 1/5] drm: Add helpers for programming hardware gamma LUTs

[Jocelyn Falempe <jfalempe@redhat.com>]

On 09/05/2025 10:23, Thomas Zimmermann wrote:
> Provide helpers that program hardware gamma LUTs. Tha gamma ramp is
> either provided by the driver or generated by the helper.
> 
> The DRM driver exports the GAMMA_LUT property with a fixed number of
> entries per color component, such as 256 on 8-bit-wide components. The
> entries describe the gamma ramp of each individual component. The new
> helper drm_crtc_load_gamma_888() loads such gamma ramp to hardware. The
> hardware uses each displayed pixel's individial components as indeces
> into the hardware gamma table.
> 
> For color modes with less than 8 bits per color component, the helpers
> drm_crtc_load_gamma_565_from() and drm_crtc_load_gamma_555_from_888()
> interpolate the provided gamma ramp to reduce it to the correct number
> of entries; 5/6/5 for RGB565-like formats and 5/5/5 for RGB1555-like
> formats.
> 
> If no gamma ramp has been provided, drivers can use the new helper
> drm_crtc_fill_gamma_888() to load a default gamma ramp with 256 entries
> per color component. For color modes with less bits, the new helpers
> drm_crtc_fill_gamma_565() and drm_crtc_fill_gamma_555() are available.
> The default gamma ramp uses a gamma factor of 1. Later patches can
> change this. For PCs, a gamma factor of 2.2 is common.
> 
> For color modes with palette, drm_crtc_load_palette_8() load an 8-bit
> palette into the hardware. If no palette has been specified,
> drm_crtc_fill_palette_8() load a system-specific default palette. This
> is currently only a grey-scale palette with increasing luminance, but
> later patches can change this. For PCs, a VGA default palette could
> be used.

Thanks for this work. I've one comment below regarding the 
drm_crtc_set_lut_func prototype.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/drm_color_mgmt.c | 206 +++++++++++++++++++++++++++++++
>   include/drm/drm_color_mgmt.h     |  27 ++++
>   2 files changed, 233 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3969dc548cff..dd3e06605180 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -630,3 +630,209 @@ int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests)
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_color_lut_check);
> +
> +/*
> + * Gamma-LUT programming
> + */
> +
> +/**
> + * drm_crtc_load_gamma_888 - Programs gamma ramp for RGB888-like formats
> + * @crtc: The displaying CRTC
> + * @lut: The gamma ramp to program
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs the gamma ramp specified in @lut to hardware. The input gamma
> + * ramp must have 256 entries per color component.
> + */
> +void drm_crtc_load_gamma_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +			     drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 256; ++i)
> +		set_gamma(crtc, i, lut[i].red, lut[i].green, lut[i].blue);
> +}
> +EXPORT_SYMBOL(drm_crtc_load_gamma_888);
> +
> +/**
> + * drm_crtc_load_gamma_565_from_888 - Programs gamma ramp for RGB565-like formats
> + * @crtc: The displaying CRTC
> + * @lut: The gamma ramp to program
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs the gamma ramp specified in @lut to hardware. The input gamma
> + * ramp must have 256 entries per color component. The helper interpolates
> + * the individual color components to reduce the number of entries to 5/6/5.
> + */
> +void drm_crtc_load_gamma_565_from_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +				      drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +	u16 r, g, b;
> +
> +	for (i = 0; i < 32; ++i) {
> +		r = lut[i * 8 + i / 4].red;
> +		g = lut[i * 4 + i / 16].green;
> +		b = lut[i * 8 + i / 4].blue;
> +		set_gamma(crtc, i, r, g, b);
> +	}
> +	/* Green has one more bit, so add padding with 0 for red and blue. */
> +	for (i = 32; i < 64; ++i) {
> +		g = lut[i * 4 + i / 16].green;
> +		set_gamma(crtc, i, 0, g, 0);
> +	}
> +}
> +EXPORT_SYMBOL(drm_crtc_load_gamma_565_from_888);
> +
> +/**
> + * drm_crtc_load_gamma_555_from_888 - Programs gamma ramp for RGB555-like formats
> + * @crtc: The displaying CRTC
> + * @lut: The gamma ramp to program
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs the gamma ramp specified in @lut to hardware. The input gamma
> + * ramp must have 256 entries per color component. The helper interpolates
> + * the individual color components to reduce the number of entries to 5/5/5.
> + */
> +void drm_crtc_load_gamma_555_from_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +				      drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +	u16 r, g, b;
> +
> +	for (i = 0; i < 32; ++i) {
> +		r = lut[i * 8 + i / 4].red;
> +		g = lut[i * 8 + i / 4].green;
> +		b = lut[i * 8 + i / 4].blue;
> +		set_gamma(crtc, i, r, g, b);
> +	}
> +}
> +EXPORT_SYMBOL(drm_crtc_load_gamma_555_from_888);
> +
> +static void fill_gamma_888(struct drm_crtc *crtc, unsigned int i, u16 r, u16 g, u16 b,
> +			   drm_crtc_set_lut_func set_gamma)
> +{
> +	r = (r << 8) | r;
> +	g = (g << 8) | g;
> +	b = (b << 8) | b;
> +
> +	set_gamma(crtc, i, r, g, b);
> +}
> +
> +/**
> + * drm_crtc_fill_gamma_888 - Programs a default gamma ramp for RGB888-like formats
> + * @crtc: The displaying CRTC
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs a default gamma ramp to hardware.
> + */
> +void drm_crtc_fill_gamma_888(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 256; ++i)
> +		fill_gamma_888(crtc, i, i, i, i, set_gamma);
> +}
> +EXPORT_SYMBOL(drm_crtc_fill_gamma_888);
> +
> +static void fill_gamma_565(struct drm_crtc *crtc, unsigned int i, u16 r, u16 g, u16 b,
> +			   drm_crtc_set_lut_func set_gamma)
> +{
> +	r = (r << 11) | (r << 6) | (r << 1) | (r >> 4);
> +	g = (g << 10) | (g << 4) | (g >> 2);
> +	b = (b << 11) | (b << 6) | (b << 1) | (b >> 4);
> +
> +	set_gamma(crtc, i, r, g, b);
> +}
> +
> +/**
> + * drm_crtc_fill_gamma_565 - Programs a default gamma ramp for RGB565-like formats
> + * @crtc: The displaying CRTC
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs a default gamma ramp to hardware.
> + */
> +void drm_crtc_fill_gamma_565(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 32; ++i)
> +		fill_gamma_565(crtc, i, i, i, i, set_gamma);
> +	/* Green has one more bit, so add padding with 0 for red and blue. */
> +	for (i = 32; i < 64; ++i)
> +		fill_gamma_565(crtc, i, 0, i, 0, set_gamma);
> +}
> +EXPORT_SYMBOL(drm_crtc_fill_gamma_565);
> +
> +static void fill_gamma_555(struct drm_crtc *crtc, unsigned int i, u16 r, u16 g, u16 b,
> +			   drm_crtc_set_lut_func set_gamma)
> +{
> +	r = (r << 11) | (r << 6) | (r << 1) | (r >> 4);
> +	g = (g << 11) | (g << 6) | (g << 1) | (g >> 4);
> +	b = (b << 11) | (b << 6) | (b << 1) | (r >> 4);
> +
> +	set_gamma(crtc, i, r, g, b);
> +}
> +
> +/**
> + * drm_crtc_fill_gamma_555 - Programs a default gamma ramp for RGB555-like formats
> + * @crtc: The displaying CRTC
> + * @set_gamma: Callback for programming the hardware gamma LUT
> + *
> + * Programs a default gamma ramp to hardware.
> + */
> +void drm_crtc_fill_gamma_555(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 32; ++i)
> +		fill_gamma_555(crtc, i, i, i, i, set_gamma);
> +}
> +EXPORT_SYMBOL(drm_crtc_fill_gamma_555);
> +
> +/*
> + * Color-LUT programming
> + */
> +
> +/**
> + * drm_crtc_load_palette_8 - Programs palette for C8-like formats
> + * @crtc: The displaying CRTC
> + * @lut: The palette to program
> + * @set_palette: Callback for programming the hardware palette
> + *
> + * Programs the palette specified in @lut to hardware. The input palette
> + * must have 256 entries per color component.
> + */
> +void drm_crtc_load_palette_8(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +			     drm_crtc_set_lut_func set_palette)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 256; ++i)
> +		set_palette(crtc, i, lut[i].red, lut[i].green, lut[i].blue);
> +}
> +EXPORT_SYMBOL(drm_crtc_load_palette_8);
> +
> +static void fill_palette_8(struct drm_crtc *crtc, unsigned int i,
> +			   drm_crtc_set_lut_func set_palette)
> +{
> +	u16 Y = (i << 8) | i; // relative luminance
> +
> +	set_palette(crtc, i, Y, Y, Y);
> +}
> +
> +/**
> + * drm_crtc_fill_palette_8 - Programs a default palette for C8-like formats
> + * @crtc: The displaying CRTC
> + * @set_palette: Callback for programming the hardware gamma LUT
> + *
> + * Programs a default palette to hardware.
> + */
> +void drm_crtc_fill_palette_8(struct drm_crtc *crtc, drm_crtc_set_lut_func set_palette)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 256; ++i)
> +		fill_palette_8(crtc, i, set_palette);
> +}
> +EXPORT_SYMBOL(drm_crtc_fill_palette_8);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index ed81741036d7..6cb577f6dba6 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -118,4 +118,31 @@ enum drm_color_lut_tests {
>   };
>   
>   int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> +
> +/*
> + * Gamma-LUT programming
> + */
> +
> +typedef void (*drm_crtc_set_lut_func)(struct drm_crtc *, unsigned int, u16, u16, u16);

All drivers using this code, have 256 LUT size, and use 8bits value.
So I would prefer to have it declared like this:
typedef void (*drm_crtc_set_lut_func)(struct drm_crtc *, u8, u8, u8, u8);

This will avoid to upcast from u8 to u16, and then from u16 to u8 in 
each driver.

> +
> +void drm_crtc_load_gamma_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +			     drm_crtc_set_lut_func set_gamma);
> +void drm_crtc_load_gamma_565_from_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +				      drm_crtc_set_lut_func set_gamma);
> +void drm_crtc_load_gamma_555_from_888(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +				      drm_crtc_set_lut_func set_gamma);
> +
> +void drm_crtc_fill_gamma_888(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma);
> +void drm_crtc_fill_gamma_565(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma);
> +void drm_crtc_fill_gamma_555(struct drm_crtc *crtc, drm_crtc_set_lut_func set_gamma);
> +
> +/*
> + * Color-LUT programming
> + */
> +
> +void drm_crtc_load_palette_8(struct drm_crtc *crtc, const struct drm_color_lut *lut,
> +			     drm_crtc_set_lut_func set_palette);
> +
> +void drm_crtc_fill_palette_8(struct drm_crtc *crtc, drm_crtc_set_lut_func set_palette);
> +
>   #endif


Best regards,

-- 

Jocelyn