On 08/22, Pekka Paalanen wrote: > On Thu, 10 Aug 2023 15:02:47 -0100 > Melissa Wen <[email protected]> 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 <[email protected]> > > Signed-off-by: Melissa Wen <[email protected]> > > --- > > .../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.
>
> 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};
```
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
>
> > + [AMDGPU_TRANSFER_FUNCTION_PQ_EOTF] = "PQ EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA22_EOTF] = "Gamma 2.2 EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA24_EOTF] = "Gamma 2.4 EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA26_EOTF] = "Gamma 2.6 EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_SRGB_INV_EOTF] = "sRGB inv_EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_BT709_INV_EOTF] = "BT.709 inv_EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_PQ_INV_EOTF] = "PQ inv_EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA22_INV_EOTF] = "Gamma 2.2 inv_EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA24_INV_EOTF] = "Gamma 2.4 inv_EOTF",
> > + [AMDGPU_TRANSFER_FUNCTION_GAMMA26_INV_EOTF] = "Gamma 2.6 inv_EOTF",
> > };
> >
> > +static const u32 amdgpu_eotf =
> > + BIT(AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_BT709_EOTF) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_PQ_EOTF) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_GAMMA22_EOTF) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_GAMMA24_EOTF) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_GAMMA26_EOTF);
> > +
> > +static struct drm_property *
> > +amdgpu_create_tf_property(struct drm_device *dev,
> > + const char *name,
> > + u32 supported_tf)
> > +{
> > + u32 transfer_functions = supported_tf |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_DEFAULT) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_LINEAR) |
> > + BIT(AMDGPU_TRANSFER_FUNCTION_UNITY);
> > + struct drm_prop_enum_list enum_list[AMDGPU_TRANSFER_FUNCTION_COUNT];
> > + int i, len;
> > +
> > + len = 0;
> > + for (i = 0; i < AMDGPU_TRANSFER_FUNCTION_COUNT; i++) {
> > + if ((transfer_functions & BIT(i)) == 0)
> > + continue;
> > +
> > + enum_list[len].type = i;
> > + enum_list[len].name = amdgpu_transfer_function_names[i];
> > + len++;
> > + }
> > +
> > + return drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> > + name, enum_list, len);
> > +}
> > +
> > int
> > amdgpu_dm_create_color_properties(struct amdgpu_device *adev)
> > {
> > @@ -116,11 +157,9 @@ amdgpu_dm_create_color_properties(struct amdgpu_device
> > *adev)
> > return -ENOMEM;
> > adev->mode_info.plane_degamma_lut_size_property = prop;
> >
> > - prop = drm_property_create_enum(adev_to_drm(adev),
> > - DRM_MODE_PROP_ENUM,
> > - "AMD_PLANE_DEGAMMA_TF",
> > - amdgpu_transfer_function_enum_list,
> > -
> > ARRAY_SIZE(amdgpu_transfer_function_enum_list));
> > + prop = amdgpu_create_tf_property(adev_to_drm(adev),
> > + "AMD_PLANE_DEGAMMA_TF",
> > + amdgpu_eotf);
> > if (!prop)
> > return -ENOMEM;
> > adev->mode_info.plane_degamma_tf_property = prop;
>
signature.asc
Description: PGP signature
