| Message ID | 1532447152-23736-2-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Colorspace connector property interface | expand |
On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -209,6 +209,17 @@ > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > > +enum extended_colorimetry { > + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, > + EXTENDED_COLORIMETRY_XV_YCC_709, > + EXTENDED_COLORIMETRY_S_YCC_601, > + EXTENDED_COLORIMETRY_ADOBE_YCC_601, > + EXTENDED_COLORIMETRY_ADOBE_RGB, > + EXTENDED_COLORIMETRY_BT2020_RGB, > + EXTENDED_COLORIMETRY_BT2020_YCC, > + EXTENDED_COLORIMETRY_BT2020_CYCC, > +}; This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure I like. Is this enum simply built to match the values you're injecting into the InfoFrame? Would we need a different enum for DisplayPort? - ajax
>-----Original Message----- >From: Adam Jackson [mailto:ajax@redhat.com] >Sent: Wednesday, August 1, 2018 1:24 AM >To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >dri-devel@lists.freedesktop.org >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten ><maarten.lankhorst@intel.com> >Subject: Re: [RFC 1/3] drm: Add colorspace property > >On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: > >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -209,6 +209,17 @@ >> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >> >> +enum extended_colorimetry { >> + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, >> + EXTENDED_COLORIMETRY_XV_YCC_709, >> + EXTENDED_COLORIMETRY_S_YCC_601, >> + EXTENDED_COLORIMETRY_ADOBE_YCC_601, >> + EXTENDED_COLORIMETRY_ADOBE_RGB, >> + EXTENDED_COLORIMETRY_BT2020_RGB, >> + EXTENDED_COLORIMETRY_BT2020_YCC, >> + EXTENDED_COLORIMETRY_BT2020_CYCC, >> +}; > >This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure >I like. This enum gives a list of all possible colorspace which can be set on the sink device. The compositors/userspace can choose one of them, based on the capabilities of sink as well as based on rendering/blending policies which are designed to take advantage of hardware resources available. If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this enum list. > >Is this enum simply built to match the values you're injecting into the InfoFrame? Yes they are as per HDMI SPEC defined Infoframe. This can directly be assigned to create the equivalent infoframe. >Would we need a different enum for DisplayPort? DP will define a SDP packet to pass this info. From userspace, we can still pass this enum value, as part of SDP packet creation DP driver can pick equivalent value as per DP spec (which may be different from this enum value). But driver will still know as to what colorspace is requested by userspace. > >- ajax
Op 01-08-18 om 16:01 schreef Shankar, Uma: > >> -----Original Message----- >> From: Adam Jackson [mailto:ajax@redhat.com] >> Sent: Wednesday, August 1, 2018 1:24 AM >> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >> dri-devel@lists.freedesktop.org >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> <maarten.lankhorst@intel.com> >> Subject: Re: [RFC 1/3] drm: Add colorspace property >> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: >> >>> --- a/include/uapi/drm/drm_mode.h >>> +++ b/include/uapi/drm/drm_mode.h >>> @@ -209,6 +209,17 @@ >>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >>> >>> +enum extended_colorimetry { >>> + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, >>> + EXTENDED_COLORIMETRY_XV_YCC_709, >>> + EXTENDED_COLORIMETRY_S_YCC_601, >>> + EXTENDED_COLORIMETRY_ADOBE_YCC_601, >>> + EXTENDED_COLORIMETRY_ADOBE_RGB, >>> + EXTENDED_COLORIMETRY_BT2020_RGB, >>> + EXTENDED_COLORIMETRY_BT2020_YCC, >>> + EXTENDED_COLORIMETRY_BT2020_CYCC, >>> +}; >> This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure >> I like. > This enum gives a list of all possible colorspace which can be set on the sink device. > The compositors/userspace can choose one of them, based on the capabilities of sink > as well as based on rendering/blending policies which are designed to take advantage > of hardware resources available. > > If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this > enum list. I would add a default, but not sure I would introduce a new enum. hdmi_extended_colorimetry is already available. I would argue to keep it as it is, Or really keep it as it is, since we have to set it to *something* when sending the infoframe. And we currently send this value. Hm maybe for DP it could be useful, if it's optional to put it in the SDP packet there. >> Is this enum simply built to match the values you're injecting into the InfoFrame? > Yes they are as per HDMI SPEC defined Infoframe. This can directly be assigned to create > the equivalent infoframe. Yes, in fact I would take the one from hdmi.h instead of redefining it. :) We already do for the other props. >> Would we need a different enum for DisplayPort? > DP will define a SDP packet to pass this info. From userspace, we can still pass this > enum value, as part of SDP packet creation DP driver can pick equivalent value as per DP spec > (which may be different from this enum value). But driver will still know as to what > colorspace is requested by userspace. Sounds good. :) ~Maarten
On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote: > Op 01-08-18 om 16:01 schreef Shankar, Uma: > > > >> -----Original Message----- > >> From: Adam Jackson [mailto:ajax@redhat.com] > >> Sent: Wednesday, August 1, 2018 1:24 AM > >> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; > >> dri-devel@lists.freedesktop.org > >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > >> <maarten.lankhorst@intel.com> > >> Subject: Re: [RFC 1/3] drm: Add colorspace property > >> > >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: > >> > >>> --- a/include/uapi/drm/drm_mode.h > >>> +++ b/include/uapi/drm/drm_mode.h > >>> @@ -209,6 +209,17 @@ > >>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > >>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > >>> > >>> +enum extended_colorimetry { > >>> + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, > >>> + EXTENDED_COLORIMETRY_XV_YCC_709, > >>> + EXTENDED_COLORIMETRY_S_YCC_601, > >>> + EXTENDED_COLORIMETRY_ADOBE_YCC_601, > >>> + EXTENDED_COLORIMETRY_ADOBE_RGB, > >>> + EXTENDED_COLORIMETRY_BT2020_RGB, > >>> + EXTENDED_COLORIMETRY_BT2020_YCC, > >>> + EXTENDED_COLORIMETRY_BT2020_CYCC, > >>> +}; > >> This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure > >> I like. > > This enum gives a list of all possible colorspace which can be set on the sink device. > > The compositors/userspace can choose one of them, based on the capabilities of sink > > as well as based on rendering/blending policies which are designed to take advantage > > of hardware resources available. > > > > If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this > > enum list. > I would add a default, but not sure I would introduce a new enum. > hdmi_extended_colorimetry is already available. Yeah, there should be a default entry for "driver automagically picks something suitable". I think the enum prop should also be some kind of superset of all CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP colorimetry options. The current list seems a bit incomplete to me.
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Wednesday, September 26, 2018 3:12 PM >To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >Cc: Shankar, Uma <uma.shankar@intel.com>; Adam Jackson ><ajax@redhat.com>; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, >Maarten <maarten.lankhorst@intel.com> >Subject: Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property > >On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote: >> Op 01-08-18 om 16:01 schreef Shankar, Uma: >> > >> >> -----Original Message----- >> >> From: Adam Jackson [mailto:ajax@redhat.com] >> >> Sent: Wednesday, August 1, 2018 1:24 AM >> >> To: Shankar, Uma <uma.shankar@intel.com>; >> >> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> >> <maarten.lankhorst@intel.com> >> >> Subject: Re: [RFC 1/3] drm: Add colorspace property >> >> >> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: >> >> >> >>> --- a/include/uapi/drm/drm_mode.h >> >>> +++ b/include/uapi/drm/drm_mode.h >> >>> @@ -209,6 +209,17 @@ >> >>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >> >>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >> >>> >> >>> +enum extended_colorimetry { >> >>> + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, >> >>> + EXTENDED_COLORIMETRY_XV_YCC_709, >> >>> + EXTENDED_COLORIMETRY_S_YCC_601, >> >>> + EXTENDED_COLORIMETRY_ADOBE_YCC_601, >> >>> + EXTENDED_COLORIMETRY_ADOBE_RGB, >> >>> + EXTENDED_COLORIMETRY_BT2020_RGB, >> >>> + EXTENDED_COLORIMETRY_BT2020_YCC, >> >>> + EXTENDED_COLORIMETRY_BT2020_CYCC, }; >> >> This doesn't give any way to distinguish "not set" from BT.601, >> >> which I'm not sure I like. >> > This enum gives a list of all possible colorspace which can be set on the sink >device. >> > The compositors/userspace can choose one of them, based on the >> > capabilities of sink as well as based on rendering/blending policies >> > which are designed to take advantage of hardware resources available. >> > >> > If you suggest to add something like NO_COLORSPACE_SET = -1, I can >> > add that to this enum list. >> I would add a default, but not sure I would introduce a new enum. >> hdmi_extended_colorimetry is already available. > >Yeah, there should be a default entry for "driver automagically picks something >suitable". > Ok got it, will add a default option to the list. >I think the enum prop should also be some kind of superset of all >CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP colorimetry >options. The current list seems a bit incomplete to me. > So should I keep this enum and append DP MSA +VSC SDP options to the list ? We can then have encoder specific enums separate from this global colorpsace enum. >-- >Ville Syrjälä >Intel
On Thu, Sep 27, 2018 at 04:29:52AM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Wednesday, September 26, 2018 3:12 PM > >To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >Cc: Shankar, Uma <uma.shankar@intel.com>; Adam Jackson > ><ajax@redhat.com>; intel-gfx@lists.freedesktop.org; dri- > >devel@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, > >Maarten <maarten.lankhorst@intel.com> > >Subject: Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property > > > >On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote: > >> Op 01-08-18 om 16:01 schreef Shankar, Uma: > >> > > >> >> -----Original Message----- > >> >> From: Adam Jackson [mailto:ajax@redhat.com] > >> >> Sent: Wednesday, August 1, 2018 1:24 AM > >> >> To: Shankar, Uma <uma.shankar@intel.com>; > >> >> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > >> >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten > >> >> <maarten.lankhorst@intel.com> > >> >> Subject: Re: [RFC 1/3] drm: Add colorspace property > >> >> > >> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote: > >> >> > >> >>> --- a/include/uapi/drm/drm_mode.h > >> >>> +++ b/include/uapi/drm/drm_mode.h > >> >>> @@ -209,6 +209,17 @@ > >> >>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > >> >>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > >> >>> > >> >>> +enum extended_colorimetry { > >> >>> + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, > >> >>> + EXTENDED_COLORIMETRY_XV_YCC_709, > >> >>> + EXTENDED_COLORIMETRY_S_YCC_601, > >> >>> + EXTENDED_COLORIMETRY_ADOBE_YCC_601, > >> >>> + EXTENDED_COLORIMETRY_ADOBE_RGB, > >> >>> + EXTENDED_COLORIMETRY_BT2020_RGB, > >> >>> + EXTENDED_COLORIMETRY_BT2020_YCC, > >> >>> + EXTENDED_COLORIMETRY_BT2020_CYCC, }; > >> >> This doesn't give any way to distinguish "not set" from BT.601, > >> >> which I'm not sure I like. > >> > This enum gives a list of all possible colorspace which can be set on the sink > >device. > >> > The compositors/userspace can choose one of them, based on the > >> > capabilities of sink as well as based on rendering/blending policies > >> > which are designed to take advantage of hardware resources available. > >> > > >> > If you suggest to add something like NO_COLORSPACE_SET = -1, I can > >> > add that to this enum list. > >> I would add a default, but not sure I would introduce a new enum. > >> hdmi_extended_colorimetry is already available. > > > >Yeah, there should be a default entry for "driver automagically picks something > >suitable". > > > > Ok got it, will add a default option to the list. > > >I think the enum prop should also be some kind of superset of all > >CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP colorimetry > >options. The current list seems a bit incomplete to me. > > > > So should I keep this enum and append DP MSA +VSC SDP options to the list ? > We can then have encoder specific enums separate from this global colorpsace > enum. My original idea was one enum and each encoder type can then pick the ones it can support by passing in the appropriate bitmask. That's assuming we can reconcile any differences between DP and HDMI in a nice way.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e..f065e6f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1397,6 +1397,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) { @@ -1502,6 +1504,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, *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 5ada064..cfe1d79 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -805,6 +805,25 @@ 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 IEC 61966-2-4 */ + { EXTENDED_COLORIMETRY_XV_YCC_601, "601" }, + /* High Definition Colorimetry based on IEC 61966-2-4 */ + { EXTENDED_COLORIMETRY_XV_YCC_709, "709" }, + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ + { EXTENDED_COLORIMETRY_S_YCC_601, "s601" }, + /* Colorimetry based on IEC 61966-2-5 [33] */ + { EXTENDED_COLORIMETRY_ADOBE_YCC_601, "adobe601" }, + /* Colorimetry based on IEC 61966-2-5 */ + { EXTENDED_COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, + /* Colorimetry based on ITU-R BT.2020 */ + { EXTENDED_COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, + /* Colorimetry based on ITU-R BT.2020 */ + { EXTENDED_COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, + /* Colorimetry based on ITU-R BT.2020 */ + { EXTENDED_COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, +}; + /** * DOC: standard connector properties * @@ -951,6 +970,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) @@ -999,6 +1024,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 a5179eb..306b536 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -443,6 +443,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 extended_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 a0b202e..3afe30b 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -841,6 +841,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 8d67243..c9d14ca 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -209,6 +209,17 @@ #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 +enum extended_colorimetry { + EXTENDED_COLORIMETRY_XV_YCC_601 = 0, + EXTENDED_COLORIMETRY_XV_YCC_709, + EXTENDED_COLORIMETRY_S_YCC_601, + EXTENDED_COLORIMETRY_ADOBE_YCC_601, + EXTENDED_COLORIMETRY_ADOBE_RGB, + EXTENDED_COLORIMETRY_BT2020_RGB, + EXTENDED_COLORIMETRY_BT2020_YCC, + EXTENDED_COLORIMETRY_BT2020_CYCC, +}; + struct drm_mode_modeinfo { __u32 clock; __u16 hdisplay;
This patch adds a colorspace property, enabling userspace to switch to various supported colorspaces. This will help enable BT2020 along with other colorspaces. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/drm_atomic.c | 4 ++++ drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 7 +++++++ include/drm/drm_mode_config.h | 6 ++++++ include/uapi/drm/drm_mode.h | 11 +++++++++++ 5 files changed, 59 insertions(+)