Re: [v3 1/3] drm: Add HDMI colorspace property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 20, 2018 at 07:52:09PM +0530, Uma Shankar wrote:
> This patch adds a HDMI colorspace property, enabling
> userspace to switch to various supported colorspaces.
> This will help enable BT2020 along with other colorspaces.
> 
> v2: Addressed Maarten and Ville's review comments. Enhanced
> the colorspace enum to incorporate both HDMI and DP supported
> colorspaces. Also, added a default option for colorspace.
> 
> v3: Removed Adobe references from enum definitions as per
> Ville, Hans Verkuil and Jonas Karlman suggestions. Changed
> Default to an unset state where driver will assign the colorspace
> is not chosen by user, suggested by Ville and Maarten. Addressed
> other misc review comments from Maarten. Split the changes to
> have separate colorspace property for DP and HDMI.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  4 +++
>  drivers/gpu/drm/drm_connector.c   | 62 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  8 +++++
>  include/drm/drm_mode_config.h     |  6 ++++
>  include/uapi/drm/drm_mode.h       | 16 ++++++++++
>  5 files changed, 96 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 86ac339..4e09a38 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->picture_aspect_ratio = val;
>  	} else if (property == config->content_type_property) {
>  		state->content_type = val;
> +	} else if (property == config->hdmi_colorspace_property) {
> +		state->hdmi_colorspace = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
>  	} else if (property == connector->content_protection_property) {
> @@ -797,6 +799,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		*val = state->picture_aspect_ratio;
>  	} else if (property == config->content_type_property) {
>  		*val = state->content_type;
> +	} else if (property == config->hdmi_colorspace_property) {
> +		*val = state->hdmi_colorspace;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
>  	} else if (property == connector->content_protection_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fa9baac..1906e4a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -826,6 +826,30 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>  };
>  DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
>  
> +static const struct drm_prop_enum_list hdmi_colorspace[] = {

My idea was that we create just the one prop, and filter it as needed.

> +	/* Standard Definition Colorimetry based on CEA 861 */
> +	{ COLORIMETRY_ITU_601, "ITU_601" },
> +	{ COLORIMETRY_ITU_709, "ITU_709" },
> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> +	{ COLORIMETRY_S_YCC_601, "S_YCC_601" },
> +	/* Colorimetry based on IEC 61966-2-5 [33] */
> +	{ COLORIMETRY_OPYCC_601, "opYCC_601" },
> +	/* Colorimetry based on IEC 61966-2-5 */
> +	{ COLORIMETRY_OPRGB, "opRGB" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> +	/* For Default case, driver will set the colorspace */
> +	{ COLORIMETRY_DEFAULT, "Default" },

"Default" needs to be the default.

> +};
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -1402,6 +1426,44 @@ int drm_mode_create_aspect_ratio_property(struct drm_device *dev)
>  EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>  
>  /**
> + * drm_mode_create_colorspace_property - create colorspace property
> + * colorspace:
> + *     This property helps select a suitable colorspace based on the sink
> + *     capability. Modern sink devices support wider gamut like BT2020.
> + *     This helps switch to BT2020 mode if the BT2020 encoded video stream
> + *     is being played by the user, same for any other colorspace.
> + * @dev: DRM device
> + *
> + * Called by a driver the first time it's needed, must be attached to desired
> + * connectors.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_create_colorspace_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> +			connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) {
> +		if (dev->mode_config.hdmi_colorspace_property)
> +			return 0;
> +
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +				"HDMI_Colorspace",
> +				hdmi_colorspace, ARRAY_SIZE(hdmi_colorspace));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		dev->mode_config.hdmi_colorspace_property = prop;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_colorspace_property);
> +
> +/**
>   * drm_mode_create_content_type_property - create content type property
>   * @dev: DRM device
>   *
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index af0a761..5fda56d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -497,6 +497,13 @@ struct drm_connector_state {
>  	unsigned int content_protection;
>  
>  	/**
> +	 * @hdmi_colorspace: Connector property to request colorspace
> +	 * change on HDMI Sink. This is most commonly used to switch to
> +	 * wider color gamuts like BT2020.
> +	 */
> +	enum hdmi_full_colorimetry hdmi_colorspace;
> +
> +	/**
>  	 * @writeback_job: Writeback job for writeback connectors
>  	 *
>  	 * Holds the framebuffer and out-fence for a writeback connector. As
> @@ -1272,6 +1279,7 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  int drm_connector_attach_content_protection_property(
>  		struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> +int drm_mode_create_colorspace_property(struct drm_connector *connector);
>  int drm_mode_create_content_type_property(struct drm_device *dev);
>  void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
>  					 const struct drm_connector_state *conn_state);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5dbeabd..ec7cab8 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -869,6 +869,12 @@ struct drm_mode_config {
>  	uint32_t cursor_width, cursor_height;
>  
>  	/**
> +	 * @hdmi_colorspace_property: Connector property to set the suitable
> +	 * colorspace supported by the HDMI sink.
> +	 */
> +	struct drm_property *hdmi_colorspace_property;
> +
> +	/**
>  	 * @suspend_state:
>  	 *
>  	 * Atomic state when suspended.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe3..cc4df26 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -210,6 +210,22 @@
>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>  
> +enum hdmi_full_colorimetry {
> +	/* CEA 861 Normal Colorimetry options */
> +	COLORIMETRY_ITU_601 = 0,
> +	COLORIMETRY_ITU_709,
> +	/* CEA 861 Extended Colorimetry Options */
> +	COLORIMETRY_XV_YCC_601,
> +	COLORIMETRY_XV_YCC_709,
> +	COLORIMETRY_S_YCC_601,
> +	COLORIMETRY_OPYCC_601,
> +	COLORIMETRY_OPRGB,
> +	COLORIMETRY_BT2020_RGB,
> +	COLORIMETRY_BT2020_YCC,
> +	COLORIMETRY_BT2020_CYCC,
> +	COLORIMETRY_DEFAULT,
> +};
> +
>  struct drm_mode_modeinfo {
>  	__u32 clock;
>  	__u16 hdisplay;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel