| Message ID | 20220124194706.930319-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/docs: Document where the C8 color lut is stored | expand |
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; >
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
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; > > >
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
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;
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(+)