Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
On Thu, Sep 27, 2018 at 04:29:52AM +, Shankar, Uma wrote:
>
>
> >-Original Message-
> >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> >Sent: Wednesday, September 26, 2018 3:12 PM
> >To: Maarten Lankhorst
> >Cc: Shankar, Uma ; Adam Jackson
> >; intel-gfx@lists.freedesktop.org; dri-
> >de...@lists.freedesktop.org; Syrjala, Ville ;
> >Lankhorst,
> >Maarten
> >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:a...@redhat.com]
> >> >> Sent: Wednesday, August 1, 2018 1:24 AM
> >> >> To: Shankar, Uma ;
> >> >> intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
> >> >> Cc: Syrjala, Ville ; Lankhorst, Maarten
> >> >>
> >> >> 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.
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
>-Original Message-
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Wednesday, September 26, 2018 3:12 PM
>To: Maarten Lankhorst
>Cc: Shankar, Uma ; Adam Jackson
>; intel-gfx@lists.freedesktop.org; dri-
>de...@lists.freedesktop.org; Syrjala, Ville ;
>Lankhorst,
>Maarten
>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:a...@redhat.com]
>> >> Sent: Wednesday, August 1, 2018 1:24 AM
>> >> To: Shankar, Uma ;
>> >> intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
>> >> Cc: Syrjala, Ville ; Lankhorst, Maarten
>> >>
>> >> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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:a...@redhat.com]
> >> Sent: Wednesday, August 1, 2018 1:24 AM
> >> To: Shankar, Uma ; intel-gfx@lists.freedesktop.org;
> >> dri-de...@lists.freedesktop.org
> >> Cc: Syrjala, Ville ; Lankhorst, Maarten
> >>
> >> 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.
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
Op 01-08-18 om 16:01 schreef Shankar, Uma:
>
>> -Original Message-
>> From: Adam Jackson [mailto:a...@redhat.com]
>> Sent: Wednesday, August 1, 2018 1:24 AM
>> To: Shankar, Uma ; intel-gfx@lists.freedesktop.org;
>> dri-de...@lists.freedesktop.org
>> Cc: Syrjala, Ville ; Lankhorst, Maarten
>>
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
>-Original Message-
>From: Adam Jackson [mailto:a...@redhat.com]
>Sent: Wednesday, August 1, 2018 1:24 AM
>To: Shankar, Uma ; intel-gfx@lists.freedesktop.org;
>dri-de...@lists.freedesktop.org
>Cc: Syrjala, Ville ; Lankhorst, Maarten
>
>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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [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.
Is this enum simply built to match the values you're injecting into the
InfoFrame? Would we need a different enum for DisplayPort?
- ajax
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC 1/3] drm: Add colorspace property
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
---
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(+)
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
