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@xxxxxxxxxxxxxx> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > > Cc: Maxime Ripard <mripard@xxxxxxxxxx> > > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> > > > Cc: David Airlie <airlied@xxxxxxxx> > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxxx> > > > > > + * > > > * “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