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

[Laurent Pinchart]

Hi Daniel,

On Mon, Jan 24, 2022 at 09:28:09PM +0100, Daniel Vetter wrote:
> On Mon, Jan 24, 2022 at 9:24 PM Laurent Pinchart wrote:
> > On Mon, Jan 24, 2022 at 08:47:06PM +0100, Daniel Vetter 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.
> >
> > Exists, maybe, exists and has a real use case, I'd be surprised.
> >
> > > Motivated by comments from Geert that we have a gap here.
> > >
> > > Cc: Geert Uytterhoeven <geert at linux-m68k.org>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Cc: Maxime Ripard <mripard at kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > > Cc: David Airlie <airlied at linux.ie>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c |  4 ++++
> > >  include/drm/drm_crtc.h           | 10 ++++++++++
> > >  2 files changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index bb14f488c8f6..96ce57ad37e6 100644
> > > --- 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 lookup table (CLUT) for indexed
> > > + *   formats like DRM_FORMAT_C8.
> >
> > CLUT also stands for Cubic Look Up Table, a type of LUT commonly used
> > for tone mapping that maps an RGB sample (in 3D space) to a colour.
> > Compared to traditional LUTs such as gamma and degamma, it allows
> > correlating colour components, while the gamma and degamma LUTs operate
> > on each colour component independently.
> >
> > Is there any commonly used acronym for the indexed colours lookup table
> > that we could use here, to avoid future confusions ?
> 
> Hm intel calls these 3DLUT, so for me there's not confusion here :-)
> But also since random acronyms are bad I put both the acronym and the
> full spelling into the text.
> 
> The cubic lut for me sounds more like cubic filtering from texture
> units (yet another thing). Do you want me to just drop the CLUT (I
> figured some people might search for that in the docs, and there's
> really no other way find this) or ok this way? I don't really have a
> better idea.

fbdev uses "color map", "color palette" seems to also be a common term.
Maybe use one of those two ?

> > Other than that,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > + *
> > >   * “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..03cc53220a2a 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 lookup table (CLUT) 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;
> > >

-- 
Regards,

Laurent Pinchart