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

[Hans Verkuil]

On 11/02/2018 03:29 PM, Jonas Karlman wrote:
> 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

Correct. The names were updated since Adobe complained to the CTA about 
trademark issues. And in all fairness, it makes sense to refer to the 
official international standard name instead of a company standard, even
though they are effectively identical.

Basically, just avoid the name 'Adobe' :-)

Regards,

	Hans