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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 07, 2020 at 05:45:02PM +0200, Tomi Valkeinen wrote:
> On 07/12/2020 17:31, Ville Syrjälä wrote:
> > 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.
> 
> That's what I did at first, but it requires iterating over the props (unless I missed a simple way
> to just check it). Probably not noticeable (in performance) but just felt a bit ugly.

Every atomic prop set iterates over that array somewhere for checking.
See drm_mode_obj_find_prop_id(). If that's too much, then we need a
hashtable or something like that just to make atomic commits acceptable.
I'd use that, instead of duplicating information, if at all possible. You
really don't have to worry about that loop, as long as you use that
function which atomic uses already.
-Daniel
-- 
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