[v5 1/2] drm: Add colorspace connector property

Sharma, Shashank shashank.sharma at intel.com
Thu Dec 20 11:57:47 UTC 2018


Regards

Shashank


On 12/11/2018 11:44 PM, Uma Shankar wrote:
> This patch adds a colorspace connector 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.
>
> v4: Addressed Chris and Ville's review comments, and created a
> common colorspace property for DP and HDMI, filtered the list
> based on the colorspaces supported by the respective protocol
> standard.
>
> v5: Made the property creation helper accept enum list based on
> platform capabilties as suggested by Shashank. Consolidated HDMI
> and DP property creation in the common helper.
>
> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
>   drivers/gpu/drm/drm_connector.c   | 82 +++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_connector.h       | 17 ++++++++
>   include/uapi/drm/drm_mode.h       | 33 ++++++++++++++++
>   4 files changed, 136 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 86ac339..9df7520 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -729,6 +729,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   			return -EINVAL;
>   		}
>   		state->content_protection = val;
> +	} else if (property == connector->colorspace_property) {
> +		state->colorspace = val;
>   	} else if (property == config->writeback_fb_id_property) {
>   		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
>   		int ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
> @@ -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 == connector->colorspace_property) {
> +		*val = state->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..46928f7 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -826,6 +826,54 @@ 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)
>   
> +/* List of HDMI Colorspaces supported */
> +static const struct drm_prop_enum_list hdmi_colorspace[] = {
Should we call this list hdmi_colorspaces :) ?
> +	/* For Default case, driver will set the colorspace */
> +	{ COLORIMETRY_DEFAULT, "Default" },
> +	/* 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" },
> +};
> +
> +/* List of DP Colorspaces supported */
Same here for dp_colorspaces ?
> +static const struct drm_prop_enum_list dp_colorspace[] = {
> +	/* For Default case, driver will set the colorspace */
> +	{ COLORIMETRY_DEFAULT, "Default" },
> +	/* 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-5 */
> +	{ COLORIMETRY_OPRGB, "opRGB" },
> +	/* DP MSA Colorimetry */
> +	{ DP_COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
> +	{ DP_COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
> +	{ DP_COLORIMETRY_SRGB, "sRGB" },
> +	{ DP_COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
> +	{ DP_COLORIMETRY_SCRGB, "scRGB" },
> +	{ DP_COLORIMETRY_DCI_P3, "DCI-P3" },
> +	{ DP_COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Profile" },
> +};
> +
>   /**
>    * DOC: standard connector properties
>    *
> @@ -1402,6 +1450,40 @@ 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.
I might be wrong, but is this alignment correct as per the property 
documentation style ?
> + * @connector: connector to set property on.
> + *
> + * 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,
> +					const struct drm_prop_enum_list *props,
> +					int num_values)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +					"Colorspace",
> +					props, num_values);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	connector->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 665b9ca..fa840db 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -31,6 +31,7 @@
>   #include <drm/drm_util.h>
>   
>   #include <uapi/drm/drm_mode.h>
> +#include <drm/drm_property.h>
>   
>   struct drm_connector_helper_funcs;
>   struct drm_modeset_acquire_ctx;
> @@ -497,6 +498,13 @@ struct drm_connector_state {
>   	unsigned int content_protection;
>   
>   	/**
> +	 * @colorspace: Connector property to request colorspace
Please fix this documentation, This is not a property, this is just a 
state variable to hold the
requested property value until next atomic commit.
> +	 * change on Sink. This is most commonly used to switch to
> +	 * wider color gamuts like BT2020.
> +	 */
> +	enum absolute_colorimetry_list colorspace;
> +
> +	/**
>   	 * @writeback_job: Writeback job for writeback connectors
>   	 *
>   	 * Holds the framebuffer and out-fence for a writeback connector. As
> @@ -978,6 +986,12 @@ struct drm_connector {
>   	struct drm_property *content_protection_property;
>   
>   	/**
> +	 * @colorspace_property: Connector property to set the suitable
> +	 * colorspace supported by the sink.
> +	 */
> +	struct drm_property *colorspace_property;
Yep, this is the actual property :)
> +
> +	/**
>   	 * @path_blob_ptr:
>   	 *
>   	 * DRM blob property data for the DP MST path property. This should only
> @@ -1248,6 +1262,9 @@ 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,
> +					const struct drm_prop_enum_list *props,
> +					int num_values);
>   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/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe3..42efab8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -210,6 +210,39 @@
>   #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>   #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>   
> +/*
> + * This is a consolidated colorimetry list supported by HDMI and
> + * DP protocol standard. The respective connectors will register
> + * a property with the subset of this list (supported by that
> + * respective protocol). Userspace will set the colorspace through
> + * a colorspace property which will be created and exposed to
> + * userspace.
> + */
> +enum absolute_colorimetry_list {
> +	/* For Default case, driver will set the colorspace */
> +	COLORIMETRY_DEFAULT = 0,
> +	/* CEA 861 Normal Colorimetry options */
> +	COLORIMETRY_ITU_601,
> +	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,
> +	/* DP MSA Colorimetry Options */
> +	DP_COLORIMETRY_Y_CBCR_ITU_601,
> +	DP_COLORIMETRY_Y_CBCR_ITU_709,
> +	DP_COLORIMETRY_SRGB,
> +	DP_COLORIMETRY_RGB_WIDE_GAMUT,
> +	DP_COLORIMETRY_SCRGB,
> +	DP_COLORIMETRY_DCI_P3,
> +	DP_COLORIMETRY_CUSTOM_COLOR_PROFILE,
> +};
> +
>   struct drm_mode_modeinfo {
>   	__u32 clock;
>   	__u16 hdisplay;
With given minor comments dealt with, please feel free to use:
Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>



More information about the dri-devel mailing list