Re: [v8 2/2] drm/i915: Attach colorspace property and enable modeset

["Sharma, Shashank" <shashank.sharma@xxxxxxxxxxxxxxx>]

  Hello Ville,

On 1/29/2019 9:33 PM, Ville Syrjälä wrote:
> On Tue, Jan 29, 2019 at 03:57:29PM +0000, Shankar, Uma wrote:
> > > -----Original Message-----
> > > From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
> > > Ville Syrjälä
> > > Sent: Tuesday, January 29, 2019 9:14 PM
> > > To: Shankar, Uma <uma.shankar@xxxxxxxxx>
> > > Cc: emil.l.velikov@xxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville
> > > <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>;
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Subject: Re: [v8 2/2] drm/i915: Attach colorspace property and enable modeset
> > >
> > > On Tue, Jan 29, 2019 at 09:22:56PM +0530, Uma Shankar wrote:
> > > > This patch attaches the colorspace connector property to the hdmi
> > > > connector. Based on colorspace change, modeset will be triggered to
> > > > switch to new colorspace.
> > > >
> > > > Based on colorspace property value create an infoframe with
> > > > appropriate colorspace. This can be used to send an infoframe packet
> > > > with proper colorspace value set which will help to enable wider color
> > > > gamut like BT2020 on sink.
> > > >
> > > > This patch attaches and enables HDMI colorspace, DP will be taken care
> > > > separately.
> > > >
> > > > v2: Merged the changes of creating infoframe as well to this patch as
> > > > per Maarten's suggestion.
> > > >
> > > > v3: Addressed review comments from Shashank. Separated HDMI and DP
> > > > colorspaces as suggested by Ville and Maarten.
> > > >
> > > > 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. Handle the
> > > > default case properly.
> > > >
> > > > v5: Added Platform specific colorspace enums and called the property
> > > > creation helper using the same.
> > > >
> > > > v6: Addressed Shashank's review comments.
> > > >
> > > > v7: Rebase
> > > >
> > > > v8: Addressed Maarten's review comments.
> > > >
> > > > Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > > > Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_atomic.c    |  1 +
> > > >   drivers/gpu/drm/i915/intel_connector.c | 63
> > > ++++++++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/i915/intel_drv.h       |  1 +
> > > >   drivers/gpu/drm/i915/intel_hdmi.c      | 24 +++++++++++++
> > > >   4 files changed, 89 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > > index 16263ad..76b7114 100644
> > > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > > @@ -124,6 +124,7 @@ int intel_digital_connector_atomic_check(struct
> > > drm_connector *conn,
> > > >   	 */
> > > >   	if (new_conn_state->force_audio != old_conn_state->force_audio ||
> > > >   	    new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb
> > > > ||
> > > > +	    new_state->colorspace != old_state->colorspace ||
> > > >   	    new_conn_state->base.picture_aspect_ratio != old_conn_state-
> > > > base.picture_aspect_ratio ||
> > > >   	    new_conn_state->base.content_type != old_conn_state-
> > > > base.content_type ||
> > > >   	    new_conn_state->base.scaling_mode !=
> > > > old_conn_state->base.scaling_mode)
> > > > diff --git a/drivers/gpu/drm/i915/intel_connector.c
> > > > b/drivers/gpu/drm/i915/intel_connector.c
> > > > index ee16758..9a12d5e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_connector.c
> > > > +++ b/drivers/gpu/drm/i915/intel_connector.c
> > > > @@ -30,6 +30,48 @@
> > > >   #include "intel_drv.h"
> > > >   #include "i915_drv.h"
> > > >
> > > > +static const struct drm_prop_enum_list gen10_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" },
> > > > +	{ 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" },
> > > > +	/* 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" }, };
> > > > +
> > > > +static const struct drm_prop_enum_list legacy_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" },
> > > > +	{ 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" },
> > > > +	/* Colorimetry based on IEC 61966-2-5 */
> > > > +	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, };
> > > Why are we duplicating these in the driver code? And why do we have different
> > > sets for gen10 and older?
> > This is to expose colorspaces which a particular platforms supports instead of giving
> > full HDMI protocol supported list as some platforms may not support all of it.
> >
> > Here, BT2020 support is added for Gen10 onwards, while older platforms support
> > all  the rest apart from BT2020.
> This is just about the infoframe. I don't think we have any platform
> limitations there.

I am not sure about this, reasons:

- this property not only prepares infoframe, but indicates userspace 
that the platforms has support for following colorspaces. Many 
userspaces might be taking a call of BT2020/HDR support based on this 
property.

- Also, can we send CTA-861-G infoframes using a HDMI-1.4b devices ? 
Even if we can, is that the right thing to do ?


- Shashank

> > For sink supported colorspaces, userspace will retrieve it from EDID and set the colorspace
> > which is supported by both platform as well as sink.
> >
> > Regards,
> > Uma Shankar
> >
> > > > +
> > > >   int intel_connector_init(struct intel_connector *connector)  {
> > > >   	struct intel_digital_connector_state *conn_state; @@ -265,3 +307,24
> > > > @@ int intel_ddc_get_modes(struct drm_connector *connector,
> > > >   			connector->dev->mode_config.aspect_ratio_property,
> > > >   			DRM_MODE_PICTURE_ASPECT_NONE);
> > > >   }
> > > > +
> > > > +void
> > > > +intel_attach_colorspace_property(struct drm_connector *connector) {
> > > > +	struct drm_device *dev = connector->dev;
> > > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 10) {
> > > > +		if (!drm_mode_create_colorspace_property(connector,
> > > > +			gen10_hdmi_colorspaces,
> > > > +			ARRAY_SIZE(gen10_hdmi_colorspaces)))
> > > > +			drm_object_attach_property(&connector->base,
> > > > +				connector->colorspace_property, 0);
> > > > +	} else {
> > > > +		if (!drm_mode_create_colorspace_property(connector,
> > > > +			legacy_hdmi_colorspaces,
> > > > +			ARRAY_SIZE(legacy_hdmi_colorspaces)))
> > > > +			drm_object_attach_property(&connector->base,
> > > > +				connector->colorspace_property, 0);
> > > > +	}
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 85b913e..5178a9a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1783,6 +1783,7 @@ int intel_connector_update_modes(struct
> > > > drm_connector *connector,  void
> > > > intel_attach_force_audio_property(struct drm_connector *connector);
> > > > void intel_attach_broadcast_rgb_property(struct drm_connector
> > > > *connector);  void intel_attach_aspect_ratio_property(struct
> > > > drm_connector *connector);
> > > > +void intel_attach_colorspace_property(struct drm_connector
> > > > +*connector);
> > > >
> > > >   /* intel_csr.c */
> > > >   void intel_csr_ucode_init(struct drm_i915_private *); diff --git
> > > > a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index 97a98e1..9ed00e3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -498,6 +498,24 @@ static void intel_hdmi_set_avi_infoframe(struct
> > > intel_encoder *encoder,
> > > >   	else
> > > >   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > >
> > > > +	if (conn_state->colorspace == DRM_MODE_COLORIMETRY_DEFAULT) {
> > > > +		/* Set ITU 709 as default for HDMI */
> > > > +		frame.avi.colorimetry = DRM_MODE_COLORIMETRY_ITU_709;
> > > > +	} else if (conn_state->colorspace <
> > > DRM_MODE_COLORIMETRY_XV_YCC_601) {
> > > > +		frame.avi.colorimetry = conn_state->colorspace;
> > > > +	} else {
> > > > +		frame.avi.colorimetry = HDMI_COLORIMETRY_EXTENDED;
> > > > +		/*
> > > > +		 * Starting from extended list where COLORIMETRY_XV_YCC_601
> > > > +		 * is the first extended mode and its value is 0 as per HDMI
> > > > +		 * specification.
> > > > +		 * TODO: This needs to be extended for LSPCON implementation
> > > > +		 * as well. Will be implemented separately.
> > > > +		 */
> > > > +		frame.avi.extended_colorimetry = conn_state->colorspace -
> > > > +			DRM_MODE_COLORIMETRY_XV_YCC_601;
> > > > +	}
> > > > +
> > > >   	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > >   					   conn_state->connector,
> > > >   					   adjusted_mode,
> > > > @@ -2143,10 +2161,16 @@ static void intel_hdmi_destroy(struct
> > > > drm_connector *connector)  intel_hdmi_add_properties(struct intel_hdmi
> > > > *intel_hdmi, struct drm_connector *connector)  {
> > > >   	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > +				hdmi_to_dig_port(intel_hdmi);
> > > >
> > > >   	intel_attach_force_audio_property(connector);
> > > >   	intel_attach_broadcast_rgb_property(connector);
> > > >   	intel_attach_aspect_ratio_property(connector);
> > > > +	/* Attach Colorspace property for Non LSPCON based device */
> > > > +	if (!intel_dig_port->lspcon.active)
> > > > +		intel_attach_colorspace_property(connector);
> > > > +
> > > >   	drm_connector_attach_content_type_property(connector);
> > > >   	connector->state->picture_aspect_ratio =
> > > HDMI_PICTURE_ASPECT_NONE;
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > --
> > > 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