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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 09, 2021 at 03:19:47PM -0500, Harry Wentland wrote:
> On 2021-11-05 08:59, Ville Syrjälä wrote:
> > On Wed, Nov 03, 2021 at 11:10:37AM -0400, 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,
> >>
> >> Is the distribution of the 128 entries uniform?
> > 
> > I guess this is the plane gamma thing despite being in intel_color.c,
> > so yeah I think that's correct.
> > 
> >> If so, is a
> >> uniform distribution of 128 points across most of the LUT
> >> good enough for HDR with 128 entries?
> > 
> > No idea how good this actually is. It is .24 so at least
> > it does have a fair bit of precision.
> > 
> 
> Precision is good but you also need enough samples. Though that's
> probably less my concern and more your concern and should become
> apparent once its used.

Yeah, for pipe gamma we have a few different variants with
non-uniform spacing of the samples. But not here AFAICS for 
whatever reason.

> 
> >>
> >>> +		.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,
> >>
> >> .start and .end are only a single entry apart. Is this correct?
> > 
> > One think I wanted to do is simplify this stuff by getting rid of
> > .end entirely. So I think this should just be '.start=1<<24' (or
> > whatever way we decide to specify the input precision, which is
> > I think another slightly open question).
> > 
> > So for this thing we could just have:
> > { .count = 128, .min = 0, .max = (1 << 24) - 1, .start = 0       },
> > { .count = 1,   .min = 0, .max = (7 << 24) - 1, .start = 1 << 24 },
> > { .count = 1,   .min = 0, .max = (7 << 24) - 1, .start = 3 << 24 },
> > { .count = 1,   .min = 0, .max = (7 << 24) - 1, .start = 7 << 24 },
> > 
> > + flags/etc. which I left out for brevity.
> > 
> 
> Makes sense. I like this.
> 
> > So that is trying to indicate that the first 129 entries are equally
> > spaced, and would be used to interpolate for input values [0.0,1.0).
> > Input values [1.0,3.0) would interpolate between entry 128 and 129,
> > and [3.0,7.0) would interpolate between entry 129 and 130.
> > 
> 
> What in the segment definition defines the 1.0 mark? In your example
> it seems to be at (1 << 24) but then we would have values that go
> beyond the input_bpc for the last three segments.

Yes, input_bpc would define the precision of the input values (.start).
so 1.0 would be at 1<<input_bpc. Tne range of input values is allowed to
extend outside the 0.0-1.0 range.

> 
> How about output_bpc? Would output_bpc somehow limit the U32.32 (or
> S31.32) entries, and if so, how?

output_bpc would define the actual precision of the output values,
so again 1.0 would be 1<<output_bpc, and .min and .max define the
min/max values (which can extend outside the 0.0-1.0 range). The
alternative I guess would be to not have .output_bpc at all and
just have .min/.max be s32.32 values. Though then you can't tell
what the actual precision is. Same could be done for .input_bpc
I suppose.

> 
> Or should we treat input_/output_bpc only as capability reporting, so
> userspace can calculate the possible error when programming the LUT?
> Again, this leaves us with the question what the input_/output_bpc
> means for our PWL entries.

Yeah, I mostly thought they might be interesting if userspace wants
to know the exact precision. But not strictly necessary if you want
just to go generate a "close enough" curve. 

What's PWL?

-- 
Ville Syrjälä
Intel