| Message ID | 20230810160314.48225-8-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: add AMD driver-specific properties for color mgmt | expand |
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"? 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? 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;
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. > > 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; >
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. 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 >> >>> + [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; >> > >
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. 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
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
On Thu, 7 Sep 2023 10:10:50 -0400 Harry Wentland <harry.wentland@amd.com> wrote: > 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(-) ... > >> 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. Of course, runtimes need to be practical. That idea was for 1D curves, and 3D mappings need a different distribution. > > 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. Sounds good! For testing optical-to-electrical kind of operations, one idea is to sample the electrical target space, and reverse the reference transform to come up with the test input values. That way one can test if the output space is sufficiently covered, and the rounding behavior as well. Electrical space usually tends to be integer encoded with not too many bits, making even exhaustive sampling feasible for 1D curves. Thanks, pq > >>> 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 >
On 09/06, Harry Wentland 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). I see. I'll update the transfer function list and docs accordingly. Thanks Melissa > > 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. > > 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 > >> > >>> + [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; > >> > > > > >
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", + [AMDGPU_TRANSFER_FUNCTION_UNITY] = "Unity", + [AMDGPU_TRANSFER_FUNCTION_SRGB_EOTF] = "sRGB EOTF", + [AMDGPU_TRANSFER_FUNCTION_BT709_EOTF] = "BT.709 EOTF", + [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;
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(-)