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

[Pekka Paalanen]

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"?

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;

Attachment: pgp0XNlKKW2T8.pgp
Description: OpenPGP digital signature