Re: [v11 1/4] drm: Add HDMI colorspace property

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

 



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.

> +/* 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.

> +/* 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).

> +#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
_______________________________________________
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