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

[Harry Wentland]

On 2021-11-09 16:45, Ville Syrjälä wrote:
> 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 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.
> 
> 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?
> 

Got it, I think.

Piece-wise linear LUT, i.e. a (usually segmented) LUT that
linearly interpolates in between entries.

Harry