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

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

 




>-----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




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

  Powered by Linux