[Intel-gfx] [v2 1/2] drm: Add colorspace property

Jonas Karlman jonas at kwiboo.se
Fri Nov 2 14:29:57 UTC 2018


On 2018-11-02 15:13, Shankar, Uma wrote:
>
>> -----Original Message-----
>> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> Sent: Friday, November 2, 2018 5:00 PM
>> To: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Shankar, Uma <uma.shankar at intel.com>; dri-devel at lists.freedesktop.org;
>> intel-gfx at lists.freedesktop.org; Syrjala, Ville <ville.syrjala at intel.com>;
>> Lankhorst, Maarten <maarten.lankhorst at intel.com>; Hans Verkuil
>> <hansverk at cisco.com>
>> Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property
>>
>> On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
>>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>>>> This patch adds a 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.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>>>>  drivers/gpu/drm/drm_connector.c   | 44
>> +++++++++++++++++++++++++++++++++++++++
>>>>  include/drm/drm_connector.h       |  7 +++++++
>>>>  include/drm/drm_mode_config.h     |  6 ++++++
>>>>  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>>>>  5 files changed, 85 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index d5b7f31..9e1d46b 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->colorspace_property) {
>>>> +		state->colorspace = val;
>>>>  	} else if (property == connector->scaling_mode_property) {
>>>>  		state->scaling_mode = val;
>>>>  	} else if (property == connector->content_protection_property) {
>>>> @@ -795,6 +797,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->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 aa18b1d..5ad52a0 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -826,6 +826,38 @@ 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 colorspace[] = {
>>>> +	/* Standard Definition Colorimetry based on CEA 861 */
>>>> +	{ COLORIMETRY_ITU_601, "601" },
>>>> +	{ COLORIMETRY_ITU_709, "709" },
>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ COLORIMETRY_XV_YCC_601, "601" },
>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ COLORIMETRY_XV_YCC_709, "709" },
>>> 2 definitions that share the same name?
>>> All in all, I think the enum strings need to be more descriptive and self-
>> consistent.
>> +1
> Sure, will modify this.
>
>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>>> +	{ COLORIMETRY_S_YCC_601, "s601" },
>>>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
>>>> +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>> Hans (cc:d) recetly noted that these things aren't called Adobe<something>
>> anymore in the CTA-861 spec. There is some new name for them, which I now
>> forget.
> EC2 EC1 EC0 Extended Colorimetry
> 0       1      1      AdobeYCC601  
> This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks
> like they are still keeping it that way. Not sure if its updated as part of any latest spec
> update.

New names is opRGB and opYCC601 according to the notice on the first page of CTA-861-G [1]

Updated CTA-861-E/F/G can be found at https://standards.cta.tech/kwspub/published_docs/

[1] https://standards.cta.tech/kwspub/published_docs/CTA-861-G_FINAL_revised_2017.pdf

>  
>>>> +	/* Colorimetry based on IEC 61966-2-5 */
>>>> +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
>>>> +	/* 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" },
>>>> +	/* DP MSA Colorimetry */
>>>> +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
>>>> +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
>>>> +	{ COLORIMETRY_SRGB, "SRGB" },
>>> sRGB
> Was trying to avoid camelcase, but for these names, I guess we can keep how
> spec defines them. Will change this.
>
>>>> +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
>>>> +	{ COLORIMETRY_SCRGB, "SCRGB" },
>>> scRGB?
> Will update this.
>
>>>> +	{ COLORIMETRY_DCI_P3, "DCIP3" },
>>> DCI-P3?
> Ok, will update
>
>>>> +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
>>> ^Typo
> Yeah, will rectify this.
>
>>>> +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
>>>> +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
>>> This enum together with the code below is broken.
>>>
>>> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
>>>
>>> I would just make it COLORIMETRY_UNSET = "Unset".
>> "Unset" might work. Though feels a bit strange to me. Other ideas would be
>> "Default", "Automatic", "Undefined" or something along those lines.
>> Ideally it should convey the meaning of "the driver will pick this for you", and for
>> that I'd lean towards "Default" or "Automatic".
> Ok, will change this to Default.
>
>>> To set the default colorimetry for all drivers, just make the default
>>> value 0 (preferred), or add it to __drm_atomic_helper_connector_reset().
> Ok, will modify this and resend the next version.
>
> Thanks Ville and Maarten for your review and suggestions.
>
> Regards,
> Uma Shankar
>>>> +};
>>>> +
>>>>  /**
>>>>   * DOC: standard connector properties
>>>>   *
>>>> @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct
>> drm_display_info *info,
>>>>   *	can also expose this property to external outputs, in which case they
>>>>   *	must support "None", which should be the default (since external screens
>>>>   *	have a built-in scaler).
>>>> + *
>>>> + * 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.
>>>>   */
>>>>
>>>>  int drm_connector_create_standard_properties(struct drm_device
>>>> *dev) @@ -1020,6 +1058,12 @@ int
>> drm_connector_create_standard_properties(struct drm_device *dev)
>>>>  		return -ENOMEM;
>>>>  	dev->mode_config.non_desktop_property = prop;
>>>>
>>>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> "Colorspace",
>>>> +					colorspace, ARRAY_SIZE(colorspace));
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	dev->mode_config.colorspace_property = prop;
>>>> +
>>>>  	return 0;
>>>>  }
>>>>
>>>> diff --git a/include/drm/drm_connector.h
>>>> b/include/drm/drm_connector.h index dd0552c..b7f5373 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. This is most commonly used to switch to wider color
>>>> +	 * gamuts like BT2020.
>>>> +	 */
>>>> +	enum encoder_colorimetry colorspace;
>>>> +
>>>> +	/**
>>>>  	 * @writeback_job: Writeback job for writeback connectors
>>>>  	 *
>>>>  	 * Holds the framebuffer and out-fence for a writeback connector.
>>>> As diff --git a/include/drm/drm_mode_config.h
>>>> b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -863,6 +863,12 @@ struct drm_mode_config {
>>>>  	uint32_t cursor_width, cursor_height;
>>>>
>>>>  	/**
>>>> +	 * @colorspace_property: Connector property to set the suitable
>>>> +	 * colorspace supported by the sink.
>>>> +	 */
>>>> +	struct drm_property *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..831cc77 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -210,6 +210,30 @@
>>>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>>>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>>>
>>>> +enum encoder_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_ADOBE_YCC_601,
>>>> +	COLORIMETRY_ADOBE_RGB,
>>>> +	COLORIMETRY_BT2020_RGB,
>>>> +	COLORIMETRY_BT2020_YCC,
>>>> +	COLORIMETRY_BT2020_CYCC,
>>>> +	/* DP MSA Colorimetry Options */
>>>> +	COLORIMETRY_Y_CBCR_ITU_601,
>>>> +	COLORIMETRY_Y_CBCR_ITU_709,
>>>> +	COLORIMETRY_SRGB,
>>>> +	COLORIMETRY_RGB_WIDE_GAMUT,
>>>> +	COLORIMETRY_SCRGB,
>>>> +	COLORIMETRY_DCI_P3,
>>>> +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
>>>> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, };
>>>> +
>>>>  struct drm_mode_modeinfo {
>>>>  	__u32 clock;
>>>>  	__u16 hdisplay;
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&reserved=0
>> --
>> Ville Syrjälä
>> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&reserved=0


More information about the Intel-gfx mailing list