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

[Harry Wentland]

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

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

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

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.

Harry