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

From: Pekka Paalanen
Date: Thu Sep 07 2023 - 11:39:03 EST

Next message: Laurent Pinchart: "Re: [PATCH 1/3] dt-bindings: media: Add THine THP7312 ISP"
Previous message: AngeloGioacchino Del Regno: "Re: [PATCH 4/6] drm: ci: Enable configs to fix mt8173 boot hang issue"
In reply to: Harry Wentland: "Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF"
Next in thread: Harry Wentland: "Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 6 Sep 2023 16:15:10 -0400
Harry Wentland <harry.wentland@xxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxx>
> >>> Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx>
> >>> ---
> >>> .../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.

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.

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

Attachment: pgpIhejAg7b6E.pgp
Description: OpenPGP digital signature

Next message: Laurent Pinchart: "Re: [PATCH 1/3] dt-bindings: media: Add THine THP7312 ISP"
Previous message: AngeloGioacchino Del Regno: "Re: [PATCH 4/6] drm: ci: Enable configs to fix mt8173 boot hang issue"
In reply to: Harry Wentland: "Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF"
Next in thread: Harry Wentland: "Re: [PATCH v2 07/34] drm/amd/display: explicitly define EOTF and inverse EOTF"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]