Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 08, 2020 at 05:55:11PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Dec 08, 2020 at 03:57:58PM +0200, Tomi Valkeinen wrote:
> > We currently have drm_atomic_helper_legacy_gamma_set() helper which can
> > be used to handle legacy gamma-set ioctl.
> > drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
> > CTM and DEGAMMA_LUT. This works fine on HW where we have either:
> > 
> > degamma -> ctm -> gamma -> out
> > 
> > or
> > 
> > ctm -> gamma -> out
> > 
> > However, if the HW has gamma table before ctm, the atomic property
> > should be DEGAMMA_LUT, and thus we have:
> > 
> > degamma -> ctm -> out
> > 
> > This is fine for userspace which sets gamma table using the properties,
> > as the userspace can check for the existence of gamma & degamma, but the
> > legacy gamma-set ioctl does not work.
> > 
> > This patch fixes the issue by changing
> > drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if
> > it exists, and DEGAMMA_LUT will be used as a fallback.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++---
> >  drivers/gpu/drm/drm_color_mgmt.c    |  4 ++++
> >  drivers/gpu/drm/drm_fb_helper.c     |  8 ++++++--
> >  include/drm/drm_crtc.h              |  3 +++
> >  4 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ba1507036f26..117b186fe646 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
> >   * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> >   * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> >   * how the atomic color management and gamma tables work.
> > + *
> > + * This function uses the GAMMA_LUT or DEGAMMA_LUT property for the gamma table.
> > + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as
> > + * a fallback.
> >   */
> >  int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  				       u16 *red, u16 *green, u16 *blue,
> > @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  	int i, ret = 0;
> >  	bool replaced;
> >  
> > +	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> > +		return -ENODEV;
> > +
> >  	state = drm_atomic_state_alloc(crtc->dev);
> >  	if (!state)
> >  		return -ENOMEM;
> > @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  		goto fail;
> >  	}
> >  
> > -	/* Reset DEGAMMA_LUT and CTM properties. */
> > -	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> > +	/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> > +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > +					      crtc->has_gamma_prop ? NULL : blob);
> >  	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > -	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
> > +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > +					      crtc->has_gamma_prop ? blob : NULL);
> >  	crtc_state->color_mgmt_changed |= replaced;
> >  
> >  	ret = drm_atomic_commit(state);
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 3bcabc2f6e0e..956e59d5f6a7 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  					   degamma_lut_size);
> >  	}
> >  
> > +	crtc->has_degamma_prop = !!degamma_lut_size;
> > +
> >  	if (has_ctm)
> >  		drm_object_attach_property(&crtc->base,
> >  					   config->ctm_property, 0);
> > @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  					   config->gamma_lut_size_property,
> >  					   gamma_lut_size);
> >  	}
> > +
> > +	crtc->has_gamma_prop = !!gamma_lut_size;
> >  }
> >  EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
> >  
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 25edf670867c..b0906ef97617 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  	drm_client_for_each_modeset(modeset, &fb_helper->client) {
> >  		crtc = modeset->crtc;
> >  
> > +		if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> > +			continue;
> > +
> >  		if (!gamma_lut)
> >  			gamma_lut = setcmap_new_gamma_lut(crtc, cmap);
> >  		if (IS_ERR(gamma_lut)) {
> > @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  			goto out_state;
> >  		}
> >  
> > +		/* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */
> >  		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > -						      NULL);
> > +						      crtc->has_gamma_prop ? NULL : gamma_lut);
> >  		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> >  		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > -						      gamma_lut);
> > +						      crtc->has_gamma_prop ? gamma_lut : NULL);
> >  		crtc_state->color_mgmt_changed |= replaced;
> >  	}
> >  
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index ba839e5e357d..4d9e217e5040 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1084,6 +1084,9 @@ struct drm_crtc {
> >  	 */
> >  	uint16_t *gamma_store;
> >  
> > +	bool has_gamma_prop : 1;
> > +	bool has_degamma_prop : 1;

I'm a bit behind on patches, but in case this got missed please remove
this and replace with the (obj, prop) lookup function thing or something
like that. Makes sure everything stays in sync, plus like I said atomic
uses this a ton. So not a problem here.
-Daniel

> > +
> >  	/** @helper_private: mid-layer private data */
> >  	const struct drm_crtc_helper_funcs *helper_private;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel