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

From: Daniel Vetter <daniel@ffwll.ch>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	dri-devel@lists.freedesktop.org, Sekhar Nori <nsekhar@ti.com>,
	Jyri Sarha <jsarha@ti.com>, Nikhil Devshatwar <nikhil.nd@ti.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma
Date: Mon, 30 Nov 2020 15:10:57 +0100	[thread overview]
Message-ID: <20201130141057.GL401619@phenom.ffwll.local> (raw)
In-Reply-To: <f744a803-6d8c-5188-7bfa-93cc05cc26bf@ti.com>

On Mon, Nov 30, 2020 at 02:12:39PM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 12:38, Laurent Pinchart wrote:
> 
> >> + * can be used when the driver exposes either only GAMMA_LUT or both GAMMA_LUT
> >> + * and DEGAMMA_LUT.
> >> + */
> >> +int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >> +				       u16 *red, u16 *green, u16 *blue,
> >> +				       uint32_t size,
> >> +				       struct drm_modeset_acquire_ctx *ctx)
> >> +{
> >> +	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, false);
> >> +}
> > 
> > I wonder, would it make sense to make this automatic by setting the
> > degamma LUT when only the DEGAMMA_LUT property exists, and the gamma LUT
> > otherwise ? Are there use cases for drm_atomic_helper_legacy_degamma_set
> > for drivers that support both gamma and degamma ?
> 
> Yes, I think drm_atomic_helper_legacy_gamma_set() could do that.
> 
> But if you look at the second patch, the driver deals with crtc_state->degamma_lut. Having
> .gamma_set = drm_atomic_helper_legacy_degamma_set makes it bit more explicit and clear what the
> driver is doing.
> 
> That said, documenting what drm_atomic_helper_legacy_gamma_set does if there's only degamma should
> also be clear enough, so... I don't have strong feelings either way =).

The thing is, the legacy helpers should be able to pull off what userspace
needs to do when it's using atomic anyway. Hard-coding information in the
kernel means we have a gap here. Hence imo legacy helpers doing the right
thing in all reasonable cases is imo better.

In many cases I think we should even go further, and ditch driver ability
to overwrite legacy helper hooks like this. I thought we'd need that
flexibility for legacy userspace being incompatible in awkward ways, but
wasn't ever really needed. Worse, many drivers forget to wire up the
compat hooks.

tldr, imo right thing to do here:
- move legacy gamma function from helpers into core code
- call it unconditionally for all atomic drivers (if there's no legacy
  drivers using the hook left then we can outright remove it)
- make sure it dtrt in all cases

Cheers, 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