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.
>
> 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
