Re: [PATCH 06/16] drm/connector: Allow drivers to pass list of supported colorspaces

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13672 bytes --]

Sorry, hand slipped on keyboard and sent out a draft of this email too
early.


On Mon, 12 Dec 2022 13:21:27 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> Drivers might not support all colorspaces defined in
> dp_colorspaces and hdmi_colorspaces. This results in
> undefined behavior when userspace is setting an
> unsupported colorspace.
> 
> Allow drivers to pass the list of supported colorspaces
> when creating the colorspace property.

Hi Harry,

what is there for drivers to support? Isn't this just infoframe data
that shall be sent down to the sink as-is with no other effect?
The kerneldoc for "Colorspace" says it has no other effect.

Is the driver confusing colorimetry with color-representation (the
RGB-YCbCr conversion)? Or is this property defining both?

I feel that the documentation of "Colorspace" KMS connector property
needs clarification, and a list of potentially available values with
explanations, more than just a reference to CTA-861-H which it does not
even do yet.

Perhaps a table, where for each enum drm_colorspace entry has a row
explaining the expectations of the sink:
- primaries and white point
- transfer characteristic
- YCbCr-RGB or similar conversion to/from some RGB

Each cell can be a reference to a spec like BT.709 or BT.601 (525 line).

I think this belongs in the kernel doc more than in color-and-hdr.

CTA-861-H does not give all the information but refers to things like
xvYCC601 which you then need to figure out from Wikipedia or whatever
which is annoying and raises questions about its correctness. Would be
better if someone who actually has access to the authoritative specs
would review the table.


Thanks,
pq


> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_connector.c               | 140 +++++++++---------
>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>  include/drm/drm_connector.h                   |   8 +-
>  4 files changed, 83 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index ddba0b9fcc17..0df5db3e4fec 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1012,64 +1012,57 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>  		 drm_dp_subconnector_enum_list)
>  
> -static const struct drm_prop_enum_list hdmi_colorspaces[] = {
> -	/* For Default case, driver will set the colorspace */
> -	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> -	/* Standard Definition Colorimetry based on CEA 861 */
> -	{ DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" },
> -	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> -	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> -	{ DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> -	/* High Definition Colorimetry based on IEC 61966-2-4 */
> -	{ DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> -	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> -	{ DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> -	/* Colorimetry based on IEC 61966-2-5 [33] */
> -	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> -	/* Colorimetry based on IEC 61966-2-5 */
> -	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> -	/* Added as part of Additional Colorimetry Extension in 861.G */
> -	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> -	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> +static const char * const colorspace_names[] = {
> +	[DRM_MODE_COLORIMETRY_DEFAULT] = "Default",
> +	[DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC",
> +	[DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC",
> +	[DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601",
> +	[DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709",
> +	[DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601",
> +	[DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601",
> +	[DRM_MODE_COLORIMETRY_OPRGB] = "opRGB",
> +	[DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC",
> +	[DRM_MODE_COLORIMETRY_BT2020_RGB] = "BT2020_RGB",
> +	[DRM_MODE_COLORIMETRY_BT2020_YCC] = "BT2020_YCC",
> +	[DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "P3_RGB_D65",
> +	[DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "P3_RGB_Theater",
> +	[DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED",
> +	[DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT",
> +	[DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
>  };
>  
> +static const u32 hdmi_colorspaces =
> +	BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) |
> +	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> +	BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
> +	BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC) |
> +	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
> +	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
> +
>  /*
>   * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry
>   * Format Table 2-120
>   */
> -static const struct drm_prop_enum_list dp_colorspaces[] = {
> -	/* For Default case, driver will set the colorspace */
> -	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> -	{ DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, "RGB_Wide_Gamut_Fixed_Point" },
> -	/* Colorimetry based on scRGB (IEC 61966-2-2) */
> -	{ DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT, "RGB_Wide_Gamut_Floating_Point" },
> -	/* Colorimetry based on IEC 61966-2-5 */
> -	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> -	/* Colorimetry based on SMPTE RP 431-2 */
> -	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> -	{ DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
> -	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> -	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> -	{ DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> -	/* High Definition Colorimetry based on IEC 61966-2-4 */
> -	{ DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> -	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> -	{ DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> -	/* Colorimetry based on IEC 61966-2-5 [33] */
> -	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> -	/* Colorimetry based on ITU-R BT.2020 */
> -	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> -};
> +static const u32 dp_colorspaces =
> +	BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED) |
> +	BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT) |
> +	BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> +	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
> +	BIT(DRM_MODE_COLORIMETRY_BT601_YCC) |
> +	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> +	BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
> +	BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
> +	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC);
>  
>  /**
>   * DOC: standard connector properties
> @@ -1972,21 +1965,34 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>   */
>  
>  static int drm_mode_create_colorspace_property(struct drm_connector *connector,
> -					const struct drm_prop_enum_list *colorspaces,
> -					int size)
> +					u32 supported_colorspaces)
>  {
>  	struct drm_device *dev = connector->dev;
> +	u32 colorspaces = supported_colorspaces | BIT(DRM_MODE_COLORIMETRY_DEFAULT);
> +	struct drm_prop_enum_list enum_list[DRM_MODE_COLORIMETRY_MAX];
> +	int i, len;
>  
>  	if (connector->colorspace_property)
>  		return 0;
>  
> -	if (!colorspaces)
> -		return 0;
> +	if (WARN_ON(supported_colorspaces == 0 ||
> +		    (supported_colorspaces & -BIT(DRM_MODE_COLORIMETRY_MAX)) != 0))
> +		return -EINVAL;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_MODE_COLORIMETRY_MAX; i++) {
> +		if ((colorspaces & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = colorspace_names[i];
> +		len++;
> +	}
>  
>  	connector->colorspace_property =
>  		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
> -					colorspaces,
> -					size);
> +					enum_list,
> +					len);
>  
>  	if (!connector->colorspace_property)
>  		return -ENOMEM;
> @@ -2003,11 +2009,12 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
> -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector)
> +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
> +					     u32 supported_colorspaces)
>  {
> -	return drm_mode_create_colorspace_property(connector,
> -						   hdmi_colorspaces,
> -						   ARRAY_SIZE(hdmi_colorspaces));
> +	u32 colorspaces = supported_colorspaces & hdmi_colorspaces;
> +
> +	return drm_mode_create_colorspace_property(connector, colorspaces);
>  }
>  EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
>  
> @@ -2021,11 +2028,12 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
> -int drm_mode_create_dp_colorspace_property(struct drm_connector *connector)
> +int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
> +					   u32 supported_colorspaces)
>  {
> -	return drm_mode_create_colorspace_property(connector,
> -						   dp_colorspaces,
> -						   ARRAY_SIZE(dp_colorspaces));
> +	u32 colorspaces = supported_colorspaces & dp_colorspaces;
> +
> +	return drm_mode_create_colorspace_property(connector, colorspaces);
>  }
>  EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
> index 1dcc268927a2..6e7cef58a626 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -283,13 +283,13 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
>  void
>  intel_attach_hdmi_colorspace_property(struct drm_connector *connector)
>  {
> -	if (!drm_mode_create_hdmi_colorspace_property(connector))
> +	if (!drm_mode_create_hdmi_colorspace_property(connector, 0xffffffff))
>  		drm_connector_attach_colorspace_property(connector);
>  }
>  
>  void
>  intel_attach_dp_colorspace_property(struct drm_connector *connector)
>  {
> -	if (!drm_mode_create_dp_colorspace_property(connector))
> +	if (!drm_mode_create_dp_colorspace_property(connector, 0xffffffff))
>  		drm_connector_attach_colorspace_property(connector);
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 6ab83296b0e4..8d08d6a36f37 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -416,7 +416,7 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_mode_create_hdmi_colorspace_property(connector);
> +	ret = drm_mode_create_hdmi_colorspace_property(connector, 0xffffffff);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index edef65388c29..5825c6ab969b 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -30,6 +30,7 @@
>  #include <linux/notifier.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_util.h>
> +#include <drm/drm_property.h>
>  
>  #include <uapi/drm/drm_mode.h>
>  
> @@ -393,6 +394,7 @@ enum drm_colorspace {
>  	DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
>  	DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
>  	DRM_MODE_COLORIMETRY_BT601_YCC,
> +	DRM_MODE_COLORIMETRY_MAX
>  };
>  
>  /**
> @@ -1818,8 +1820,10 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
>  bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
>  					     struct drm_connector_state *new_state);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> -int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector);
> -int drm_mode_create_dp_colorspace_property(struct drm_connector *connector);
> +int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
> +					     u32 supported_colorspaces);
> +int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
> +					   u32 supported_colorspaces);
>  int drm_mode_create_content_type_property(struct drm_device *dev);
>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]