Re: [v10 1/3] 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: Monday, February 4, 2019 10:54 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; dri-
>devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten
><maarten.lankhorst@xxxxxxxxx>
>Subject: Re:  [v10 1/3] drm: Add HDMI colorspace property
>
>On Mon, Feb 04, 2019 at 05:08:40PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
>> >Sent: Monday, February 4, 2019 8:55 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:  [v10 1/3] drm: Add HDMI colorspace property
>> >
>> >On Fri, Feb 01, 2019 at 08:50:11PM +0200, Ville Syrjälä wrote:
>> >> On Wed, Jan 30, 2019 at 06:24:24PM +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.
>> >> >
>> >> > 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   | 75
>> >+++++++++++++++++++++++++++++++++++++++
>> >> >  include/drm/drm_connector.h       | 46 ++++++++++++++++++++++++
>> >> >  3 files changed, 125 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..ed10dd9 100644
>> >> > --- a/drivers/gpu/drm/drm_connector.c
>> >> > +++ b/drivers/gpu/drm/drm_connector.c
>> >> > @@ -826,6 +826,30 @@ 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_ITU_601, "ITU_601" },
>> >>
>> >> The spec no longer has the BT.601 note here. Which I guess makes
>> >> sense since BT.601 didn't even specify any color primaries
>> >> initially. Later versions do by they have distinct primaries for
>> >> NTSC vs. PAL. The spec calls this just SMPTE 170M now, so I think we should
>do the same.
>>
>> Ok Sure, Will update this name. Yeah this was a little confusing as to
>> what name to go with. Will take the one defined now in spec i.e., SMPTE 170M.
>>
>> >>
>> >> > +	{ DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>> >> > +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> >> > +	{ DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> >> > +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>> >> > +	{ DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> >> > +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> >> > +	{ DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> >> > +	/* Colorimetry based on IEC 61966-2-5 [33] */
>> >> > +	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>> >>
>> >> The spelling is rather inconsistent here. XV_ vs. S_ vs. op. Why
>> >> not use the same spelling style for all?
>>
>> This is the names defined in spec and I used as is. I will make them
>> as XVYCC, SYCC and OPYCC for consistency. Hope this is ok ?
>>
>> >>
>> >> > +	/* Colorimetry based on IEC 61966-2-5 */
>> >> > +	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>> >> > +	/* 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_YCC, "BT2020_YCC" },
>> >> > +	/* Colorimetry based on ITU-R BT.2020 */
>> >> > +	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>> >>
>> >> "BT2020" vs. "ITU_709" is rather inconsistent as well.
>>
>> Ok, will update this.
>>
>> >> We also seem to be missing the two DCI-P3 things here.
>>
>> Yeah, I initially did as per 861.F where it was not there. This got
>> added in 861.G, will  add this.
>>
>> >> > +};
>> >> > +
>> >> >  /**
>> >> >   * DOC: standard connector properties
>> >> >   *
>> >> > @@ -1548,6 +1572,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..29495b3 100644
>> >> > --- a/include/drm/drm_connector.h
>> >> > +++ b/include/drm/drm_connector.h
>> >> > @@ -253,6 +253,38 @@ 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_ITU_601			1
>> >> > +#define DRM_MODE_COLORIMETRY_ITU_709			2
>> >> > +/* CEA 861 Extended Colorimetry Options */
>> >> > +#define DRM_MODE_COLORIMETRY_XV_YCC_601
>	3
>> >> > +#define DRM_MODE_COLORIMETRY_XV_YCC_709
>	4
>> >> > +#define DRM_MODE_COLORIMETRY_S_YCC_601			5
>> >> > +#define DRM_MODE_COLORIMETRY_OPYCC_601
>	6
>> >> > +#define DRM_MODE_COLORIMETRY_OPRGB			7
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_RGB
>	8
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_YCC
>	9
>> >> > +#define DRM_MODE_COLORIMETRY_BT2020_CYCC		10
>> >> > +/* DP MSA Colorimetry Options */
>> >> > +#define DRM_MODE_DP_COLORIMETRY_Y_CBCR_ITU_601
>	11
>> >> > +#define DRM_MODE_DP_COLORIMETRY_Y_CBCR_ITU_709
>	12
>> >> > +#define DRM_MODE_DP_COLORIMETRY_SRGB			13
>> >> > +#define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT
>> >	14
>> >> > +#define DRM_MODE_DP_COLORIMETRY_SCRGB			15
>> >> > +#define DRM_MODE_DP_COLORIMETRY_DCI_P3			16
>> >> > +#define DRM_MODE_DP_COLORIMETRY_CUSTOM_COLOR_PROFILE
>> >	17
>> >>
>> >> I think some kind of macro to define these would be good. Assuming
>> >> we're trying directly interpret these as the C/EC/ACE bits. I can't
>> >> remember if multiple enum values can have the same integer value.
>> >> If not we'd also need a YCbCr vs. RGB bit in there somewhere.
>> >> Assuming we want to keep RGB and YCbCr separate. Currently only
>> >> BT.2020 has a conflict between the two.
>> >
>> >So I think I'm leaning towards having separate YCbCr vs. RGB values
>> >(which is what you had here already, with BT.2020 being the only case
>> >where that actually matters). That way someone could even do YCbCr
>> >4:4:4 passthrough by just stuffing YUV data into their RGB
>> >framebuffer (with the components swizzled the right way).
>> >
>> >The one thing that rather breaks that usecase is the automagic 4k
>> >YCbCr 4:2:0 fallback. So in in order to guarantee that things work
>> >correctly I guess we still need explcit control over the output CSC.
>> >
>> >Ie. some kind of output color encoding prop like this:
>> >	default /* current behaviour */
>> >	ycbcr BT.601
>> >	ycbcr BT.709
>> >	ycbcr BT.2020
>> >	/* no ycbcr 4:2:0 fallback for below modes? */
>> >	rgb 4:4:4
>> >	ycbcr 4:4:4 BT.601
>> >	ycbcr 4:4:4 BT.709
>> >	ycbcr 4:4:4 BT.2020
>> >	ycbcr 4:2:2 BT.601
>> >	ycbcr 4:2:2 BT.709
>> >	ycbcr 4:2:2 BT.2020
>> >	ycbcr 4:2:0 BT.601
>> >	ycbcr 4:2:0 BT.709
>> >	ycbcr 4:2:0 BT.2020
>> >
>> >Hmm. In fact the automagic 4:2:0 fallback sort of breaks the
>> >colorspace prop alredy because if someone set an RGB colorspace but
>> >we end up converting to YCbCr 4:2:0 what should we tell the sink?
>> >Maybe we just reject the modeset in that case and tell users they
>> >need to wait for the output color encoding prop to support that usecase?
>>
>> Hi yes, an output encoding property to control this make sense and will be
>useful.
>> So if a user decides to have a RGB buffer at source or wants to blend
>> multiple layers in RGB, and still want o/p converted to YUV420 at o/p,
>> he can use the current colorspace property to set that and send to
>> sink. In case he doesn't do it explicitly we can still let the content be driven but
>with warnings or choose to fail his modeset if that seems better.
>>
>> What do you suggest, should we keep current property interface like
>> this and extend the usage with a separate property on top of it.
>
>I think separate prop to control the output CSC is best. The colorspace prop
>should only control the infoframe. If the two are in violent disagreement we can
>fail the modeset.

Ok Sure, Will take that as a separate task to add a property interface for output CSC.
Thanks Ville !!!

>>
>> Thanks Ville for your review comments and suggestions.
>>
>> Regards,
>> Uma Shankar
>>
>> >
>> >>
>> >> > +
>> >> >  /**
>> >> >   * struct drm_display_info - runtime data about the connected sink
>> >> >   *
>> >> > @@ -503,6 +535,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 +1034,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 +1314,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
>> >
>> >--
>> >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