RE: [PATCH 04/21] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes

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

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Harry
> Wentland
> Sent: Monday, June 28, 2021 8:45 PM
> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Subject: Re: [PATCH 04/21] drm/i915/xelpd: Define Degamma Lut range struct for
> HDR planes
> 
> On 2021-06-01 6:52 a.m., 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 dab892d2251b..c735d06a6b54 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -2093,6 +2093,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 |
> 
> Why are these using DRM_MODE_LUT_GAMMA and not
> DRM_MODE_LUT_DEGAMMA when the lut_type for this LUT is
> LUT_TYPE_DEGAMMA?

Thanks Harry for the comments.

Yeah this is an oversight, will fix this.
> 
> 
> > +			  DRM_MODE_LUT_REFLECT_NEGATIVE |
> > +			  DRM_MODE_LUT_INTERPOLATE |
> > +			  DRM_MODE_LUT_NON_DECREASING),
> > +		.count = 128,
> > +		.input_bpc = 24, .output_bpc = 16,
> 
> Why do we need more than 16 bpc for LUT? FP16 is enough to represent HDR in
> linear space. Wouldn't 16 bpc be enough?

Pipe sometimes works internally on higher precision (just to take care of rounding etc.), later the
extra data gets dropped at the end of the pipe. So from source side you are right, 16bpc is enough
but the lut precision can go higher.

> 
> > +		.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,
> 
> How can max be 1 << 27 if input_bpc is 24?

This is to take care of > 1.0 section. 1.0 to 3.0 and 3.0 to 7.0.
So we have 3.24 format for Lut to take care of this. 

Also, I have an action to update the series with UAPI doc and new naming for the property.
My apologies for being late on that one. Will update and send that out soon.

Thanks & Regards,
Uma Shankar
> 
> Harry
> 
> > +	},
> > +	/* 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,
> > +	},
> > +};
> > +
> >  void intel_color_init(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >