>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Ville Syrjälä >Sent: Monday, February 4, 2019 10:54 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; dri- >devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [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.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: [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. 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-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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
