[PATCH] drm/docs: Document where the C8 color lut is stored

[Geert Uytterhoeven]

Hi Daniel,

Thanks for your patch!

On Mon, Jan 24, 2022 at 11:16 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Also add notes that for atomic drivers it's really somewhere else and
> no longer in struct drm_crtc.
>
> Maybe we should put a bigger warning here that this is confusing,
> since the pixel format is a plane property, but the GAMMA_LUT property
> is on the crtc. But I think we can fix this if/when someone finds a
> need for a per-plane CLUT, since I'm not sure such hw even exists. I'm
> also not sure whether even hardware with a CLUT and a full color
> correction pipeline with degamm/cgm/gamma exists.

IIRC (it's been a looong time) some set-top-box hardware did support
this.  It made sense, as the CLUT is per-plane, while the gamma value
is a property of the display output device.
At that time, desktop hardware supported only a single plane, so
hardware complexity could be reduced by letting software handle
that through a single clut (for "pseudocolor") or gamma table (for
"directcolor").
For hardware with multiple alpha-blended planes (some CLUT, some
ARGB, some (A)YCbCr), doing it in software is either very complicated
or impossible, especially if you have two heads needing different
gamma values.
Whether such hardware still exists, and needs to be supported,
I do not know...

> Motivated by comments from Geert that we have a gap here.
>
> v2: More names for color luts (Laurent).

+1, that would help for sure!

> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -82,6 +82,10 @@
>   *     driver boot-up state too. Drivers can access this blob through
>   *     &drm_crtc_state.gamma_lut.
>   *
> + *     Note that for mostly historical reasons stemming from Xorg heritage,
> + *     this is also used to store the color map (also sometimes color lut, CLUT
> + *     or color palette) for indexed formats like DRM_FORMAT_C8.
> + *
>   * “GAMMA_LUT_SIZE”:
>   *     Unsigned range property to give the size of the lookup table to be set
>   *     on the GAMMA_LUT property (the size depends on the underlying hardware).
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 4d01b4d89775..a70baea0636c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -285,6 +285,10 @@ struct drm_crtc_state {
>          * Lookup table for converting pixel data after the color conversion
>          * matrix @ctm.  See drm_crtc_enable_color_mgmt(). The blob (if not
>          * NULL) is an array of &struct drm_color_lut.
> +        *
> +        * Note that for mostly historical reasons stemming from Xorg heritage,
> +        * this is also used to store the color map (also sometimes color lut,
> +        * CLUT or color palette) for indexed formats like DRM_FORMAT_C8.
>          */
>         struct drm_property_blob *gamma_lut;
>
> @@ -1075,12 +1079,18 @@ struct drm_crtc {
>         /**
>          * @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
>          * by calling drm_mode_crtc_set_gamma_size().
> +        *
> +        * Note that atomic drivers need to instead use
> +        * &drm_crtc_state.gamma_lut. See drm_crtc_enable_color_mgmt().
>          */
>         uint32_t gamma_size;
>
>         /**
>          * @gamma_store: Gamma ramp values used by the legacy SETGAMMA and
>          * GETGAMMA IOCTls. Set up by calling drm_mode_crtc_set_gamma_size().
> +        *
> +        * Note that atomic drivers need to instead use
> +        * &drm_crtc_state.gamma_lut. See drm_crtc_enable_color_mgmt().
>          */
>         uint16_t *gamma_store;

This is indeed what I ended up using, as
drivers/gpu/drm/drm_fb_helper.c:setcmap_atomic() fills in
drm_crtc.gamma_store[].  But as I understand it, I should instead
use the gamma_lut above?

BTW, to check if the CLUT changed, I look at
drm_crtc_state.color_mgmt_changed.  This works reasonably well,
but I still see more CLUT reloads than expected.
Who clears drm_crtc_state.color_mgmt_changed again?
Is there a better check?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds