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

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

[Daniel Vetter]

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.

Motivated by comments from Geert that we have a gap here.

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Daniel Vetter <daniel.vetter@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.
+ *
  * “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;
 
-- 
2.33.0

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

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

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@linux-m68k.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Daniel Vetter <daniel.vetter@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 ?

Other than that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@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

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

[Daniel Vetter]

On Mon, Jan 24, 2022 at 9:24 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> 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@linux-m68k.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.
-Daniel

>
> Other than that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [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@linux-m68k.org>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@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@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

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

[Daniel Vetter]

On Mon, Jan 24, 2022 at 9:41 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@linux-m68k.org>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@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 ?

Sounds good, I'll just add them all in parathesis. The more the better
the chance people find this.
-Daniel

>
> > > Other than that,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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.

Motivated by comments from Geert that we have a gap here.

v2: More names for color luts (Laurent).

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Daniel Vetter <daniel.vetter@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..9079fbe21d2f 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 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;
 
-- 
2.33.0

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

[Alex Deucher]

On Mon, Jan 24, 2022 at 5:16 PM Daniel Vetter <daniel.vetter@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.
>
> Motivated by comments from Geert that we have a gap here.
>
> v2: More names for color luts (Laurent).
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.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..9079fbe21d2f 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 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;
>
> --
> 2.33.0
>

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

[Daniel Vetter]

On Mon, Jan 24, 2022 at 05:18:07PM -0500, Alex Deucher wrote:
> On Mon, Jan 24, 2022 at 5:16 PM Daniel Vetter <daniel.vetter@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.
> >
> > Motivated by comments from Geert that we have a gap here.
> >
> > v2: More names for color luts (Laurent).
> >
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks for taking a look, pushed to drm-misc-next.
-Daniel

> 
> > ---
> >  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..9079fbe21d2f 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 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;
> >
> > --
> > 2.33.0
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3856 bytes --]

Hi

Am 24.01.22 um 23:16 schrieb Daniel Vetter:
> 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

Older HW has cursor planes with separate palettes; 16 colors on Matrox 
IIRC. Not very useful for us, though.

Best regards
Thomas

> also not sure whether even hardware with a CLUT and a full color
> correction pipeline with degamm/cgm/gamma exists.
> 
> Motivated by comments from Geert that we have a gap here.
> 
> v2: More names for color luts (Laurent).
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Daniel Vetter <daniel.vetter@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..9079fbe21d2f 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 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;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [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@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@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@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

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

[Daniel Vetter]

On Tue, Jan 25, 2022 at 09:22:15AM +0100, Geert Uytterhoeven wrote:
> Hi Daniel,
> 
> Thanks for your patch!
> 
> On Mon, Jan 24, 2022 at 11:16 PM Daniel Vetter <daniel.vetter@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...

Yeah I think extending this is a all-around compatible way isn't too
tricky, just a bit of work:
- add the clut to the plane state
- add a helper which takes the plane state, and if you have an indexed
  pixel format first grabs the plane state clut, and if that's not
  present, the crtc gamma lut as fallback. Also gives you nothing if you
  don't have a indexed pixel format
- convert drivers over to that helper

This way legacy userspace which only uses a primary plane and the
gamma-as-a-clut thing will keep working, while we can expose the full
features. And drivers don't have to care about the compat either.

> > 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@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?

Yup.

> 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?

It only checks whether the blob property changed, but not the contents. So
if you see this set when nothing changes then I guess some code somewhere
is creating a new lut blob property for every screen update. If this goes
through the legacy gamma_set ioctl/vfunc interface then I think we'll do
that in there every time, and at least for fbdev emulation the fix would
be to instead use the atomic interfaces and cache the blob prop. Or
implement the lut blob prop caching in gamma_set, but that's kinda a hack
(since userspace also shouldn't call that one just for lolz).
-Daniel

> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

On Tue, Jan 25, 2022 at 09:35:54AM +0100, Daniel Vetter wrote:
> On Tue, Jan 25, 2022 at 09:22:15AM +0100, Geert Uytterhoeven wrote:
> > Hi Daniel,
> > 
> > Thanks for your patch!
> > 
> > On Mon, Jan 24, 2022 at 11:16 PM Daniel Vetter <daniel.vetter@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...
> 
> Yeah I think extending this is a all-around compatible way isn't too
> tricky, just a bit of work:
> - add the clut to the plane state
> - add a helper which takes the plane state, and if you have an indexed
>   pixel format first grabs the plane state clut, and if that's not
>   present, the crtc gamma lut as fallback. Also gives you nothing if you
>   don't have a indexed pixel format
> - convert drivers over to that helper
> 
> This way legacy userspace which only uses a primary plane and the
> gamma-as-a-clut thing will keep working, while we can expose the full
> features. And drivers don't have to care about the compat either.
> 
> > > 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@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?
> 
> Yup.

I forgot to add why: The legacy store is only filled in for the legacy
interfaces, so if someone uses the atomic properties directly, then your
driver looking at the legacy gamma store will see nothing.

We could/should perhaps make the legacy store entirely defunct for atomic
drivers, but that means a bunch of typing and auditing. We have the same
problem in other areas too due to the gradual switch towards atomic in
many drivers, so definitely an area where you can sink a lot of time into.
-Daniel


> > 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?
> 
> It only checks whether the blob property changed, but not the contents. So
> if you see this set when nothing changes then I guess some code somewhere
> is creating a new lut blob property for every screen update. If this goes
> through the legacy gamma_set ioctl/vfunc interface then I think we'll do
> that in there every time, and at least for fbdev emulation the fix would
> be to instead use the atomic interfaces and cache the blob prop. Or
> implement the lut blob prop caching in gamma_set, but that's kinda a hack
> (since userspace also shouldn't call that one just for lolz).
> -Daniel
> 
> > 
> > Thanks!
> > 
> > Gr{oetje,eeting}s,
> > 
> >                         Geert
> > 
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Tue, Jan 25, 2022 at 09:39:23AM +0100, Daniel Vetter wrote:
> On Tue, Jan 25, 2022 at 09:35:54AM +0100, Daniel Vetter wrote:
> > On Tue, Jan 25, 2022 at 09:22:15AM +0100, Geert Uytterhoeven wrote:
> > > Hi Daniel,
> > > 
> > > Thanks for your patch!
> > > 
> > > On Mon, Jan 24, 2022 at 11:16 PM Daniel Vetter <daniel.vetter@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...
> > 
> > Yeah I think extending this is a all-around compatible way isn't too
> > tricky, just a bit of work:
> > - add the clut to the plane state
> > - add a helper which takes the plane state, and if you have an indexed
> >   pixel format first grabs the plane state clut, and if that's not
> >   present, the crtc gamma lut as fallback. Also gives you nothing if you
> >   don't have a indexed pixel format
> > - convert drivers over to that helper
> > 
> > This way legacy userspace which only uses a primary plane and the
> > gamma-as-a-clut thing will keep working, while we can expose the full
> > features. And drivers don't have to care about the compat either.

https://github.com/vsyrjala/linux/commits/fb_helper_c8_lut_4

> > 
> > > > 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@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?
> > 
> > Yup.
> 
> I forgot to add why: The legacy store is only filled in for the legacy
> interfaces, so if someone uses the atomic properties directly, then your
> driver looking at the legacy gamma store will see nothing.
> 
> We could/should perhaps make the legacy store entirely defunct for atomic
> drivers, but that means a bunch of typing and auditing. We have the same
> problem in other areas too due to the gradual switch towards atomic in
> many drivers, so definitely an area where you can sink a lot of time into.

I have some wip stuff here that tried to nuke the gamma_store stuff:
https://github.com/vsyrjala/linux/commits/fb_helper_c8_lut_4

As for adding a LUT to the planes that needs careful thought as it
interacts heavily with the whole plane color management work.

Also there's hardware (everything supported by i915 for example) where
each plane might have a dedicated gamma ramp for !C8 while still relying
on the pipe LUT for C8 scanout. Multiple planes can be scanning out
C8 simultaneously and they all share the same LUT. And while that is
going on the pipe LUT can then not be used for anything else.

-- 
Ville Syrjälä
Intel

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

[Daniel Vetter]

On Tue, Jan 25, 2022 at 10:52:18AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 25, 2022 at 09:39:23AM +0100, Daniel Vetter wrote:
> > On Tue, Jan 25, 2022 at 09:35:54AM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 25, 2022 at 09:22:15AM +0100, Geert Uytterhoeven wrote:
> > > > Hi Daniel,
> > > > 
> > > > Thanks for your patch!
> > > > 
> > > > On Mon, Jan 24, 2022 at 11:16 PM Daniel Vetter <daniel.vetter@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...
> > > 
> > > Yeah I think extending this is a all-around compatible way isn't too
> > > tricky, just a bit of work:
> > > - add the clut to the plane state
> > > - add a helper which takes the plane state, and if you have an indexed
> > >   pixel format first grabs the plane state clut, and if that's not
> > >   present, the crtc gamma lut as fallback. Also gives you nothing if you
> > >   don't have a indexed pixel format
> > > - convert drivers over to that helper
> > > 
> > > This way legacy userspace which only uses a primary plane and the
> > > gamma-as-a-clut thing will keep working, while we can expose the full
> > > features. And drivers don't have to care about the compat either.
> 
> https://github.com/vsyrjala/linux/commits/fb_helper_c8_lut_4

Yeah resurrecting those might be a good idea.
-Daniel


> 
> > > 
> > > > > 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@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?
> > > 
> > > Yup.
> > 
> > I forgot to add why: The legacy store is only filled in for the legacy
> > interfaces, so if someone uses the atomic properties directly, then your
> > driver looking at the legacy gamma store will see nothing.
> > 
> > We could/should perhaps make the legacy store entirely defunct for atomic
> > drivers, but that means a bunch of typing and auditing. We have the same
> > problem in other areas too due to the gradual switch towards atomic in
> > many drivers, so definitely an area where you can sink a lot of time into.
> 
> I have some wip stuff here that tried to nuke the gamma_store stuff:
> https://github.com/vsyrjala/linux/commits/fb_helper_c8_lut_4
> 
> As for adding a LUT to the planes that needs careful thought as it
> interacts heavily with the whole plane color management work.
> 
> Also there's hardware (everything supported by i915 for example) where
> each plane might have a dedicated gamma ramp for !C8 while still relying
> on the pipe LUT for C8 scanout. Multiple planes can be scanning out
> C8 simultaneously and they all share the same LUT. And while that is
> going on the pipe LUT can then not be used for anything else.
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch