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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sat, Dec 05, 2020 at 12:35:25AM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Dec 03, 2020 at 01:48:44PM +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>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++---
> >  drivers/gpu/drm/drm_color_mgmt.c    |  4 ++++
> >  include/drm/drm_crtc.h              |  3 +++
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ba1507036f26..fe59c8ea42a9 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,
> > @@ -3525,6 +3529,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >  	struct drm_color_lut *blob_data;
> >  	int i, ret = 0;
> >  	bool replaced;
> > +	bool use_degamma;
> > +
> > +	if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
> > +		return -ENODEV;
> > +
> > +	use_degamma = !crtc->has_gamma_prop;
> >  
> >  	state = drm_atomic_state_alloc(crtc->dev);
> >  	if (!state)
> > @@ -3554,10 +3564,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,
> > +					      use_degamma ? blob : NULL);
> >  	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,
> > +					      use_degamma ? NULL : blob);
> >  	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/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index ba839e5e357d..9e1f06047e3d 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;
> > +	bool has_degamma_prop;
> 
> We could use a bitfield to save a bit of memory. Apart from that, the
> patch looks good to me.

Or we could just check if the crtc has the relevant prop or not.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  	/** @helper_private: mid-layer private data */
> >  	const struct drm_crtc_helper_funcs *helper_private;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel