>-----Original Message-----
>From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 5:28 PM
>To: Shankar, Uma <uma.shan...@intel.com>; intel-gfx@lists.freedesktop.org;
>dri-de...@lists.freedesktop.org
>Cc: hansv...@cisco.com; Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst,
>Maarten <maarten.lankho...@intel.com>; jo...@kwiboo.se
>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.shan...@intel.com>
>> ---
>> 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.sha...@intel.com>
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-de...@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
