[Intel-gfx] [v2 1/2] drm: Add colorspace property
Shankar, Uma
uma.shankar at intel.com
Fri Nov 2 14:13:29 UTC 2018
>-----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.
>> > + /* 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://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
More information about the Intel-gfx
mailing list