Re: [v7 1/2] drm: Add colorspace connector property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Uma,

On Tue, Jan 29, 2019 at 01:30:43PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx]
> >Sent: Tuesday, January 29, 2019 5:54 PM
> >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> >dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten
> ><maarten.lankhorst@xxxxxxxxx>
> >Subject: Re:  [v7 1/2] drm: Add colorspace connector property
> >
> >Op 28-01-2019 om 09:44 schreef Uma Shankar:
> >> 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. Driver will expose the platform supported colorspaces,
> >> however sink supported colorspaces should be retrieved by userspace
> >> from EDID.
> >>
> >> 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.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
> >>  drivers/gpu/drm/drm_connector.c   | 95
> >+++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_connector.h       | 17 +++++++
> >>  include/uapi/drm/drm_mode.h       | 32 +++++++++++++
> >>  4 files changed, 148 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..eafa643 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_colorspaces[] = {
> >> +	/* 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 */ static const struct
> >> +drm_prop_enum_list dp_colorspaces[] = {
> >> +	/* 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
> >>   *
> >> @@ -1548,6 +1596,53 @@ 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. Driver will expose the platform supported colorspaces,
> >> + *     however sink supported colorspaces should be retrieved by userspace
> >> + *     from EDID.
> >> + *
> >> + *     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,
> >> +					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 9941613..e32cd11 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;
> >> @@ -503,6 +504,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 +1003,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 +1283,9 @@ 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,
> >> +					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
> >> a439c2e..2bcdfcf 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -210,6 +210,38 @@
> >>  #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.
> >> + */
> >> +
> >> +/* For Default case, driver will set the colorspace */
> >> +#define COLORIMETRY_DEFAULT	0
> >> +/* CEA 861 Normal Colorimetry options */
> >> +#define COLORIMETRY_ITU_601	1
> >> +#define COLORIMETRY_ITU_709	2
> >> +/* CEA 861 Extended Colorimetry Options */
> >> +#define COLORIMETRY_XV_YCC_601	3
> >> +#define COLORIMETRY_XV_YCC_709	4
> >> +#define COLORIMETRY_S_YCC_601	5
> >> +#define COLORIMETRY_OPYCC_601	6
> >> +#define COLORIMETRY_OPRGB	7
> >> +#define COLORIMETRY_BT2020_RGB	8
> >> +#define COLORIMETRY_BT2020_YCC	9
> >> +#define COLORIMETRY_BT2020_CYCC	10
> >> +/* DP MSA Colorimetry Options */
> >> +#define DP_COLORIMETRY_Y_CBCR_ITU_601	11
> >> +#define DP_COLORIMETRY_Y_CBCR_ITU_709	12
> >> +#define	DP_COLORIMETRY_SRGB		13
> >> +#define	DP_COLORIMETRY_RGB_WIDE_GAMUT	14
> >tab vs space :)
> 
> Sure, will fix this.
> 
> >> +#define DP_COLORIMETRY_SCRGB		15
> >> +#define DP_COLORIMETRY_DCI_P3		16
> >> +#define DP_COLORIMETRY_CUSTOM_COLOR_PROFILE	17

Sorry, I somehow missed your reply earlier in the month - but this
wasn't what I meant.

The expectation with enum properties is that the numeric values are
determined at runtime - userspace shouldn't depend on them being known
at compile-time. So, in include/uapi there shouldn't be these numeric
values.

The strings themselves effectively form the UABI, so I was wondering
if they should be defined in include/uapi, but you would be the first
to do that.

Daniel Vetter and/or Daniel Stone might have opinions on this.

Thanks,
-Brian

> >
> >Since this is in UAPI, you need to prefix with DRM_MODE_
> 
> Ok sure, will update this. Thanks  Maarten !!!
> 
> Regards,
> Uma Shankar
> 
> >~Maarten
> >
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux