Re: [Intel-gfx] [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8492 bytes --]

On Fri, 12 Nov 2021 16:54:35 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Thu, Nov 11, 2021 at 04:10:41PM -0500, Harry Wentland wrote:
> > 
> > 
> > On 2021-11-11 15:42, Shankar, Uma wrote:  
> > > 
> > >   
> > >> -----Original Message-----
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> Sent: Thursday, November 11, 2021 10:13 PM
> > >> To: Harry Wentland <harry.wentland@amd.com>
> > >> Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> > >> devel@lists.freedesktop.org; ppaalanen@gmail.com; brian.starkey@arm.com;
> > >> sebastian@sebastianwick.net; Shashank.Sharma@amd.com
> > >> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for
> > >> HDR planes
> > >>
> > >> On Thu, Nov 11, 2021 at 10:17:17AM -0500, Harry Wentland wrote:  
> > >>>
> > >>>
> > >>> On 2021-09-06 17:38, Uma Shankar wrote:  
> > >>>> Define the structure with XE_LPD degamma lut ranges. HDR and SDR
> > >>>> planes have different capabilities, implemented respective structure
> > >>>> for the HDR planes.
> > >>>>
> > >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > >>>> ---
> > >>>>  drivers/gpu/drm/i915/display/intel_color.c | 52
> > >>>> ++++++++++++++++++++++
> > >>>>  1 file changed, 52 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > >>>> b/drivers/gpu/drm/i915/display/intel_color.c
> > >>>> index afcb4bf3826c..6403bd74324b 100644
> > >>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
> > >>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > >>>> @@ -2092,6 +2092,58 @@ static void icl_read_luts(struct intel_crtc_state  
> > >> *crtc_state)  
> > >>>>  	}
> > >>>>  }
> > >>>>
> > >>>> + /* FIXME input bpc? */
> > >>>> +__maybe_unused
> > >>>> +static const struct drm_color_lut_range d13_degamma_hdr[] = {
> > >>>> +	/* segment 1 */
> > >>>> +	{
> > >>>> +		.flags = (DRM_MODE_LUT_GAMMA |
> > >>>> +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > >>>> +			  DRM_MODE_LUT_INTERPOLATE |
> > >>>> +			  DRM_MODE_LUT_NON_DECREASING),
> > >>>> +		.count = 128,
> > >>>> +		.input_bpc = 24, .output_bpc = 16,
> > >>>> +		.start = 0, .end = (1 << 24) - 1,
> > >>>> +		.min = 0, .max = (1 << 24) - 1,
> > >>>> +	},
> > >>>> +	/* segment 2 */
> > >>>> +	{
> > >>>> +		.flags = (DRM_MODE_LUT_GAMMA |
> > >>>> +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > >>>> +			  DRM_MODE_LUT_INTERPOLATE |
> > >>>> +			  DRM_MODE_LUT_REUSE_LAST |
> > >>>> +			  DRM_MODE_LUT_NON_DECREASING),
> > >>>> +		.count = 1,
> > >>>> +		.input_bpc = 24, .output_bpc = 16,
> > >>>> +		.start = (1 << 24) - 1, .end = 1 << 24,
> > >>>> +		.min = 0, .max = (1 << 27) - 1,
> > >>>> +	},
> > >>>> +	/* Segment 3 */
> > >>>> +	{
> > >>>> +		.flags = (DRM_MODE_LUT_GAMMA |
> > >>>> +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > >>>> +			  DRM_MODE_LUT_INTERPOLATE |
> > >>>> +			  DRM_MODE_LUT_REUSE_LAST |
> > >>>> +			  DRM_MODE_LUT_NON_DECREASING),
> > >>>> +		.count = 1,
> > >>>> +		.input_bpc = 24, .output_bpc = 16,
> > >>>> +		.start = 1 << 24, .end = 3 << 24,
> > >>>> +		.min = 0, .max = (1 << 27) - 1,
> > >>>> +	},
> > >>>> +	/* Segment 4 */
> > >>>> +	{
> > >>>> +		.flags = (DRM_MODE_LUT_GAMMA |
> > >>>> +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > >>>> +			  DRM_MODE_LUT_INTERPOLATE |
> > >>>> +			  DRM_MODE_LUT_REUSE_LAST |
> > >>>> +			  DRM_MODE_LUT_NON_DECREASING),
> > >>>> +		.count = 1,
> > >>>> +		.input_bpc = 24, .output_bpc = 16,
> > >>>> +		.start = 3 << 24, .end = 7 << 24,
> > >>>> +		.min = 0, .max = (1 << 27) - 1,
> > >>>> +	},
> > >>>> +};  
> > >>>
> > >>> If I understand this right, userspace would need this definition in
> > >>> order to populate the degamma blob. Should this sit in a UAPI header?  
> > > 
> > > Hi Harry, Pekka and Ville,
> > > Sorry for being a bit late on the replies, got side tracked with various issues.
> > > I am back on this. Apologies for delay.
> > >   
> > >> My original idea (not sure it's fully realized in this series) is to have a new
> > >> GAMMA_MODE/etc. enum property on each crtc (or plane) for which each enum
> > >> value points to a kernel provided blob that contains one of these LUT descriptors.
> > >> Userspace can then query them dynamically and pick the best one for its current use
> > >> case.  
> > > 
> > > We have this as part of the series Ville. Patch 3 of this series creates a DEGAMMA_MODE
> > > property just for this. With that userspace can just query the blob_id's and will get the
> > > various degamma mode possible and the respective segment and lut distributions.
> > > 
> > > This will be generic, so for userspace it should just be able to query this and parse and get
> > > the lut distribution and segment ranges.
> > >   
> > 
> > Thanks for the explanation.
> > 
> > Uma, have you had a chance to sketch some of this out in IGT? I'm trying
> > to see how userspace would do this in practice and will try to sketch an
> > IGT test for this myself, but if you have it already we could share the
> > effort.
> >   
> > >> The algorithm for choosing the best one might be something like:
> > >> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc
> > >> - prefer interpolated vs. direct lookup based on current needs (eg. X
> > >>   could prefer direct lookup to get directcolor visuals).
> > >> - prefer one with extended range values if needed
> > >> - for HDR prefer smaller step size in dark tones,
> > >>   for SDR perhaps prefer a more uniform step size
> > >>
> > >> Or maybe we should include some kind of usage hints as well?  
> > > 
> > > I think the segment range and distribution of lut should be enough for a userspace
> > > to pick the right ones, but we can add some examples in UAPI descriptions as hints.
> > >   
> > 
> > The range might be enough, but we're already parsing hints like "GAMMA"
> > or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or
> > "SDR" as well.
> >   
> > >> And I was thinking of even adding a new property type (eg.
> > >> ENUM_BLOB) just for this sort of usecase. That could let us have a bit more generic
> > >> code to do all the validation around the property values and whatnot.
> > >>
> > >> The one nagging concern I really have with GAMMA_MODE is how a mix of old and
> > >> new userspace would work. Though that is more of a generic issue with any new
> > >> property really.  
> > > 
> > > For plane properties getting added newly, old userspace will not get it so I think this should be ok.
> > > Newer userspace will implement this and get the new functionality.
> > > Problem will be in extending this to crtc where we have a legacy baggage, the client caps approach
> > > may help us there. Have it as part of separate series just to not mix it with this new plane stuff, though
> > > the idea remains same based on your design. Series below for reference:  
> > > https://patchwork.freedesktop.org/series/90821/>>   
> > 
> > Could we just assume we do a uniform LUT if userspace doesn't
> > set a _MODE enum value for the respective gamma?
> > 
> > Maybe the _MODE should have a default enum value that means
> > a uniform (legacy) LUT.  
> 
> Yeah, it definitely needs a default like that. But the problem arises
> when new userspace sets it to something else and then hands the reins
> over to some old userspace that doesn't know how to reset it back to
> default.

This very problem is the one where I have been suggesting that
userspace that supports temporarily handing DRM-master to something
else needs to be prepared to save and restore also unrecognized KMS
properties.

We've also had talk about a "reset" switch in KMS, but I forget the
conclusion.

Both ideas lack the people working on them. I don't think we can design
each new KMS property ad hoc to somehow magically be compatible with
old vs. new client interoperation. In fact, the problem exists already
with e.g. the old GAMMA etc. properties.

OTOH, when a userspace client is reported to misbehave because
something else left KMS in a funny state, it is just too easy to simply
patch the innocent but misbehaving client to understand whatever new
property the other one was using, particularly if it's just to reset it
to some hardcoded expected value. So it's unclear if this problem even
needs a solution.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]