On Mon, Feb 04, 2019 at 05:08:40PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >Sent: Monday, February 4, 2019 8:55 PM > >To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; > >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>; dri- > >devel@xxxxxxxxxxxxxxxxxxxxx > >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.shankar@xxxxxxxxx> > >> > Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx> > >> > Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > >> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >> > --- > >> > 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-gfx@xxxxxxxxxxxxxxxxxxxxx > >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > >> -- > >> Ville Syrjälä > >> Intel > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >-- > >Ville Syrjälä > >Intel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
