>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Ville Syrjälä >Sent: Tuesday, February 5, 2019 11:43 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: [v11 1/4] drm: Add HDMI colorspace property > >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" 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 ? 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-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
