RE: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes

["Shankar, Uma" <uma.shankar@intel.com>]

> -----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.

> 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.

> 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/

Regards,
Uma Shankar

> --
> Ville Syrjälä
> Intel