>-----Original Message-----
>From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
>Ville Syrjälä
>Sent: Monday, February 4, 2019 10:54 PM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: intel-...@lists.freedesktop.org; Syrjala, Ville <ville.syrj...@intel.com>;
>dri-
>de...@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankho...@intel.com>
>Subject: Re: [Intel-gfx] [v10 1/3] drm: Add HDMI colorspace property
>
>On Mon, Feb 04, 2019 at 05:08:40PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>> >Sent: Monday, February 4, 2019 8:55 PM
>> >To: Shankar, Uma <uma.shan...@intel.com>
>> >Cc: intel-...@lists.freedesktop.org; Syrjala, Ville
>> ><ville.syrj...@intel.com>; Lankhorst, Maarten
>> ><maarten.lankho...@intel.com>; dri- de...@lists.freedesktop.org
>> >Subject: Re: [Intel-gfx] [v10 1/3] drm: Add HDMI colorspace property
>> >
>> >On Fri, Feb 01, 2019 at 08:50:11PM +0200, Ville Syrjälä wrote:
>> >> On Wed, Jan 30, 2019 at 06:24:24PM +0530, Uma Shankar wrote:
>> >> > Create a new connector property to program colorspace to sink
>> >> > devices. Modern sink devices support more than 1 type of
>> >> > colorspace like 601, 709, BT2020 etc. This helps to switch based
>> >> > on content type which is to be displayed. The decision lies with
>> >> > compositors as to in which scenarios, a particular colorspace will be
>picked.
>> >> >
>> >> > This will be helpful mostly to switch to higher gamut colorspaces
>> >> > like BT2020 when the media content is encoded as BT2020. Thereby
>> >> > giving a good visual experience to users.
>> >> >
>> >> > The expectation from userspace is that it should parse the EDID
>> >> > and get supported colorspaces. Use this property and switch to
>> >> > the one supported. Sink supported colorspaces should be retrieved
>> >> > by userspace from EDID and driver will not explicitly expose them.
>> >> >
>> >> > Basically the expectation from userspace is:
>> >> > - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>> >> > colorspace
>> >> > - Set this new property to let the sink know what it
>> >> > converted the CRTC output to.
>> >> >
>> >> > 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.
>> >> >
>> >> > v3: Removed Adobe references from enum definitions as per Ville,
>> >> > Hans Verkuil and Jonas Karlman suggestions. Changed Default to an
>> >> > unset state where driver will assign the colorspace is not chosen
>> >> > by user, suggested by Ville and Maarten. Addressed other misc
>> >> > review comments from Maarten. Split the changes to have separate
>> >> > colorspace property for DP and HDMI.
>> >> >
>> >> > v4: Addressed Chris and Ville's review comments, and created a
>> >> > common colorspace property for DP and HDMI, filtered the list
>> >> > based on the colorspaces supported by the respective protocol standard.
>> >> >
>> >> > v5: Made the property creation helper accept enum list based on
>> >> > platform capabilties as suggested by Shashank. Consolidated HDMI
>> >> > and DP property creation in the common helper.
>> >> >
>> >> > v6: Addressed Shashank's review comments.
>> >> >
>> >> > v7: Added defines instead of enum in uapi as per Brian Starkey's
>> >> > suggestion in order to go with string matching at userspace.
>> >> > Updated the commit message to add more details as well kernel docs.
>> >> >
>> >> > v8: Addressed Maarten's review comments.
>> >> >
>> >> > v9: Removed macro defines from uapi as per Brian Starkey and
>> >> > Daniel Stone's comments and moved to drm include file. Moved back
>> >> > to older design with exposing all HDMI colorspaces to userspace
>> >> > since infoframe capability is there even on legacy platforms, as
>> >> > per Ville's review comments.
>> >> >
>> >> > v10: Fixed sparse warnings, updated the RB from Maarten and Jani's ack.
>> >> >
>> >> > Signed-off-by: Uma Shankar <uma.shan...@intel.com>
>> >> > Acked-by: Jani Nikula <jani.nik...@intel.com>
>> >> > Reviewed-by: Shashank Sharma <shashank.sha...@intel.com>
>> >> > Reviewed-by: Maarten Lankhorst
>> >> > <maarten.lankho...@linux.intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
>> >> > drivers/gpu/drm/drm_connector.c | 75
>> >+++++++++++++++++++++++++++++++++++++++
>> >> > include/drm/drm_connector.h | 46 ++++++++++++++++++++++++
>> >> > 3 files changed, 125 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> >> > b/drivers/gpu/drm/drm_atomic_uapi.c
>> >> > index 9a1f41a..9b5d44f 100644
>> >> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> >> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> >> > @@ -746,6 +746,8 @@ static int
>> >> > drm_atomic_connector_set_property(struct
>> >drm_connector *connector,
>> >> > return -EINVAL;
>> >> > }
>> >> > state->content_protection = val;
>> >> > + } else if (property == connector->colorspace_property) {
>> >> > + state->colorspace = val;
>> >> > } else if (property == config->writeback_fb_id_property) {
>> >> > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev,
>> >NULL, val);
>> >> > int ret =
>> >> > drm_atomic_set_writeback_fb_for_connector(state,
>> >fb);
>> >> > @@ -814,6 +816,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 == connector->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 8475396..ed10dd9 100644
>> >> > --- a/drivers/gpu/drm/drm_connector.c
>> >> > +++ b/drivers/gpu/drm/drm_connector.c
>> >> > @@ -826,6 +826,30 @@ 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 hdmi_colorspaces[] = {
>> >> > + /* For Default case, driver will set the colorspace */
>> >> > + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
>> >> > + /* Standard Definition Colorimetry based on CEA 861 */
>> >> > + { DRM_MODE_COLORIMETRY_ITU_601, "ITU_601" },
>> >>
>> >> The spec no longer has the BT.601 note here. Which I guess makes
>> >> sense since BT.601 didn't even specify any color primaries
>> >> initially. Later versions do by they have distinct primaries for
>> >> NTSC vs. PAL. The spec calls this just SMPTE 170M now, so I think we
>> >> should
>do the same.
>>
>> Ok Sure, Will update this name. Yeah this was a little confusing as to
>> what name to go with. Will take the one defined now in spec i.e., SMPTE 170M.
>>
>> >>
>> >> > + { DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>> >> > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> >> > + { DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> >> > + /* High Definition Colorimetry based on IEC 61966-2-4 */
>> >> > + { DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> >> > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> >> > + { DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> >> > + /* Colorimetry based on IEC 61966-2-5 [33] */
>> >> > + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>> >>
>> >> The spelling is rather inconsistent here. XV_ vs. S_ vs. op. Why
>> >> not use the same spelling style for all?
>>
>> This is the names defined in spec and I used as is. I will make them
>> as XVYCC, SYCC and OPYCC for consistency. Hope this is ok ?
>>
>> >>
>> >> > + /* Colorimetry based on IEC 61966-2-5 */
>> >> > + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>> >> > + /* Colorimetry based on ITU-R BT.2020 */
>> >> > + { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> >> > + /* Colorimetry based on ITU-R BT.2020 */
>> >> > + { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> >> > + /* Colorimetry based on ITU-R BT.2020 */
>> >> > + { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>> >>
>> >> "BT2020" vs. "ITU_709" is rather inconsistent as well.
>>
>> Ok, will update this.
>>
>> >> We also seem to be missing the two DCI-P3 things here.
>>
>> Yeah, I initially did as per 861.F where it was not there. This got
>> added in 861.G, will add this.
>>
>> >> > +};
>> >> > +
>> >> > /**
>> >> > * DOC: standard connector properties
>> >> > *
>> >> > @@ -1548,6 +1572,57 @@ int
>> >> > drm_mode_create_aspect_ratio_property(struct drm_device *dev)
>> >> > EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>> >> >
>> >> > /**
>> >> > + * DOC: standard connector properties
>> >> > + *
>> >> > + * Colorspace:
>> >> > + * drm_mode_create_colorspace_property - create colorspace
>property
>> >> > + * 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.
>> >> > Thereby
>> >> > + * giving a good visual experience to users.
>> >> > + *
>> >> > + * The expectation from userspace is that it should parse the EDID
>> >> > + * and get supported colorspaces. Use this property and switch to
>> >> > the
>> >> > + * one supported. Sink supported colorspaces should be retrieved by
>> >> > + * userspace from EDID and driver will not explicitly expose them.
>> >> > + *
>> >> > + * Basically the expectation from userspace is:
>> >> > + * - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink
>> >> > + * colorspace
>> >> > + * - Set this new property to let the sink know what it
>> >> > + * converted the CRTC output to.
>> >> > + * - This property is just to inform sink what colorspace
>> >> > + * source is trying to drive.
>> >> > + *
>> >> > + * Called by a driver the first time it's needed, must be
>> >> > +attached to desired
>> >> > + * connectors.
>> >> > + */
>> >> > +int drm_mode_create_colorspace_property(struct drm_connector
>> >> > +*connector) {
>> >> > + struct drm_device *dev = connector->dev;
>> >> > + struct drm_property *prop;
>> >> > +
>> >> > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
>> >> > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) {
>> >> > + prop = drm_property_create_enum(dev,
>> >DRM_MODE_PROP_ENUM,
>> >> > + "Colorspace",
>> >> > + hdmi_colorspaces,
>> >> > +
>> > ARRAY_SIZE(hdmi_colorspaces));
>> >> > + if (!prop)
>> >> > + return -ENOMEM;
>> >> > + } else {
>> >> > + DRM_DEBUG_KMS("Colorspace property not supported\n");
>> >> > + return 0;
>> >> > + }
>> >> > +
>> >> > + connector->colorspace_property = prop;
>> >> > +
>> >> > + return 0;
>> >> > +}
>> >> > +EXPORT_SYMBOL(drm_mode_create_colorspace_property);
>> >> > +
>> >> > +/**
>> >> > * drm_mode_create_content_type_property - create content type
>property
>> >> > * @dev: DRM device
>> >> > *
>> >> > diff --git a/include/drm/drm_connector.h
>> >> > b/include/drm/drm_connector.h index 9941613..29495b3 100644
>> >> > --- a/include/drm/drm_connector.h
>> >> > +++ b/include/drm/drm_connector.h
>> >> > @@ -253,6 +253,38 @@ enum drm_panel_orientation {
>> >> > DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>> >> > };
>> >> >
>> >> > +/*
>> >> > + * This is a consolidated colorimetry list supported by HDMI and
>> >> > + * DP protocol standard. The respective connectors will register
>> >> > + * a property with the subset of this list (supported by that
>> >> > + * respective protocol). Userspace will set the colorspace
>> >> > +through
>> >> > + * a colorspace property which will be created and exposed to
>> >> > + * userspace.
>> >> > + */
>> >> > +
>> >> > +/* For Default case, driver will set the colorspace */
>> >> > +#define DRM_MODE_COLORIMETRY_DEFAULT 0
>> >> > +/* CEA 861 Normal Colorimetry options */
>> >> > +#define DRM_MODE_COLORIMETRY_ITU_601 1
>> >> > +#define DRM_MODE_COLORIMETRY_ITU_709 2
>> >> > +/* CEA 861 Extended Colorimetry Options */
>> >> > +#define DRM_MODE_COLORIMETRY_XV_YCC_601
> 3
>> >> > +#define DRM_MODE_COLORIMETRY_XV_YCC_709
> 4
>> >> > +#define DRM_MODE_COLORIMETRY_S_YCC_601 5
>> >> > +#define DRM_MODE_COLORIMETRY_OPYCC_601
> 6
>> >> > +#define DRM_MODE_COLORIMETRY_OPRGB 7
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_RGB
> 8
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_YCC
> 9
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_CYCC 10
>> >> > +/* DP MSA Colorimetry Options */
>> >> > +#define DRM_MODE_DP_COLORIMETRY_Y_CBCR_ITU_601
> 11
>> >> > +#define DRM_MODE_DP_COLORIMETRY_Y_CBCR_ITU_709
> 12
>> >> > +#define DRM_MODE_DP_COLORIMETRY_SRGB 13
>> >> > +#define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT
>> > 14
>> >> > +#define DRM_MODE_DP_COLORIMETRY_SCRGB 15
>> >> > +#define DRM_MODE_DP_COLORIMETRY_DCI_P3 16
>> >> > +#define DRM_MODE_DP_COLORIMETRY_CUSTOM_COLOR_PROFILE
>> > 17
>> >>
>> >> I think some kind of macro to define these would be good. Assuming
>> >> we're trying directly interpret these as the C/EC/ACE bits. I can't
>> >> remember if multiple enum values can have the same integer value.
>> >> If not we'd also need a YCbCr vs. RGB bit in there somewhere.
>> >> Assuming we want to keep RGB and YCbCr separate. Currently only
>> >> BT.2020 has a conflict between the two.
>> >
>> >So I think I'm leaning towards having separate YCbCr vs. RGB values
>> >(which is what you had here already, with BT.2020 being the only case
>> >where that actually matters). That way someone could even do YCbCr
>> >4:4:4 passthrough by just stuffing YUV data into their RGB
>> >framebuffer (with the components swizzled the right way).
>> >
>> >The one thing that rather breaks that usecase is the automagic 4k
>> >YCbCr 4:2:0 fallback. So in in order to guarantee that things work
>> >correctly I guess we still need explcit control over the output CSC.
>> >
>> >Ie. some kind of output color encoding prop like this:
>> > default /* current behaviour */
>> > ycbcr BT.601
>> > ycbcr BT.709
>> > ycbcr BT.2020
>> > /* no ycbcr 4:2:0 fallback for below modes? */
>> > rgb 4:4:4
>> > ycbcr 4:4:4 BT.601
>> > ycbcr 4:4:4 BT.709
>> > ycbcr 4:4:4 BT.2020
>> > ycbcr 4:2:2 BT.601
>> > ycbcr 4:2:2 BT.709
>> > ycbcr 4:2:2 BT.2020
>> > ycbcr 4:2:0 BT.601
>> > ycbcr 4:2:0 BT.709
>> > ycbcr 4:2:0 BT.2020
>> >
>> >Hmm. In fact the automagic 4:2:0 fallback sort of breaks the
>> >colorspace prop alredy because if someone set an RGB colorspace but
>> >we end up converting to YCbCr 4:2:0 what should we tell the sink?
>> >Maybe we just reject the modeset in that case and tell users they
>> >need to wait for the output color encoding prop to support that usecase?
>>
>> Hi yes, an output encoding property to control this make sense and will be
>useful.
>> So if a user decides to have a RGB buffer at source or wants to blend
>> multiple layers in RGB, and still want o/p converted to YUV420 at o/p,
>> he can use the current colorspace property to set that and send to
>> sink. In case he doesn't do it explicitly we can still let the content be
>> driven but
>with warnings or choose to fail his modeset if that seems better.
>>
>> What do you suggest, should we keep current property interface like
>> this and extend the usage with a separate property on top of it.
>
>I think separate prop to control the output CSC is best. The colorspace prop
>should only control the infoframe. If the two are in violent disagreement we
>can
>fail the modeset.
Ok Sure, Will take that as a separate task to add a property interface for
output CSC.
Thanks Ville !!!
>>
>> Thanks Ville for your review comments and suggestions.
>>
>> Regards,
>> Uma Shankar
>>
>> >
>> >>
>> >> > +
>> >> > /**
>> >> > * struct drm_display_info - runtime data about the connected sink
>> >> > *
>> >> > @@ -503,6 +535,13 @@ struct drm_connector_state {
>> >> > unsigned int content_protection;
>> >> >
>> >> > /**
>> >> > + * @colorspace: State variable for Connector property to request
>> >> > + * colorspace change on Sink. This is most commonly used to
>> >> > switch
>> >> > + * to wider color gamuts like BT2020.
>> >> > + */
>> >> > + u32 colorspace;
>> >> > +
>> >> > + /**
>> >> > * @writeback_job: Writeback job for writeback connectors
>> >> > *
>> >> > * Holds the framebuffer and out-fence for a writeback
>> >> > connector.
>> >> > As @@ -995,6 +1034,12 @@ struct drm_connector {
>> >> > struct drm_property *content_protection_property;
>> >> >
>> >> > /**
>> >> > + * @colorspace_property: Connector property to set the suitable
>> >> > + * colorspace supported by the sink.
>> >> > + */
>> >> > + struct drm_property *colorspace_property;
>> >> > +
>> >> > + /**
>> >> > * @path_blob_ptr:
>> >> > *
>> >> > * DRM blob property data for the DP MST path property. This
>> >> > should only @@ -1269,6 +1314,7 @@ int
>> >> > drm_connector_attach_vrr_capable_property(
>> >> > int drm_connector_attach_content_protection_property(
>> >> > struct drm_connector *connector); int
>> >> > drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>> >> > +int drm_mode_create_colorspace_property(struct drm_connector
>> >> > +*connector);
>> >> > int drm_mode_create_content_type_property(struct drm_device
>> >> > *dev); void drm_hdmi_avi_infoframe_content_type(struct
>> >> > hdmi_avi_infoframe
>> >*frame,
>> >> > const struct
>> >> > drm_connector_state
>> >*conn_state);
>> >> > --
>> >> > 1.9.1
>> >> >
>> >> > _______________________________________________
>> >> > Intel-gfx mailing list
>> >> > intel-...@lists.freedesktop.org
>> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>
>> >> --
>> >> Ville Syrjälä
>> >> Intel
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> intel-...@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
