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

[Harry Wentland <harry.wentland@amd.com>]

On 2021-11-12 03:37, Pekka Paalanen wrote:
> On Thu, 11 Nov 2021 21:58:35 +0000
> "Shankar, Uma" <uma.shankar@intel.com> wrote:
> 
>>> -----Original Message-----
>>> From: Harry Wentland <harry.wentland@amd.com>
>>> Sent: Friday, November 12, 2021 2:41 AM
>>> To: Shankar, Uma <uma.shankar@intel.com>; Ville Syrjälä
>>> <ville.syrjala@linux.intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>>> ppaalanen@gmail.com; brian.starkey@arm.com; sebastian@sebastianwick.net;
>>> Shashank.Sharma@amd.com
>>> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for
>>> HDR planes
>>>
>>>
>>>
>>> On 2021-11-11 15:42, Shankar, Uma wrote:  
>>>>
>>>>  
>>>>> -----Original Message-----
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Sent: Thursday, November 11, 2021 10:13 PM
>>>>> To: Harry Wentland <harry.wentland@amd.com>
>>>>> Cc: Shankar, Uma <uma.shankar@intel.com>;
>>>>> intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org;
>>>>> ppaalanen@gmail.com; brian.starkey@arm.com;
>>>>> sebastian@sebastianwick.net; Shashank.Sharma@amd.com
>>>>> Subject: Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range
>>>>> struct for HDR planes
>>>>>
>>>>> On Thu, Nov 11, 2021 at 10:17:17AM -0500, 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,
>>>>>>> +		.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,
>>>>>>> +		.min = 0, .max = (1 << 27) - 1,
>>>>>>> +	},
>>>>>>> +	/* 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,
>>>>>>> +	},
>>>>>>> +};  
>>>>>>
>>>>>> If I understand this right, userspace would need this definition in
>>>>>> order to populate the degamma blob. Should this sit in a UAPI header?  
> 
> Are you asking whether 'struct drm_color_lut_range` is defined in any
> userspace visible header?
> 
> It seems to be in patch 2.
> 
>>>>
>>>> Hi Harry, Pekka and Ville,
>>>> Sorry for being a bit late on the replies, got side tracked with various issues.
>>>> I am back on this. Apologies for delay.
>>>>  
>>>>> My original idea (not sure it's fully realized in this series) is to
>>>>> have a new GAMMA_MODE/etc. enum property on each crtc (or plane) for
>>>>> which each enum value points to a kernel provided blob that contains one of  
>>> these LUT descriptors.  
>>>>> Userspace can then query them dynamically and pick the best one for
>>>>> its current use case.  
>>>>
>>>> We have this as part of the series Ville. Patch 3 of this series
>>>> creates a DEGAMMA_MODE property just for this. With that userspace can
>>>> just query the blob_id's and will get the various degamma mode possible and the  
>>> respective segment and lut distributions.  
>>>>
>>>> This will be generic, so for userspace it should just be able to query
>>>> this and parse and get the lut distribution and segment ranges.
>>>>  
>>>
>>> Thanks for the explanation.
>>>
>>> Uma, have you had a chance to sketch some of this out in IGT? I'm trying to see how
>>> userspace would do this in practice and will try to sketch an IGT test for this myself,
>>> but if you have it already we could share the effort.  
>>
>> Yes Harry, we do have some sample IGT's to test this. Will send those out and will copy
>> you and all the stakeholders.
>>

Thanks. The set is on my list of items to review.

>>>>> The algorithm for choosing the best one might be something like:
>>>>> - prefer LUT with bpc >= FB bpc, but perhaps not needlessly high bpc
>>>>> - prefer interpolated vs. direct lookup based on current needs (eg. X
>>>>>   could prefer direct lookup to get directcolor visuals).
>>>>> - prefer one with extended range values if needed
>>>>> - for HDR prefer smaller step size in dark tones,
>>>>>   for SDR perhaps prefer a more uniform step size
>>>>>
>>>>> Or maybe we should include some kind of usage hints as well?  
>>>>
>>>> I think the segment range and distribution of lut should be enough for
>>>> a userspace to pick the right ones, but we can add some examples in UAPI  
>>> descriptions as hints.  
>>>>  
>>>
>>> The range might be enough, but we're already parsing hints like "GAMMA"
>>> or "DEGAMMA". I wonder if it would make sense to add a flag for "HDR" or "SDR" as
>>> well.  
> 
> What hints are GAMMA or DEGAMMA and who's parsing them? I thought they
> are just arbitrary names to identify the element's position in the
> abstract pipeline.
> 

They are provided with the segment definitions, e.g. in 
https://patchwork.freedesktop.org/patch/452589/?series=90826&rev=2

I believe they are indicating whether a segment definition is
intended for degamma (linearization) use or for gamma (delinearization)
use.

>>
>> On Intel hardware, we differentiate this with precision and have HDR planes (they have extra
>> Lut precision and samples) separately called out. We could add SDR/HDR FLAG as well.
> 
> What about wide color gamut SDR? That probably needs more precision
> than "normal" SDR but is not HDR.
> 
> I can't think of how SDR/HDR flags would work or what they would mean.
> Feels a bit too simple for practice. Maybe that concept should be
> created by a hypothetical userspace helper library instead.
> 

Maybe this is a decision best left up to compositors. A compositor
will know best what precision and range it needs.

Harry

> 
> Thanks,
> pq
>