>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Sharma, Shashank >Sent: Thursday, December 20, 2018 5:28 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >dri-devel@xxxxxxxxxxxxxxxxxxxxx >Cc: hansverk@xxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, >Maarten <maarten.lankhorst@xxxxxxxxx>; jonas@xxxxxxxxx >Subject: Re: [v5 1/2] drm: Add colorspace connector property > >Regards > >Shashank > > >On 12/11/2018 11:44 PM, Uma Shankar wrote: >> This patch adds a colorspace connector property, enabling userspace to >> switch to various supported colorspaces. >> This will help enable BT2020 along with other colorspaces. >> >> 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. >> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> --- >> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ >> drivers/gpu/drm/drm_connector.c | 82 >+++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_connector.h | 17 ++++++++ >> include/uapi/drm/drm_mode.h | 33 ++++++++++++++++ >> 4 files changed, 136 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >> b/drivers/gpu/drm/drm_atomic_uapi.c >> index 86ac339..9df7520 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -729,6 +729,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); @@ >> -797,6 +799,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 fa9baac..46928f7 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -826,6 +826,54 @@ 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) >> >> +/* List of HDMI Colorspaces supported */ static const struct >> +drm_prop_enum_list hdmi_colorspace[] = { >Should we call this list hdmi_colorspaces :) ? Ok, will update this. >> + /* For Default case, driver will set the colorspace */ >> + { COLORIMETRY_DEFAULT, "Default" }, >> + /* Standard Definition Colorimetry based on CEA 861 */ >> + { COLORIMETRY_ITU_601, "ITU_601" }, >> + { COLORIMETRY_ITU_709, "ITU_709" }, >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_601, "XV_YCC_601" }, >> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_709, "XV_YCC_709" }, >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> + { COLORIMETRY_S_YCC_601, "S_YCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 [33] */ >> + { COLORIMETRY_OPYCC_601, "opYCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 */ >> + { COLORIMETRY_OPRGB, "opRGB" }, >> + /* Colorimetry based on ITU-R BT.2020 */ >> + { COLORIMETRY_BT2020_RGB, "BT2020_RGB" }, >> + /* Colorimetry based on ITU-R BT.2020 */ >> + { COLORIMETRY_BT2020_YCC, "BT2020_YCC" }, >> + /* Colorimetry based on ITU-R BT.2020 */ >> + { COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" }, }; >> + >> +/* List of DP Colorspaces supported */ >Same here for dp_colorspaces ? Sure. >> +static const struct drm_prop_enum_list dp_colorspace[] = { >> + /* For Default case, driver will set the colorspace */ >> + { COLORIMETRY_DEFAULT, "Default" }, >> + /* Standard Definition Colorimetry based on CEA 861 */ >> + { COLORIMETRY_ITU_601, "ITU_601" }, >> + { COLORIMETRY_ITU_709, "ITU_709" }, >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_601, "XV_YCC_601" }, >> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_709, "XV_YCC_709" }, >> + /* Colorimetry based on IEC 61966-2-5 */ >> + { COLORIMETRY_OPRGB, "opRGB" }, >> + /* DP MSA Colorimetry */ >> + { DP_COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, >> + { DP_COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, >> + { DP_COLORIMETRY_SRGB, "sRGB" }, >> + { DP_COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, >> + { DP_COLORIMETRY_SCRGB, "scRGB" }, >> + { DP_COLORIMETRY_DCI_P3, "DCI-P3" }, >> + { DP_COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Profile" }, }; >> + >> /** >> * DOC: standard connector properties >> * >> @@ -1402,6 +1450,40 @@ int drm_mode_create_aspect_ratio_property(struct >drm_device *dev) >> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property); >> >> /** >> + * drm_mode_create_colorspace_property - create colorspace property >> + * Colorspace: >> + * 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. >I might be wrong, but is this alignment correct as per the property documentation >style ? There are similar documentation for various properties like: drm_mode_create_content_type_property Followed the same format. I hope this should be ok ? I can add some doc reference as well to consolidate all color related stuff in one common place. >> + * @connector: connector to set property on. >> + * >> + * Called by a driver the first time it's needed, must be attached to >> +desired >> + * connectors. >> + * >> + * Returns: >> + * Zero on success, negative errno on failure. >> + */ >> +int drm_mode_create_colorspace_property(struct drm_connector >*connector, >> + const struct drm_prop_enum_list >*props, >> + int num_values) >> +{ >> + struct drm_device *dev = connector->dev; >> + struct drm_property *prop; >> + >> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, >> + "Colorspace", >> + props, num_values); >> + if (!prop) >> + return -ENOMEM; >> + >> + 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 665b9ca..fa840db 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -31,6 +31,7 @@ >> #include <drm/drm_util.h> >> >> #include <uapi/drm/drm_mode.h> >> +#include <drm/drm_property.h> >> >> struct drm_connector_helper_funcs; >> struct drm_modeset_acquire_ctx; >> @@ -497,6 +498,13 @@ struct drm_connector_state { >> unsigned int content_protection; >> >> /** >> + * @colorspace: Connector property to request colorspace >Please fix this documentation, This is not a property, this is just a state variable to >hold the requested property value until next atomic commit. Ok, sure will rectify this. >> + * change on Sink. This is most commonly used to switch to >> + * wider color gamuts like BT2020. >> + */ >> + enum absolute_colorimetry_list colorspace; >> + >> + /** >> * @writeback_job: Writeback job for writeback connectors >> * >> * Holds the framebuffer and out-fence for a writeback connector. >> As @@ -978,6 +986,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; >Yep, this is the actual property :) Yes !!! >> + >> + /** >> * @path_blob_ptr: >> * >> * DRM blob property data for the DP MST path property. This should >> only @@ -1248,6 +1262,9 @@ int >drm_connector_attach_scaling_mode_property(struct drm_connector >*connector, >> 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, >> + const struct drm_prop_enum_list >*props, >> + int num_values); >> 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); diff --git >> a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index >> d3e0fe3..42efab8 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -210,6 +210,39 @@ >> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >> >> +/* >> + * 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. >> + */ >> +enum absolute_colorimetry_list { >> + /* For Default case, driver will set the colorspace */ >> + COLORIMETRY_DEFAULT = 0, >> + /* CEA 861 Normal Colorimetry options */ >> + COLORIMETRY_ITU_601, >> + COLORIMETRY_ITU_709, >> + /* CEA 861 Extended Colorimetry Options */ >> + COLORIMETRY_XV_YCC_601, >> + COLORIMETRY_XV_YCC_709, >> + COLORIMETRY_S_YCC_601, >> + COLORIMETRY_OPYCC_601, >> + COLORIMETRY_OPRGB, >> + COLORIMETRY_BT2020_RGB, >> + COLORIMETRY_BT2020_YCC, >> + COLORIMETRY_BT2020_CYCC, >> + /* DP MSA Colorimetry Options */ >> + DP_COLORIMETRY_Y_CBCR_ITU_601, >> + DP_COLORIMETRY_Y_CBCR_ITU_709, >> + DP_COLORIMETRY_SRGB, >> + DP_COLORIMETRY_RGB_WIDE_GAMUT, >> + DP_COLORIMETRY_SCRGB, >> + DP_COLORIMETRY_DCI_P3, >> + DP_COLORIMETRY_CUSTOM_COLOR_PROFILE, >> +}; >> + >> struct drm_mode_modeinfo { >> __u32 clock; >> __u16 hdisplay; >With given minor comments dealt with, please feel free to use: >Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> Thanks Shashank for the review and useful feedback. I will update the series with the fixes suggested. Regards, Uma Shankar >_______________________________________________ >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
