Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF

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

On 2023-09-07 03:49, Pekka Paalanen wrote:
> On Wed, 6 Sep 2023 16:15:10 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2023-08-25 10:18, Melissa Wen wrote:
>>> On 08/22, Pekka Paalanen wrote:  
>>>> On Thu, 10 Aug 2023 15:02:47 -0100
>>>> Melissa Wen <mwen@igalia.com> wrote:
>>>>  
>>>>> Instead of relying on color block names to get the transfer function
>>>>> intention regarding encoding pixel's luminance, define supported
>>>>> Electro-Optical Transfer Functions (EOTFs) and inverse EOTFs, that
>>>>> includes pure gamma or standardized transfer functions.
>>>>>
>>>>> Suggested-by: Harry Wentland <harry.wentland@amd.com>
>>>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>>>> ---
>>>>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 19 +++--
>>>>>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 69 +++++++++++++++----
>>>>>  2 files changed, 67 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> index c749c9cb3d94..f6251ed89684 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> @@ -718,14 +718,21 @@ extern const struct amdgpu_ip_block_version dm_ip_block;
>>>>>  
>>>>>  enum amdgpu_transfer_function {
>>>>>  	AMDGPU_TRANSFER_FUNCTION_DEFAULT,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_SRGB,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_BT709,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_PQ,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_BT709_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_PQ_EOTF,
>>>>>  	AMDGPU_TRANSFER_FUNCTION_LINEAR,
>>>>>  	AMDGPU_TRANSFER_FUNCTION_UNITY,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_GAMMA22,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_GAMMA24,
>>>>> -	AMDGPU_TRANSFER_FUNCTION_GAMMA26,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA22_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA24_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA26_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_SRGB_INV_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_BT709_INV_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_PQ_INV_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA22_INV_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA24_INV_EOTF,
>>>>> +	AMDGPU_TRANSFER_FUNCTION_GAMMA26_INV_EOTF,
>>>>> +        AMDGPU_TRANSFER_FUNCTION_COUNT
>>>>>  };
>>>>>  
>>>>>  struct dm_plane_state {
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>>> index 56ce008b9095..cc2187c0879a 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>>> @@ -85,18 +85,59 @@ void amdgpu_dm_init_color_mod(void)
>>>>>  }
>>>>>  
>>>>>  #ifdef AMD_PRIVATE_COLOR
>>>>> -static const struct drm_prop_enum_list amdgpu_transfer_function_enum_list[] = {
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_DEFAULT, "Default" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_SRGB, "sRGB" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_BT709, "BT.709" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_LINEAR, "Linear" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_UNITY, "Unity" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
>>>>> -	{ AMDGPU_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
>>>>> +static const char * const
>>>>> +amdgpu_transfer_function_names[] = {
>>>>> +	[AMDGPU_TRANSFER_FUNCTION_DEFAULT]		= "Default",
>>>>> +	[AMDGPU_TRANSFER_FUNCTION_LINEAR]		= "Linear",  
>>>>
>>>> Hi,
>>>>
>>>> if the below is identity, then what is linear? Is there a coefficient
>>>> (multiplier) somewhere? Offset?
>>>>  
>>>>> +	[AMDGPU_TRANSFER_FUNCTION_UNITY]		= "Unity",  
>>>>
>>>> Should "Unity" be called "Identity"?  
>>>
>>> AFAIU, AMD treats Linear and Unity as the same: Identity. So, IIUC,
>>> indeed merging both as identity sounds the best approach.     
>>
>> Agreed.
>>
>>>>
>>>> Doesn't unity mean that the output is always 1.0 regardless of input?
>>>>  
>>>>> +	[AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF]		= "sRGB EOTF",
>>>>> +	[AMDGPU_TRANSFER_FUNCTION_BT709_EOTF]		= "BT.709 EOTF",  
>>>>
>>>> BT.709 says about "Overall opto-electronic transfer characteristics at
>>>> source":
>>>>
>>>> 	In typical production practice the encoding function of image
>>>> 	sources is adjusted so that the final picture has the desired
>>>> 	look, as viewed on a reference monitor having the reference
>>>> 	decoding function of Recommendation ITU-R BT.1886, in the
>>>> 	reference viewing environment defined in Recommendation ITU-R
>>>> 	BT.2035.
>>>>
>>>> IOW, typically people tweak the encoding function instead of using
>>>> BT.709 OETF as is, which means that inverting the BT.709 OETF produces
>>>> something slightly unknown. The note about BT.1886 means that that
>>>> something is also not quite how it's supposed to be turned into light.
>>>>
>>>> Should this enum item be "BT.709 inverse OETF" and respectively below a
>>>> "BT.709 OETF"?
>>>>
>>>> What curve does the hardware actually implement?  
>>>
>>> Hmmmm.. I think I got confused in using OETF here since it's done within
>>> a camera. Looking at the coefficients used by AMD color module when not
>>> using ROM but build encoding and decoding curves[1] on pre-defined TF
>>> setup, I understand it's using OETF parameters for building both sRGB
>>> and BT 709:
>>>
>>> ```
>>> /*sRGB     709     2.2 2.4 P3*/
>>> static const int32_t numerator01[] = { 31308,   180000, 0,  0,  0};
>>> static const int32_t numerator02[] = { 12920,   4500,   0,  0,  0};
>>> static const int32_t numerator03[] = { 55,      99,     0,  0,  0};
>>> static const int32_t numerator04[] = { 55,      99,     0,  0,  0};
>>> static const int32_t numerator05[] = { 2400,    2222,   2200, 2400, 2600};
>>> ```
>>>   
>>
>> The first column here looks like the sRGB coefficients in Skia:
>> https://skia.googlesource.com/skia/+/19936eb1b23fef5187b07fb2e0e67dcf605c0672/include/core/SkColorSpace.h#46
>>
>> The color module uses the same coefficients to calculate the transform
>> to linear space and from linear space. So it would support a TF and its
>> inverse.
>>
>> From what I understand for sRGB and PQ its the EOTF and its inverse.
>>
>> For BT.709 we should probably call it BT.709 inverse OETF (instead of
>> EOTF) and BT.709 OETF (instead of inverse EOTF).
>>
>> While I'm okay to move ahead with these AMD driver-specific properties
>> without IGT tests (since they're not enabled and not UABI) we really
>> need IGT tests once they become UABI with the Color Pipeline API. And we
>> need more than just CRC testing. We'll need to do pixel-by-pixel comparison
>> so we can verify that the KMS driver behaves exactly how we expect for a
>> large range of values.
> 
> Yes, please, very much, about the generic color UAPI.
> 
> I believe IGT should contain the reference curve for all named fixed
> curves computed with standard libc math functions in double precision,
> and compute error statistics between that and hardware results.
> The actual test image would iterate through e.g. 1024 (all 10-bit
> values for integer format framebuffer) different values - 1024 is
> nothing as a number of pixels. Then we decide on acceptable error
> thresholds.
> 

1024 isn't a lot of values and fine if we test R, G, and B independently.
Unfortunately 1024^3 is about a billion pixels, so for testing 3DLUTs
(or other cases where we need to test the combination of RGB together)
we won't be able to cover all inputs with a single framebuffer.

> It should also be tested with a floating-point framebuffer format, FP16
> or FP32, with a value distribution designed to be sensitive to typical
> numerical problems. For example, an inverse EOTF should be carefully
> tested with values near zero, since those are the most problematic and
> likely cause the most visible errors.
> 
> Once all that is done, we can be very sure of what curve any hardware
> actually implements.
> 
> I might even go far enough to suggest that any generic color UAPI with
> named fixed curves cannot land without such tests.
> 

I tend to agree, though I think the same should on some level apply to
custom LUTs or other custom transforms.

The IGT tests I'm writing will each have a "transform" function that does
the transform in CPU as reference.

Harry

> 
> Thanks,
> pq
> 
>>
>> Harry
>>
>>> Then EOTF and inverse EOTF for PQ [2], and OETF and it seems an inverse
>>> OETF but called EOTF for HLG[3]. But I'm an external dev, better if
>>> Harry can confirm.
>>>
>>> Thank you for pointing it out.
>>>
>>> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/amd/display/modules/color/color_gamma.c#n55
>>> [2] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/amd/display/modules/color/color_gamma.c#n106
>>> [3] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/amd/display/modules/color/color_gamma.c#n174
>>>   
>>>>
>>>> The others seem fine to me.
>>>>
>>>>
>>>> Thanks,
>>>> pq