On Tue, Feb 05, 2019 at 05:32:16PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >Sent: Tuesday, February 5, 2019 10:02 PM > >To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville > ><ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > >Subject: Re: [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.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 | 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" > > > > >> +/* 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-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >-- > >Ville Syrjälä > >Intel -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
