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@xxxxxxxxx>
>>> ---
>>> 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
