RE: [v4 1/3] drm: Add HDMI colorspace property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Sharma, Shashank
>Sent: Wednesday, November 28, 2018 9:41 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
>dri-devel@xxxxxxxxxxxxxxxxxxxxx
>Cc: Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville
><ville.syrjala@xxxxxxxxx>; jonas@xxxxxxxxx; hansverk@xxxxxxxxx;
>Brian.Starkey@xxxxxxx
>Subject: Re: [v4 1/3] drm: Add HDMI colorspace property
>
>Regards
>
>Shashank
>
>
>On 11/27/2018 10:10 PM, 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.
>>
>> 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c |  4 +++
>>   drivers/gpu/drm/drm_connector.c   | 61
>+++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       | 14 +++++++++
>>   include/uapi/drm/drm_mode.h       | 33 +++++++++++++++++++++
>>   4 files changed, 112 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..57d36e4 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[] = {
>> +	/* 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" },
>This still doesn't help us with the platform specific colorspace support, like:
>Even for HDMI, Until GEN10, we don't want to add BT2020 colorspace enum
>values for Intel driver, which would be difficult to know for a user.
>
>How about we modify the design a drm helper bit like this:
>1. Keep the absolute_colorimetry_list as the full range of colorimetry enum
>values supported by DRM for HDMI display 2. Make the DRM helper accept a
>series/subset of these enum values, which will be passed from the core driver
>(like I915), and the DRM functions creates property with these passed values.
>3. Call the DRM helper function from functions core driver function (like
>intel_ddi_init()) from where we already know:
>     - Which display to select enum values from (HDMI/DP)
>     - Which enum values to pick based on current platform (Like if GEN < GEN9,
>add dont add REC_2020 etc)

This sounds good, thanks Shashank. I can develop along these lines. The sink capability will still be on
userspace, but atleast the platform capabilities can be taken care inside driver itself.
I hope this approach is ok with community ?

Regards,
Uma Shankar

>- Shashank
>> +};
>> +
>>   /**
>>    * DOC: standard connector properties
>>    *
>> @@ -1402,6 +1426,43 @@ 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.
>> + * @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) {
>> +	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) {
>> +
>> +		prop = drm_property_create_enum(dev,
>DRM_MODE_PROP_ENUM,
>> +						"Colorspace",
>> +						hdmi_colorspace,
>> +						ARRAY_SIZE(hdmi_colorspace));
>> +		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..e98efb1 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;
>>
>>   	/**
>> +	 * @colorspace: Connector property to request colorspace
>> +	 * 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 +985,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;
>> +
>> +	/**
>>   	 * @path_blob_ptr:
>>   	 *
>>   	 * DRM blob property data for the DP MST path property. This should
>> only @@ -1248,6 +1261,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/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;

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux