[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