RE: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace property

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

 



Regards
Shashank

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
> Shankar, Uma
> Sent: Friday, February 8, 2019 5:45 PM
> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>
> Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace property
> 
> >> >> >-----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: [Intel-gfx] [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
> >?
> >
> >I don't think we can separate them. The values of the colorimetry bits
> >doesn't mean anything without the Y bits. It would make the uapi kinda
> >crazy if we allow the user to specify BT.2020 RGB but then we actually
> >signal BT.2020 YCbCr in the infoframe, or vice versa. Or we just signal
> >a totally invalid value for all the other cases.
> 
> I agree we need data format also to be send along with colorspace, but they are 2
> different things. To handle YCbCr and variants (YUV 444, 420 or 422) and RGB I feel
> we should expose a different API instead of using this one.  We can create an output
> csc property which does that job for us.
> 
> So from a user perspective if we wants to set a colorspace we should use the one in
> this series and set the data format separately using the other interface. Both will be
> received in the state variables and later Infoframe packet will be created accordingly.
> Clubbing both in one may lead to lot of possible combinations exposed as enum
> which may not look too clean.
> 
> What do you say about handling that in the output csc property. I will go with what you
> recommend .

Programming Y2Y1Y0 is already taken care of, when we added support for YCBCR outputs (4:2:0 implementation). 
In intel_hdmi.c:intel_hdmi_set_avi_infoframe():

if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
	frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
else if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)
	frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
else
	frame.avi.colorspace = HDMI_COLORSPACE_RGB;

IMO This looks like a better way to handle this, ie while adding support for a new format, add support for corresponding AVI IF fileds. This will also make scope of the property under discussion less complicated.  

> 
> Regards,
> Uma Shankar
> 
> 
> 
> >>
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux