On Fri, Feb 08, 2019 at 12:36:25PM +0000, Sharma, Shashank wrote: > Regards > Shashank > > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf > > Of > > Shankar, Uma > > Sent: Friday, February 8, 2019 5:45 PM > > To: Ville Syrjälä <ville.syrj...@linux.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] [v11 1/4] drm: Add HDMI colorspace property > > > > >> >> >-----Original Message----- > > >> >> >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com] > > >> >> >Sent: Tuesday, February 5, 2019 10:02 PM > > >> >> >To: Shankar, Uma <uma.shan...@intel.com> > > >> >> >Cc: intel-...@lists.freedesktop.org; > > >> >> >dri-devel@lists.freedesktop.org; Syrjala, Ville > > >> >> ><ville.syrj...@intel.com>; Lankhorst, Maarten > > >> >> ><maarten.lankho...@intel.com> > > >> >> >Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace > > >> >> >property > > >> >> > > > >> >> >On Tue, Feb 05, 2019 at 09:33:34PM +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. > > >> >> >> > > >> >> >> v11: Addressed Ville's review comments. Updated the Macro > > >> >> >> naming and added DCI-P3 colorspace as well defined in CEA 861.G > > >> >> >> spec. > > >> >> >> > > >> >> >> 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 | 78 > > >> >> >+++++++++++++++++++++++++++++++++++++++ > > >> >> >> include/drm/drm_connector.h | 50 +++++++++++++++++++++++++ > > >> >> >> 3 files changed, 132 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..4309873 100644 > > >> >> >> --- a/drivers/gpu/drm/drm_connector.c > > >> >> >> +++ b/drivers/gpu/drm/drm_connector.c > > >> >> >> @@ -826,6 +826,33 @@ 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_SMPTE_170M, "SMPTE_170M" }, > > >> >> >> + { DRM_MODE_COLORIMETRY_BT709, "BT709" }, > > >> >> >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ > > >> >> >> + { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" }, > > >> >> >> + /* High Definition Colorimetry based on IEC 61966-2-4 */ > > >> >> >> + { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" }, > > >> >> >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ > > >> >> >> + { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" }, > > >> >> >> + /* Colorimetry based on IEC 61966-2-5 [33] */ > > >> >> >> + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" }, > > >> >> >> + /* Colorimetry based on IEC 61966-2-5 */ > > >> >> >> + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, > > >> >> >> + /* 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_RGB, "BT2020_RGB" }, > > >> >> >> + /* Colorimetry based on ITU-R BT.2020 */ > > >> >> >> + { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, > > >> >> >> + /* Added as part of Additional Colorimetry Extension in 861.G */ > > >> >> >> + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI- > > >P3_RGB_D65" }, > > >> >> >> + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI- > > >> >> >P3_RGB_Theater" }, > > >> >> >> +}; > > >> >> >> + > > >> >> >> /** > > >> >> >> * DOC: standard connector properties > > >> >> >> * > > >> >> >> @@ -1548,6 +1575,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..58db66e 100644 > > >> >> >> --- a/include/drm/drm_connector.h > > >> >> >> +++ b/include/drm/drm_connector.h > > >> >> >> @@ -253,6 +253,42 @@ 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_NO_DATA 0 > > >> >> >> +#define DRM_MODE_COLORIMETRY_SMPTE_170M > > >> > 1 > > >> >> >> +#define DRM_MODE_COLORIMETRY_BT709 2 > > >> >> > > > >> >> >Still missing the YCbCr information in these two. > > >> >> > > >> >> As per CTA 861.G spec, we have these as only SMPTE_170M and ITU-R > > >> >> BT709, with no specific mention of RGB or YCbCr. Hence, kept it as > > >> >> per spec. We seem to have a common field for both as per CTA spec. > > >> >> Correct me if my understanding is wrong. > > >> > > > >> >Check > > >> >"Table 14 Picture Colorimetry Indicated by the RGB or YC B C R (Y), > > >> >Colorimetry > > >> >(C) and Extended Colorimetry (EC) Field Settings" > > >> > > >> These Y2 Y1 Y0 values should be programmed separately and not > > >> through the colorspace as they are data formats isn't it. I feel this > > >> should get controlled separately independent of colorimetry, or > > >> should we add all the YCbCr and RGB versions and program them as part > > >> of this property itself > > >? > > > > > >I don't think we can separate them. The values of the colorimetry bits > > >doesn't mean anything without the Y bits. It would make the uapi kinda > > >crazy if we allow the user to specify BT.2020 RGB but then we actually > > >signal BT.2020 YCbCr in the infoframe, or vice versa. Or we just signal > > >a totally invalid value for all the other cases. > > > > I agree we need data format also to be send along with colorspace, but they > > are 2 > > different things. To handle YCbCr and variants (YUV 444, 420 or 422) and > > RGB I feel > > we should expose a different API instead of using this one. We can create > > an output > > csc property which does that job for us. > > > > So from a user perspective if we wants to set a colorspace we should use > > the one in > > this series and set the data format separately using the other interface. > > Both will be > > received in the state variables and later Infoframe packet will be created > > accordingly. > > Clubbing both in one may lead to lot of possible combinations exposed as > > enum > > which may not look too clean. > > > > What do you say about handling that in the output csc property. I will go > > with what you > > recommend . > > Programming Y2Y1Y0 is already taken care of, when we added support for YCBCR > outputs (4:2:0 implementation). > In intel_hdmi.c:intel_hdmi_set_avi_infoframe(): > > if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) > frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > else if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) > frame.avi.colorspace = HDMI_COLORSPACE_YUV444; > else > frame.avi.colorspace = HDMI_COLORSPACE_RGB; > > IMO This looks like a better way to handle this, ie while adding support for > a new format, add support for corresponding AVI IF fileds. This will also > make scope of the property under discussion less complicated.
That's an exceptional case. We're programming the CSC in that case to do the RGB->YCbCr conversion without telling userspace. So we must also configure the Y bits automagically. What we want is a check like so: if (prop.colorspace != default && output_format != RGB) return -INVAL; because we can't set up the CSC to make a mess of the user's carefully crafted pixels. The pixels might not even contain RGB data in that case. We'll need to extend that a little bit once we add the explicit control of the output CSC via another prop. But the same principle should hold. > > > > > Regards, > > Uma Shankar > > > > > > > > >> > > >> My idea was just to update colorimetry fields alone as part of this > > >> interface. > > >> > > >> >> > > >> >> > > > >> >> >> +/* CEA 861 Extended Colorimetry Options */ #define > > >> >> >> +DRM_MODE_COLORIMETRY_XVYCC_601 > > > 3 > > >> >> >> +#define DRM_MODE_COLORIMETRY_XVYCC_709 > > > 4 > > >> >> >> +#define DRM_MODE_COLORIMETRY_SYCC_601 5 > > >> >> >> +#define DRM_MODE_COLORIMETRY_OPYCC_601 > > > 6 > > >> >> >> +#define DRM_MODE_COLORIMETRY_OPRGB 7 > > >> >> >> +#define DRM_MODE_COLORIMETRY_BT2020_YCC > > > 8 > > >> >> >> +/* Both BT2020_RGB and BT2020_YCbCbCr have same value */ > > >> >> >> +#define DRM_MODE_COLORIMETRY_BT2020_RGB > > > 9 > > >> >> >> +#define DRM_MODE_COLORIMETRY_BT2020_CYCC 9 > > >> >> > > > >> >> >I though you didn't want these numbers to be based on the spec > > >> >> >numbers? This duplicated value seems to go against that idea. > > >> >> > > >> >> Yeah, for HDMI somehow was trying to utilize the definitions to > > >> >> advantage. But I feel, It's better to de-couple this. Define this > > >> >> as normal enum values sequentially so that userspace get readable > > >> >> serial number > > >> >kind values. > > >> >> And use these in encoder files to get proper values to be > > >> >> programmed as per respective spec, while defining those values per > > >> >> encoder > > >separately. > > >> >Hope this is fine. > > >> >> > > >> >> >> +/* Additional Colorimetry extension added as part of CTA 861.G */ > > >> >> >> +#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65 10 > > >> >> >> +#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER > > >> > 11 > > >> >> >> + > > >> >> >> +/* DP MSA Colorimetry Options */ #define > > >> >> >> +DRM_MODE_DP_COLORIMETRY_YCBCR_ITU_601 > > > 12 > > >> >> >> +#define DRM_MODE_DP_COLORIMETRY_YCBCR_ITU_709 > > > 13 > > >> >> > > > >> >> >Still inconsistent naming in many places (ITU_ vs. BT, YCBCR vs. > > >> >> >YCC, order of the two). > > >> >> > > >> >> Will fix this. Thanks Ville. > > >> >> > > >> >> > > > >> >> >> +#define DRM_MODE_DP_COLORIMETRY_SRGB 14 > > >> >> >> +#define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT > > >> > 15 > > >> >> >> +#define DRM_MODE_DP_COLORIMETRY_SCRGB > > > 16 > > >> >> >> + > > >> >> >> /** > > >> >> >> * struct drm_display_info - runtime data about the connected sink > > >> >> >> * > > >> >> >> @@ -503,6 +539,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 +1038,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 +1318,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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel