[v2 0/2] Add Colorspace connector property interface

[v2 0/2] Add Colorspace connector property interface

[Uma Shankar]

This patch series creates 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. Kernel will not give the supported colorspaces since
this is panel dependent and our curremt property infrastructure is
not supporting it. 

Have tested this using xrandr by using below command:
xrandr --output HDMI2 --set "Colorspace" "BT2020_rgb"

v2: Addressed Ville and Maarten's review comments. Merged the 2nd
and 3rd patch into one common logical patch.

Uma Shankar (2):
  drm: Add colorspace property
  drm/i915: Attach colorspace property and enable modeset

 drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
 drivers/gpu/drm/drm_connector.c     | 44 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_atomic.c |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c   |  5 +++++
 include/drm/drm_connector.h         |  7 ++++++
 include/drm/drm_mode_config.h       |  6 +++++
 include/uapi/drm/drm_mode.h         | 24 ++++++++++++++++++++
 7 files changed, 91 insertions(+)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[v2 1/2] drm: Add colorspace property

[Uma Shankar]

This patch adds a colorspace property enabling
userspace to switch to various supported colorspaces.
This will help enable BT2020 along with other colorspaces.

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.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
 drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  7 +++++++
 include/drm/drm_mode_config.h     |  6 ++++++
 include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
 5 files changed, 85 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f31..9e1d46b 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->picture_aspect_ratio = val;
 	} else if (property == config->content_type_property) {
 		state->content_type = val;
+	} else if (property == config->colorspace_property) {
+		state->colorspace = val;
 	} else if (property == connector->scaling_mode_property) {
 		state->scaling_mode = val;
 	} else if (property == connector->content_protection_property) {
@@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -826,6 +826,38 @@ 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 colorspace[] = {
+	/* Standard Definition Colorimetry based on CEA 861 */
+	{ COLORIMETRY_ITU_601, "601" },
+	{ COLORIMETRY_ITU_709, "709" },
+	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
+	{ COLORIMETRY_XV_YCC_601, "601" },
+	/* High Definition Colorimetry based on IEC 61966-2-4 */
+	{ COLORIMETRY_XV_YCC_709, "709" },
+	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
+	{ COLORIMETRY_S_YCC_601, "s601" },
+	/* Colorimetry based on IEC 61966-2-5 [33] */
+	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
+	/* Colorimetry based on IEC 61966-2-5 */
+	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
+	/* DP MSA Colorimetry */
+	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
+	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
+	{ COLORIMETRY_SRGB, "SRGB" },
+	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
+	{ COLORIMETRY_SCRGB, "SCRGB" },
+	{ COLORIMETRY_DCI_P3, "DCIP3" },
+	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
+	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
+	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
+};
+
 /**
  * DOC: standard connector properties
  *
@@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
  *	can also expose this property to external outputs, in which case they
  *	must support "None", which should be the default (since external screens
  *	have a built-in scaler).
+ *
+ * colorspace:
+ *	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.
  */
 
 int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.non_desktop_property = prop;
 
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
+					colorspace, ARRAY_SIZE(colorspace));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.colorspace_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index dd0552c..b7f5373 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -497,6 +497,13 @@ struct drm_connector_state {
 	unsigned int content_protection;
 
 	/**
+	 * @colorspace: Connector property to request colorspace
+	 * change. This is most commonly used to switch to wider color
+	 * gamuts like BT2020.
+	 */
+	enum encoder_colorimetry colorspace;
+
+	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
 	 * Holds the framebuffer and out-fence for a writeback connector. As
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index d643d26..a6eb0aa 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -863,6 +863,12 @@ struct drm_mode_config {
 	uint32_t cursor_width, cursor_height;
 
 	/**
+	 * @colorspace_property: Connector property to set the suitable
+	 * colorspace supported by the sink.
+	 */
+	struct drm_property *colorspace_property;
+
+	/**
 	 * @suspend_state:
 	 *
 	 * Atomic state when suspended.
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index d3e0fe3..831cc77 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -210,6 +210,30 @@
 #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
 #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
 
+enum encoder_colorimetry {
+	/* CEA 861 Normal Colorimetry options */
+	COLORIMETRY_ITU_601 = 0,
+	COLORIMETRY_ITU_709,
+	/* CEA 861 Extended Colorimetry Options */
+	COLORIMETRY_XV_YCC_601,
+	COLORIMETRY_XV_YCC_709,
+	COLORIMETRY_S_YCC_601,
+	COLORIMETRY_ADOBE_YCC_601,
+	COLORIMETRY_ADOBE_RGB,
+	COLORIMETRY_BT2020_RGB,
+	COLORIMETRY_BT2020_YCC,
+	COLORIMETRY_BT2020_CYCC,
+	/* DP MSA Colorimetry Options */
+	COLORIMETRY_Y_CBCR_ITU_601,
+	COLORIMETRY_Y_CBCR_ITU_709,
+	COLORIMETRY_SRGB,
+	COLORIMETRY_RGB_WIDE_GAMUT,
+	COLORIMETRY_SCRGB,
+	COLORIMETRY_DCI_P3,
+	COLORIMETRY_CUSTOM_COLOR_PROFILE,
+	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
+};
+
 struct drm_mode_modeinfo {
 	__u32 clock;
 	__u16 hdisplay;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property

[Maarten Lankhorst]

Op 31-10-18 om 13:05 schreef Uma Shankar:
> This patch adds a colorspace property enabling
> userspace to switch to various supported colorspaces.
> This will help enable BT2020 along with other colorspaces.
>
> 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.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>  drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  7 +++++++
>  include/drm/drm_mode_config.h     |  6 ++++++
>  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>  5 files changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f31..9e1d46b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->picture_aspect_ratio = val;
>  	} else if (property == config->content_type_property) {
>  		state->content_type = val;
> +	} else if (property == config->colorspace_property) {
> +		state->colorspace = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
>  	} else if (property == connector->content_protection_property) {
> @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -826,6 +826,38 @@ 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 colorspace[] = {
> +	/* Standard Definition Colorimetry based on CEA 861 */
> +	{ COLORIMETRY_ITU_601, "601" },
> +	{ COLORIMETRY_ITU_709, "709" },
> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_601, "601" },
> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_709, "709" },
2 definitions that share the same name?
All in all, I think the enum strings need to be more descriptive and self-consistent.
> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> +	{ COLORIMETRY_S_YCC_601, "s601" },
> +	/* Colorimetry based on IEC 61966-2-5 [33] */
> +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
> +	/* Colorimetry based on IEC 61966-2-5 */
> +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
> +	/* DP MSA Colorimetry */
> +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
> +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
> +	{ COLORIMETRY_SRGB, "SRGB" },
sRGB
> +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
> +	{ COLORIMETRY_SCRGB, "SCRGB" },
scRGB?
> +	{ COLORIMETRY_DCI_P3, "DCIP3" },
DCI-P3?
> +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
^Typo
> +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
> +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
This enum together with the code below is broken.

+	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,

I would just make it COLORIMETRY_UNSET = "Unset".

To set the default colorimetry for all drivers, just make the default value 0 (preferred),
or add it to __drm_atomic_helper_connector_reset().

> +};
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>   *	can also expose this property to external outputs, in which case they
>   *	must support "None", which should be the default (since external screens
>   *	have a built-in scaler).
> + *
> + * colorspace:
> + *	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.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.non_desktop_property = prop;
>  
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
> +					colorspace, ARRAY_SIZE(colorspace));
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.colorspace_property = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index dd0552c..b7f5373 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -497,6 +497,13 @@ struct drm_connector_state {
>  	unsigned int content_protection;
>  
>  	/**
> +	 * @colorspace: Connector property to request colorspace
> +	 * change. This is most commonly used to switch to wider color
> +	 * gamuts like BT2020.
> +	 */
> +	enum encoder_colorimetry colorspace;
> +
> +	/**
>  	 * @writeback_job: Writeback job for writeback connectors
>  	 *
>  	 * Holds the framebuffer and out-fence for a writeback connector. As
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index d643d26..a6eb0aa 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -863,6 +863,12 @@ struct drm_mode_config {
>  	uint32_t cursor_width, cursor_height;
>  
>  	/**
> +	 * @colorspace_property: Connector property to set the suitable
> +	 * colorspace supported by the sink.
> +	 */
> +	struct drm_property *colorspace_property;
> +
> +	/**
>  	 * @suspend_state:
>  	 *
>  	 * Atomic state when suspended.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe3..831cc77 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -210,6 +210,30 @@
>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>  
> +enum encoder_colorimetry {
> +	/* CEA 861 Normal Colorimetry options */
> +	COLORIMETRY_ITU_601 = 0,
> +	COLORIMETRY_ITU_709,
> +	/* CEA 861 Extended Colorimetry Options */
> +	COLORIMETRY_XV_YCC_601,
> +	COLORIMETRY_XV_YCC_709,
> +	COLORIMETRY_S_YCC_601,
> +	COLORIMETRY_ADOBE_YCC_601,
> +	COLORIMETRY_ADOBE_RGB,
> +	COLORIMETRY_BT2020_RGB,
> +	COLORIMETRY_BT2020_YCC,
> +	COLORIMETRY_BT2020_CYCC,
> +	/* DP MSA Colorimetry Options */
> +	COLORIMETRY_Y_CBCR_ITU_601,
> +	COLORIMETRY_Y_CBCR_ITU_709,
> +	COLORIMETRY_SRGB,
> +	COLORIMETRY_RGB_WIDE_GAMUT,
> +	COLORIMETRY_SCRGB,
> +	COLORIMETRY_DCI_P3,
> +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> +};
> +
>  struct drm_mode_modeinfo {
>  	__u32 clock;
>  	__u16 hdisplay;


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property

[Ville Syrjälä]

On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
> Op 31-10-18 om 13:05 schreef Uma Shankar:
> > This patch adds a colorspace property enabling
> > userspace to switch to various supported colorspaces.
> > This will help enable BT2020 along with other colorspaces.
> >
> > 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.
> >
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
> >  drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  7 +++++++
> >  include/drm/drm_mode_config.h     |  6 ++++++
> >  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
> >  5 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d5b7f31..9e1d46b 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		state->picture_aspect_ratio = val;
> >  	} else if (property == config->content_type_property) {
> >  		state->content_type = val;
> > +	} else if (property == config->colorspace_property) {
> > +		state->colorspace = val;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		state->scaling_mode = val;
> >  	} else if (property == connector->content_protection_property) {
> > @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -826,6 +826,38 @@ 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 colorspace[] = {
> > +	/* Standard Definition Colorimetry based on CEA 861 */
> > +	{ COLORIMETRY_ITU_601, "601" },
> > +	{ COLORIMETRY_ITU_709, "709" },
> > +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > +	{ COLORIMETRY_XV_YCC_601, "601" },
> > +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> > +	{ COLORIMETRY_XV_YCC_709, "709" },
> 2 definitions that share the same name?
> All in all, I think the enum strings need to be more descriptive and self-consistent.
+1

> > +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> > +	{ COLORIMETRY_S_YCC_601, "s601" },
> > +	/* Colorimetry based on IEC 61966-2-5 [33] */
> > +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },

Hans (cc:d) recetly noted that these things aren't called
Adobe<something> anymore in the CTA-861 spec. There is some new name for
them, which I now forget.

> > +	/* Colorimetry based on IEC 61966-2-5 */
> > +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
> > +	/* Colorimetry based on ITU-R BT.2020 */
> > +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
> > +	/* DP MSA Colorimetry */
> > +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
> > +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
> > +	{ COLORIMETRY_SRGB, "SRGB" },
> sRGB
> > +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
> > +	{ COLORIMETRY_SCRGB, "SCRGB" },
> scRGB?
> > +	{ COLORIMETRY_DCI_P3, "DCIP3" },
> DCI-P3?
> > +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
> ^Typo
> > +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
> > +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
> This enum together with the code below is broken.
> 
> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> 
> I would just make it COLORIMETRY_UNSET = "Unset".

"Unset" might work. Though feels a bit strange to me. Other ideas
would be "Default", "Automatic", "Undefined" or something along those lines.
Ideally it should convey the meaning of "the driver will pick this for you",
and for that I'd lean towards "Default" or "Automatic".

> 
> To set the default colorimetry for all drivers, just make the default value 0 (preferred),
> or add it to __drm_atomic_helper_connector_reset().
> 
> > +};
> > +
> >  /**
> >   * DOC: standard connector properties
> >   *
> > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
> >   *	can also expose this property to external outputs, in which case they
> >   *	must support "None", which should be the default (since external screens
> >   *	have a built-in scaler).
> > + *
> > + * colorspace:
> > + *	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.
> >   */
> >  
> >  int drm_connector_create_standard_properties(struct drm_device *dev)
> > @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
> >  		return -ENOMEM;
> >  	dev->mode_config.non_desktop_property = prop;
> >  
> > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
> > +					colorspace, ARRAY_SIZE(colorspace));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.colorspace_property = prop;
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index dd0552c..b7f5373 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -497,6 +497,13 @@ struct drm_connector_state {
> >  	unsigned int content_protection;
> >  
> >  	/**
> > +	 * @colorspace: Connector property to request colorspace
> > +	 * change. This is most commonly used to switch to wider color
> > +	 * gamuts like BT2020.
> > +	 */
> > +	enum encoder_colorimetry colorspace;
> > +
> > +	/**
> >  	 * @writeback_job: Writeback job for writeback connectors
> >  	 *
> >  	 * Holds the framebuffer and out-fence for a writeback connector. As
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index d643d26..a6eb0aa 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -863,6 +863,12 @@ struct drm_mode_config {
> >  	uint32_t cursor_width, cursor_height;
> >  
> >  	/**
> > +	 * @colorspace_property: Connector property to set the suitable
> > +	 * colorspace supported by the sink.
> > +	 */
> > +	struct drm_property *colorspace_property;
> > +
> > +	/**
> >  	 * @suspend_state:
> >  	 *
> >  	 * Atomic state when suspended.
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index d3e0fe3..831cc77 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -210,6 +210,30 @@
> >  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> >  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> >  
> > +enum encoder_colorimetry {
> > +	/* CEA 861 Normal Colorimetry options */
> > +	COLORIMETRY_ITU_601 = 0,
> > +	COLORIMETRY_ITU_709,
> > +	/* CEA 861 Extended Colorimetry Options */
> > +	COLORIMETRY_XV_YCC_601,
> > +	COLORIMETRY_XV_YCC_709,
> > +	COLORIMETRY_S_YCC_601,
> > +	COLORIMETRY_ADOBE_YCC_601,
> > +	COLORIMETRY_ADOBE_RGB,
> > +	COLORIMETRY_BT2020_RGB,
> > +	COLORIMETRY_BT2020_YCC,
> > +	COLORIMETRY_BT2020_CYCC,
> > +	/* DP MSA Colorimetry Options */
> > +	COLORIMETRY_Y_CBCR_ITU_601,
> > +	COLORIMETRY_Y_CBCR_ITU_709,
> > +	COLORIMETRY_SRGB,
> > +	COLORIMETRY_RGB_WIDE_GAMUT,
> > +	COLORIMETRY_SCRGB,
> > +	COLORIMETRY_DCI_P3,
> > +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
> > +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> > +};
> > +
> >  struct drm_mode_modeinfo {
> >  	__u32 clock;
> >  	__u16 hdisplay;
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v2 1/2] drm: Add colorspace property

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, November 2, 2018 5:00 PM
>To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
>intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil
><hansverk@cisco.com>
>Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property
>
>On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>> > This patch adds a colorspace property enabling userspace to switch
>> > to various supported colorspaces.
>> > This will help enable BT2020 along with other colorspaces.
>> >
>> > 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.
>> >
>> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>> >  drivers/gpu/drm/drm_connector.c   | 44
>+++++++++++++++++++++++++++++++++++++++
>> >  include/drm/drm_connector.h       |  7 +++++++
>> >  include/drm/drm_mode_config.h     |  6 ++++++
>> >  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>> >  5 files changed, 85 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> > b/drivers/gpu/drm/drm_atomic_uapi.c
>> > index d5b7f31..9e1d46b 100644
>> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct
>drm_connector *connector,
>> >  		state->picture_aspect_ratio = val;
>> >  	} else if (property == config->content_type_property) {
>> >  		state->content_type = val;
>> > +	} else if (property == config->colorspace_property) {
>> > +		state->colorspace = val;
>> >  	} else if (property == connector->scaling_mode_property) {
>> >  		state->scaling_mode = val;
>> >  	} else if (property == connector->content_protection_property) {
>> > @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
>> > --- a/drivers/gpu/drm/drm_connector.c
>> > +++ b/drivers/gpu/drm/drm_connector.c
>> > @@ -826,6 +826,38 @@ 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 colorspace[] = {
>> > +	/* Standard Definition Colorimetry based on CEA 861 */
>> > +	{ COLORIMETRY_ITU_601, "601" },
>> > +	{ COLORIMETRY_ITU_709, "709" },
>> > +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> > +	{ COLORIMETRY_XV_YCC_601, "601" },
>> > +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>> > +	{ COLORIMETRY_XV_YCC_709, "709" },
>> 2 definitions that share the same name?
>> All in all, I think the enum strings need to be more descriptive and self-
>consistent.
>+1

Sure, will modify this.

>> > +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> > +	{ COLORIMETRY_S_YCC_601, "s601" },
>> > +	/* Colorimetry based on IEC 61966-2-5 [33] */
>> > +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>
>Hans (cc:d) recetly noted that these things aren't called Adobe<something>
>anymore in the CTA-861 spec. There is some new name for them, which I now
>forget.

EC2 EC1 EC0 Extended Colorimetry
0       1      1      AdobeYCC601  
This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks
like they are still keeping it that way. Not sure if its updated as part of any latest spec
update.
 
>> > +	/* Colorimetry based on IEC 61966-2-5 */
>> > +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
>> > +	/* Colorimetry based on ITU-R BT.2020 */
>> > +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
>> > +	/* Colorimetry based on ITU-R BT.2020 */
>> > +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
>> > +	/* Colorimetry based on ITU-R BT.2020 */
>> > +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
>> > +	/* DP MSA Colorimetry */
>> > +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
>> > +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
>> > +	{ COLORIMETRY_SRGB, "SRGB" },
>> sRGB

Was trying to avoid camelcase, but for these names, I guess we can keep how
spec defines them. Will change this.

>> > +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
>> > +	{ COLORIMETRY_SCRGB, "SCRGB" },
>> scRGB?

Will update this.

>> > +	{ COLORIMETRY_DCI_P3, "DCIP3" },
>> DCI-P3?

Ok, will update

>> > +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
>> ^Typo

Yeah, will rectify this.

>> > +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
>> > +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
>> This enum together with the code below is broken.
>>
>> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
>>
>> I would just make it COLORIMETRY_UNSET = "Unset".
>
>"Unset" might work. Though feels a bit strange to me. Other ideas would be
>"Default", "Automatic", "Undefined" or something along those lines.
>Ideally it should convey the meaning of "the driver will pick this for you", and for
>that I'd lean towards "Default" or "Automatic".

Ok, will change this to Default.

>>
>> To set the default colorimetry for all drivers, just make the default
>> value 0 (preferred), or add it to __drm_atomic_helper_connector_reset().

Ok, will modify this and resend the next version.

Thanks Ville and Maarten for your review and suggestions.

Regards,
Uma Shankar
>> > +};
>> > +
>> >  /**
>> >   * DOC: standard connector properties
>> >   *
>> > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct
>drm_display_info *info,
>> >   *	can also expose this property to external outputs, in which case they
>> >   *	must support "None", which should be the default (since external screens
>> >   *	have a built-in scaler).
>> > + *
>> > + * colorspace:
>> > + *	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.
>> >   */
>> >
>> >  int drm_connector_create_standard_properties(struct drm_device
>> > *dev) @@ -1020,6 +1058,12 @@ int
>drm_connector_create_standard_properties(struct drm_device *dev)
>> >  		return -ENOMEM;
>> >  	dev->mode_config.non_desktop_property = prop;
>> >
>> > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>"Colorspace",
>> > +					colorspace, ARRAY_SIZE(colorspace));
>> > +	if (!prop)
>> > +		return -ENOMEM;
>> > +	dev->mode_config.colorspace_property = prop;
>> > +
>> >  	return 0;
>> >  }
>> >
>> > diff --git a/include/drm/drm_connector.h
>> > b/include/drm/drm_connector.h index dd0552c..b7f5373 100644
>> > --- a/include/drm/drm_connector.h
>> > +++ b/include/drm/drm_connector.h
>> > @@ -497,6 +497,13 @@ struct drm_connector_state {
>> >  	unsigned int content_protection;
>> >
>> >  	/**
>> > +	 * @colorspace: Connector property to request colorspace
>> > +	 * change. This is most commonly used to switch to wider color
>> > +	 * gamuts like BT2020.
>> > +	 */
>> > +	enum encoder_colorimetry colorspace;
>> > +
>> > +	/**
>> >  	 * @writeback_job: Writeback job for writeback connectors
>> >  	 *
>> >  	 * Holds the framebuffer and out-fence for a writeback connector.
>> > As diff --git a/include/drm/drm_mode_config.h
>> > b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644
>> > --- a/include/drm/drm_mode_config.h
>> > +++ b/include/drm/drm_mode_config.h
>> > @@ -863,6 +863,12 @@ struct drm_mode_config {
>> >  	uint32_t cursor_width, cursor_height;
>> >
>> >  	/**
>> > +	 * @colorspace_property: Connector property to set the suitable
>> > +	 * colorspace supported by the sink.
>> > +	 */
>> > +	struct drm_property *colorspace_property;
>> > +
>> > +	/**
>> >  	 * @suspend_state:
>> >  	 *
>> >  	 * Atomic state when suspended.
>> > diff --git a/include/uapi/drm/drm_mode.h
>> > b/include/uapi/drm/drm_mode.h index d3e0fe3..831cc77 100644
>> > --- a/include/uapi/drm/drm_mode.h
>> > +++ b/include/uapi/drm/drm_mode.h
>> > @@ -210,6 +210,30 @@
>> >  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> >  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>> >
>> > +enum encoder_colorimetry {
>> > +	/* CEA 861 Normal Colorimetry options */
>> > +	COLORIMETRY_ITU_601 = 0,
>> > +	COLORIMETRY_ITU_709,
>> > +	/* CEA 861 Extended Colorimetry Options */
>> > +	COLORIMETRY_XV_YCC_601,
>> > +	COLORIMETRY_XV_YCC_709,
>> > +	COLORIMETRY_S_YCC_601,
>> > +	COLORIMETRY_ADOBE_YCC_601,
>> > +	COLORIMETRY_ADOBE_RGB,
>> > +	COLORIMETRY_BT2020_RGB,
>> > +	COLORIMETRY_BT2020_YCC,
>> > +	COLORIMETRY_BT2020_CYCC,
>> > +	/* DP MSA Colorimetry Options */
>> > +	COLORIMETRY_Y_CBCR_ITU_601,
>> > +	COLORIMETRY_Y_CBCR_ITU_709,
>> > +	COLORIMETRY_SRGB,
>> > +	COLORIMETRY_RGB_WIDE_GAMUT,
>> > +	COLORIMETRY_SCRGB,
>> > +	COLORIMETRY_DCI_P3,
>> > +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
>> > +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, };
>> > +
>> >  struct drm_mode_modeinfo {
>> >  	__u32 clock;
>> >  	__u16 hdisplay;
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 1/2] drm: Add colorspace property

[Jonas Karlman]

On 2018-11-02 15:13, Shankar, Uma wrote:
>
>> -----Original Message-----
>> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> Sent: Friday, November 2, 2018 5:00 PM
>> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
>> intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>> Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil
>> <hansverk@cisco.com>
>> Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property
>>
>> On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
>>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>>>> This patch adds a colorspace property enabling userspace to switch
>>>> to various supported colorspaces.
>>>> This will help enable BT2020 along with other colorspaces.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>>>>  drivers/gpu/drm/drm_connector.c   | 44
>> +++++++++++++++++++++++++++++++++++++++
>>>>  include/drm/drm_connector.h       |  7 +++++++
>>>>  include/drm/drm_mode_config.h     |  6 ++++++
>>>>  include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>>>>  5 files changed, 85 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index d5b7f31..9e1d46b 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct
>> drm_connector *connector,
>>>>  		state->picture_aspect_ratio = val;
>>>>  	} else if (property == config->content_type_property) {
>>>>  		state->content_type = val;
>>>> +	} else if (property == config->colorspace_property) {
>>>> +		state->colorspace = val;
>>>>  	} else if (property == connector->scaling_mode_property) {
>>>>  		state->scaling_mode = val;
>>>>  	} else if (property == connector->content_protection_property) {
>>>> @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -826,6 +826,38 @@ 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 colorspace[] = {
>>>> +	/* Standard Definition Colorimetry based on CEA 861 */
>>>> +	{ COLORIMETRY_ITU_601, "601" },
>>>> +	{ COLORIMETRY_ITU_709, "709" },
>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ COLORIMETRY_XV_YCC_601, "601" },
>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>>> +	{ COLORIMETRY_XV_YCC_709, "709" },
>>> 2 definitions that share the same name?
>>> All in all, I think the enum strings need to be more descriptive and self-
>> consistent.
>> +1
> Sure, will modify this.
>
>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>>> +	{ COLORIMETRY_S_YCC_601, "s601" },
>>>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
>>>> +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>> Hans (cc:d) recetly noted that these things aren't called Adobe<something>
>> anymore in the CTA-861 spec. There is some new name for them, which I now
>> forget.
> EC2 EC1 EC0 Extended Colorimetry
> 0       1      1      AdobeYCC601  
> This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks
> like they are still keeping it that way. Not sure if its updated as part of any latest spec
> update.

New names is opRGB and opYCC601 according to the notice on the first page of CTA-861-G [1]

Updated CTA-861-E/F/G can be found at https://standards.cta.tech/kwspub/published_docs/

[1] https://standards.cta.tech/kwspub/published_docs/CTA-861-G_FINAL_revised_2017.pdf

>  
>>>> +	/* Colorimetry based on IEC 61966-2-5 */
>>>> +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
>>>> +	/* Colorimetry based on ITU-R BT.2020 */
>>>> +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
>>>> +	/* Colorimetry based on ITU-R BT.2020 */
>>>> +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
>>>> +	/* Colorimetry based on ITU-R BT.2020 */
>>>> +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
>>>> +	/* DP MSA Colorimetry */
>>>> +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
>>>> +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
>>>> +	{ COLORIMETRY_SRGB, "SRGB" },
>>> sRGB
> Was trying to avoid camelcase, but for these names, I guess we can keep how
> spec defines them. Will change this.
>
>>>> +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
>>>> +	{ COLORIMETRY_SCRGB, "SCRGB" },
>>> scRGB?
> Will update this.
>
>>>> +	{ COLORIMETRY_DCI_P3, "DCIP3" },
>>> DCI-P3?
> Ok, will update
>
>>>> +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
>>> ^Typo
> Yeah, will rectify this.
>
>>>> +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
>>>> +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
>>> This enum together with the code below is broken.
>>>
>>> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
>>>
>>> I would just make it COLORIMETRY_UNSET = "Unset".
>> "Unset" might work. Though feels a bit strange to me. Other ideas would be
>> "Default", "Automatic", "Undefined" or something along those lines.
>> Ideally it should convey the meaning of "the driver will pick this for you", and for
>> that I'd lean towards "Default" or "Automatic".
> Ok, will change this to Default.
>
>>> To set the default colorimetry for all drivers, just make the default
>>> value 0 (preferred), or add it to __drm_atomic_helper_connector_reset().
> Ok, will modify this and resend the next version.
>
> Thanks Ville and Maarten for your review and suggestions.
>
> Regards,
> Uma Shankar
>>>> +};
>>>> +
>>>>  /**
>>>>   * DOC: standard connector properties
>>>>   *
>>>> @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct
>> drm_display_info *info,
>>>>   *	can also expose this property to external outputs, in which case they
>>>>   *	must support "None", which should be the default (since external screens
>>>>   *	have a built-in scaler).
>>>> + *
>>>> + * colorspace:
>>>> + *	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.
>>>>   */
>>>>
>>>>  int drm_connector_create_standard_properties(struct drm_device
>>>> *dev) @@ -1020,6 +1058,12 @@ int
>> drm_connector_create_standard_properties(struct drm_device *dev)
>>>>  		return -ENOMEM;
>>>>  	dev->mode_config.non_desktop_property = prop;
>>>>
>>>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>> "Colorspace",
>>>> +					colorspace, ARRAY_SIZE(colorspace));
>>>> +	if (!prop)
>>>> +		return -ENOMEM;
>>>> +	dev->mode_config.colorspace_property = prop;
>>>> +
>>>>  	return 0;
>>>>  }
>>>>
>>>> diff --git a/include/drm/drm_connector.h
>>>> b/include/drm/drm_connector.h index dd0552c..b7f5373 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -497,6 +497,13 @@ struct drm_connector_state {
>>>>  	unsigned int content_protection;
>>>>
>>>>  	/**
>>>> +	 * @colorspace: Connector property to request colorspace
>>>> +	 * change. This is most commonly used to switch to wider color
>>>> +	 * gamuts like BT2020.
>>>> +	 */
>>>> +	enum encoder_colorimetry colorspace;
>>>> +
>>>> +	/**
>>>>  	 * @writeback_job: Writeback job for writeback connectors
>>>>  	 *
>>>>  	 * Holds the framebuffer and out-fence for a writeback connector.
>>>> As diff --git a/include/drm/drm_mode_config.h
>>>> b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -863,6 +863,12 @@ struct drm_mode_config {
>>>>  	uint32_t cursor_width, cursor_height;
>>>>
>>>>  	/**
>>>> +	 * @colorspace_property: Connector property to set the suitable
>>>> +	 * colorspace supported by the sink.
>>>> +	 */
>>>> +	struct drm_property *colorspace_property;
>>>> +
>>>> +	/**
>>>>  	 * @suspend_state:
>>>>  	 *
>>>>  	 * Atomic state when suspended.
>>>> diff --git a/include/uapi/drm/drm_mode.h
>>>> b/include/uapi/drm/drm_mode.h index d3e0fe3..831cc77 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -210,6 +210,30 @@
>>>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>>>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>>>
>>>> +enum encoder_colorimetry {
>>>> +	/* CEA 861 Normal Colorimetry options */
>>>> +	COLORIMETRY_ITU_601 = 0,
>>>> +	COLORIMETRY_ITU_709,
>>>> +	/* CEA 861 Extended Colorimetry Options */
>>>> +	COLORIMETRY_XV_YCC_601,
>>>> +	COLORIMETRY_XV_YCC_709,
>>>> +	COLORIMETRY_S_YCC_601,
>>>> +	COLORIMETRY_ADOBE_YCC_601,
>>>> +	COLORIMETRY_ADOBE_RGB,
>>>> +	COLORIMETRY_BT2020_RGB,
>>>> +	COLORIMETRY_BT2020_YCC,
>>>> +	COLORIMETRY_BT2020_CYCC,
>>>> +	/* DP MSA Colorimetry Options */
>>>> +	COLORIMETRY_Y_CBCR_ITU_601,
>>>> +	COLORIMETRY_Y_CBCR_ITU_709,
>>>> +	COLORIMETRY_SRGB,
>>>> +	COLORIMETRY_RGB_WIDE_GAMUT,
>>>> +	COLORIMETRY_SCRGB,
>>>> +	COLORIMETRY_DCI_P3,
>>>> +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
>>>> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, };
>>>> +
>>>>  struct drm_mode_modeinfo {
>>>>  	__u32 clock;
>>>>  	__u16 hdisplay;
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&amp;sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&amp;reserved=0
>> --
>> Ville Syrjälä
>> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&amp;sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&amp;reserved=0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property

[Hans Verkuil]

On 11/02/2018 03:29 PM, Jonas Karlman wrote:
> On 2018-11-02 15:13, Shankar, Uma wrote:
>>
>>> -----Original Message-----
>>> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>>> Sent: Friday, November 2, 2018 5:00 PM
>>> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
>>> intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>>> Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil
>>> <hansverk@cisco.com>
>>> Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property
>>>
>>> On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote:
>>>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>>>>> This patch adds a colorspace property enabling userspace to switch
>>>>> to various supported colorspaces.
>>>>> This will help enable BT2020 along with other colorspaces.
>>>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>>>>>   drivers/gpu/drm/drm_connector.c   | 44
>>> +++++++++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_connector.h       |  7 +++++++
>>>>>   include/drm/drm_mode_config.h     |  6 ++++++
>>>>>   include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>>>>>   5 files changed, 85 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> index d5b7f31..9e1d46b 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>>> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct
>>> drm_connector *connector,
>>>>>   		state->picture_aspect_ratio = val;
>>>>>   	} else if (property == config->content_type_property) {
>>>>>   		state->content_type = val;
>>>>> +	} else if (property == config->colorspace_property) {
>>>>> +		state->colorspace = val;
>>>>>   	} else if (property == connector->scaling_mode_property) {
>>>>>   		state->scaling_mode = val;
>>>>>   	} else if (property == connector->content_protection_property) {
>>>>> @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -826,6 +826,38 @@ 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 colorspace[] = {
>>>>> +	/* Standard Definition Colorimetry based on CEA 861 */
>>>>> +	{ COLORIMETRY_ITU_601, "601" },
>>>>> +	{ COLORIMETRY_ITU_709, "709" },
>>>>> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>>>> +	{ COLORIMETRY_XV_YCC_601, "601" },
>>>>> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>>>> +	{ COLORIMETRY_XV_YCC_709, "709" },
>>>> 2 definitions that share the same name?
>>>> All in all, I think the enum strings need to be more descriptive and self-
>>> consistent.
>>> +1
>> Sure, will modify this.
>>
>>>>> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>>>> +	{ COLORIMETRY_S_YCC_601, "s601" },
>>>>> +	/* Colorimetry based on IEC 61966-2-5 [33] */
>>>>> +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>>> Hans (cc:d) recetly noted that these things aren't called Adobe<something>
>>> anymore in the CTA-861 spec. There is some new name for them, which I now
>>> forget.
>> EC2 EC1 EC0 Extended Colorimetry
>> 0       1      1      AdobeYCC601
>> This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks
>> like they are still keeping it that way. Not sure if its updated as part of any latest spec
>> update.
> 
> New names is opRGB and opYCC601 according to the notice on the first page of CTA-861-G [1]
> 
> Updated CTA-861-E/F/G can be found at https://standards.cta.tech/kwspub/published_docs/
> 
> [1] https://standards.cta.tech/kwspub/published_docs/CTA-861-G_FINAL_revised_2017.pdf

Correct. The names were updated since Adobe complained to the CTA about 
trademark issues. And in all fairness, it makes sense to refer to the 
official international standard name instead of a company standard, even
though they are effectively identical.

Basically, just avoid the name 'Adobe' :-)

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v2 1/2] drm: Add colorspace property

[Sharma, Shashank]

Regards

Shashank


On 10/31/2018 5:35 PM, Uma Shankar wrote:
> This patch adds a colorspace property enabling
> userspace to switch to various supported colorspaces.
> This will help enable BT2020 along with other colorspaces.
>
> 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.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>   drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_connector.h       |  7 +++++++
>   include/drm/drm_mode_config.h     |  6 ++++++
>   include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>   5 files changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f31..9e1d46b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>   		state->picture_aspect_ratio = val;
>   	} else if (property == config->content_type_property) {
>   		state->content_type = val;
> +	} else if (property == config->colorspace_property) {
> +		state->colorspace = val;
>   	} else if (property == connector->scaling_mode_property) {
>   		state->scaling_mode = val;
>   	} else if (property == connector->content_protection_property) {
> @@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -826,6 +826,38 @@ 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 colorspace[] = {
> +	/* Standard Definition Colorimetry based on CEA 861 */
> +	{ COLORIMETRY_ITU_601, "601" },
> +	{ COLORIMETRY_ITU_709, "709" },
> +	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_601, "601" },
> +	/* High Definition Colorimetry based on IEC 61966-2-4 */
> +	{ COLORIMETRY_XV_YCC_709, "709" },
> +	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> +	{ COLORIMETRY_S_YCC_601, "s601" },
> +	/* Colorimetry based on IEC 61966-2-5 [33] */
> +	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
> +	/* Colorimetry based on IEC 61966-2-5 */
> +	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
> +	/* Colorimetry based on ITU-R BT.2020 */
> +	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
> +	/* DP MSA Colorimetry */
> +	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
> +	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
> +	{ COLORIMETRY_SRGB, "SRGB" },
> +	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
> +	{ COLORIMETRY_SCRGB, "SCRGB" },
> +	{ COLORIMETRY_DCI_P3, "DCIP3" },
> +	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
> +	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
> +	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
> +};
> +
>   /**
>    * DOC: standard connector properties
>    *
> @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>    *	can also expose this property to external outputs, in which case they
>    *	must support "None", which should be the default (since external screens
>    *	have a built-in scaler).
> + *
> + * colorspace:
> + *	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.
As commented on the previous patch, we might have to consider the HW 
capability while doing this.
>    */
>   
>   int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.non_desktop_property = prop;
>   
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
> +					colorspace, ARRAY_SIZE(colorspace));
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.colorspace_property = prop;
> +
>   	return 0;
>   }
>   
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index dd0552c..b7f5373 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -497,6 +497,13 @@ struct drm_connector_state {
>   	unsigned int content_protection;
>   
>   	/**
> +	 * @colorspace: Connector property to request colorspace
> +	 * change. This is most commonly used to switch to wider color
> +	 * gamuts like BT2020.
> +	 */
> +	enum encoder_colorimetry colorspace;
A connector state property with enum name encoder_colorimetry is a bit 
confusing, but I might be nitpicking ;-)
>   
> +
> +	/**
>   	 * @writeback_job: Writeback job for writeback connectors
>   	 *
>   	 * Holds the framebuffer and out-fence for a writeback connector. As
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index d643d26..a6eb0aa 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -863,6 +863,12 @@ struct drm_mode_config {
>   	uint32_t cursor_width, cursor_height;
>   
>   	/**
> +	 * @colorspace_property: Connector property to set the suitable
> +	 * colorspace supported by the sink.
> +	 */
> +	struct drm_property *colorspace_property;
> +
> +	/**
>   	 * @suspend_state:
>   	 *
>   	 * Atomic state when suspended.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe3..831cc77 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -210,6 +210,30 @@
>   #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>   #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>   
> +enum encoder_colorimetry {
> +	/* CEA 861 Normal Colorimetry options */
> +	COLORIMETRY_ITU_601 = 0,
> +	COLORIMETRY_ITU_709,
> +	/* CEA 861 Extended Colorimetry Options */
> +	COLORIMETRY_XV_YCC_601,
> +	COLORIMETRY_XV_YCC_709,
> +	COLORIMETRY_S_YCC_601,
> +	COLORIMETRY_ADOBE_YCC_601,
> +	COLORIMETRY_ADOBE_RGB,
> +	COLORIMETRY_BT2020_RGB,
> +	COLORIMETRY_BT2020_YCC,
> +	COLORIMETRY_BT2020_CYCC,
> +	/* DP MSA Colorimetry Options */
> +	COLORIMETRY_Y_CBCR_ITU_601,
> +	COLORIMETRY_Y_CBCR_ITU_709,
> +	COLORIMETRY_SRGB,
> +	COLORIMETRY_RGB_WIDE_GAMUT,
> +	COLORIMETRY_SCRGB,
> +	COLORIMETRY_DCI_P3,
> +	COLORIMETRY_CUSTOM_COLOR_PROFILE,
> +	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
> +};
> +
>   struct drm_mode_modeinfo {
>   	__u32 clock;
>   	__u16 hdisplay;
- Shashank
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 1/2] drm: Add colorspace property

[Brian Starkey]

Hi Uma,

On Wed, Oct 31, 2018 at 05:35:45PM +0530, Uma Shankar wrote:
>This patch adds a colorspace property enabling
>userspace to switch to various supported colorspaces.
>This will help enable BT2020 along with other colorspaces.
>
>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.
>
>Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>---
> drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
> drivers/gpu/drm/drm_connector.c   | 44 +++++++++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h       |  7 +++++++
> include/drm/drm_mode_config.h     |  6 ++++++
> include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
> 5 files changed, 85 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>index d5b7f31..9e1d46b 100644
>--- a/drivers/gpu/drm/drm_atomic_uapi.c
>+++ b/drivers/gpu/drm/drm_atomic_uapi.c
>@@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> 		state->picture_aspect_ratio = val;
> 	} else if (property == config->content_type_property) {
> 		state->content_type = val;
>+	} else if (property == config->colorspace_property) {
>+		state->colorspace = val;
> 	} else if (property == connector->scaling_mode_property) {
> 		state->scaling_mode = val;
> 	} else if (property == connector->content_protection_property) {
>@@ -795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
>--- a/drivers/gpu/drm/drm_connector.c
>+++ b/drivers/gpu/drm/drm_connector.c
>@@ -826,6 +826,38 @@ 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 colorspace[] = {
>+	/* Standard Definition Colorimetry based on CEA 861 */
>+	{ COLORIMETRY_ITU_601, "601" },
>+	{ COLORIMETRY_ITU_709, "709" },
>+	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>+	{ COLORIMETRY_XV_YCC_601, "601" },
>+	/* High Definition Colorimetry based on IEC 61966-2-4 */
>+	{ COLORIMETRY_XV_YCC_709, "709" },
>+	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>+	{ COLORIMETRY_S_YCC_601, "s601" },
>+	/* Colorimetry based on IEC 61966-2-5 [33] */
>+	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>+	/* Colorimetry based on IEC 61966-2-5 */
>+	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
>+	/* Colorimetry based on ITU-R BT.2020 */
>+	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
>+	/* Colorimetry based on ITU-R BT.2020 */
>+	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
>+	/* Colorimetry based on ITU-R BT.2020 */
>+	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
>+	/* DP MSA Colorimetry */
>+	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
>+	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
>+	{ COLORIMETRY_SRGB, "SRGB" },
>+	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
>+	{ COLORIMETRY_SCRGB, "SCRGB" },
>+	{ COLORIMETRY_DCI_P3, "DCIP3" },
>+	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
>+	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
>+	{ COLORIMETRY_DEFAULT, "Default: ITU_709" },
>+};
>+
> /**
>  * DOC: standard connector properties
>  *
>@@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
>  *	can also expose this property to external outputs, in which case they
>  *	must support "None", which should be the default (since external screens
>  *	have a built-in scaler).
>+ *
>+ * colorspace:

Is it "colorspace" or "Colorspace"? The code says capital-C, the doc
(and whatever little convention there is) says lower-case.

>+ *	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.
>  */
>
> int drm_connector_create_standard_properties(struct drm_device *dev)
>@@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
> 		return -ENOMEM;
> 	dev->mode_config.non_desktop_property = prop;
>
>+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
>+					colorspace, ARRAY_SIZE(colorspace));

Similar to what others have said: I'm not sure it's a great idea to
expose the full set by default. At a minimum, actually applying this
property requires some control over the HDMI/DP transmitter doesn't
it? And I believe they vary in capabilities?

Further, I'd expect some of them to require more extensive changes
further through the pipe (e.g. scRGB), though I admit I don't know
much about how Sinks handle this stuff.

Do you have a subset which you have a real user and use-case for which
we might settle on before throwing them all in and making it UAPI?

Cheers,
-Brian


>+	if (!prop)
>+		return -ENOMEM;
>+	dev->mode_config.colorspace_property = prop;
>+
> 	return 0;
> }
>
>diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>index dd0552c..b7f5373 100644
>--- a/include/drm/drm_connector.h
>+++ b/include/drm/drm_connector.h
>@@ -497,6 +497,13 @@ struct drm_connector_state {
> 	unsigned int content_protection;
>
> 	/**
>+	 * @colorspace: Connector property to request colorspace
>+	 * change. This is most commonly used to switch to wider color
>+	 * gamuts like BT2020.
>+	 */
>+	enum encoder_colorimetry colorspace;
>+
>+	/**
> 	 * @writeback_job: Writeback job for writeback connectors
> 	 *
> 	 * Holds the framebuffer and out-fence for a writeback connector. As
>diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>index d643d26..a6eb0aa 100644
>--- a/include/drm/drm_mode_config.h
>+++ b/include/drm/drm_mode_config.h
>@@ -863,6 +863,12 @@ struct drm_mode_config {
> 	uint32_t cursor_width, cursor_height;
>
> 	/**
>+	 * @colorspace_property: Connector property to set the suitable
>+	 * colorspace supported by the sink.
>+	 */
>+	struct drm_property *colorspace_property;
>+
>+	/**
> 	 * @suspend_state:
> 	 *
> 	 * Atomic state when suspended.
>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>index d3e0fe3..831cc77 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -210,6 +210,30 @@
> #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>
>+enum encoder_colorimetry {
>+	/* CEA 861 Normal Colorimetry options */
>+	COLORIMETRY_ITU_601 = 0,
>+	COLORIMETRY_ITU_709,
>+	/* CEA 861 Extended Colorimetry Options */
>+	COLORIMETRY_XV_YCC_601,
>+	COLORIMETRY_XV_YCC_709,
>+	COLORIMETRY_S_YCC_601,
>+	COLORIMETRY_ADOBE_YCC_601,
>+	COLORIMETRY_ADOBE_RGB,
>+	COLORIMETRY_BT2020_RGB,
>+	COLORIMETRY_BT2020_YCC,
>+	COLORIMETRY_BT2020_CYCC,
>+	/* DP MSA Colorimetry Options */
>+	COLORIMETRY_Y_CBCR_ITU_601,
>+	COLORIMETRY_Y_CBCR_ITU_709,
>+	COLORIMETRY_SRGB,
>+	COLORIMETRY_RGB_WIDE_GAMUT,
>+	COLORIMETRY_SCRGB,
>+	COLORIMETRY_DCI_P3,
>+	COLORIMETRY_CUSTOM_COLOR_PROFILE,
>+	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709,
>+};
>+
> struct drm_mode_modeinfo {
> 	__u32 clock;
> 	__u16 hdisplay;
>-- 
>1.9.1
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [v2 1/2] drm: Add colorspace property

[Shankar, Uma]

>-----Original Message-----
>From: Brian Starkey [mailto:Brian.Starkey@arm.com]
>Sent: Tuesday, November 20, 2018 9:06 PM
>To: Shankar, Uma <uma.shankar@intel.com>; Syrjala, Ville
><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>;
>emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org
>Cc: nd <nd@arm.com>
>Subject: Re: [v2 1/2] drm: Add colorspace property
>
>Hi Uma,
>
>On Wed, Oct 31, 2018 at 05:35:45PM +0530, Uma Shankar wrote:
>>This patch adds a colorspace property enabling userspace to switch to
>>various supported colorspaces.
>>This will help enable BT2020 along with other colorspaces.
>>
>>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.
>>
>>Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>---
>> drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
>> drivers/gpu/drm/drm_connector.c   | 44
>+++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_connector.h       |  7 +++++++
>> include/drm/drm_mode_config.h     |  6 ++++++
>> include/uapi/drm/drm_mode.h       | 24 +++++++++++++++++++++
>> 5 files changed, 85 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>b/drivers/gpu/drm/drm_atomic_uapi.c
>>index d5b7f31..9e1d46b 100644
>>--- a/drivers/gpu/drm/drm_atomic_uapi.c
>>+++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>@@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct
>drm_connector *connector,
>> 		state->picture_aspect_ratio = val;
>> 	} else if (property == config->content_type_property) {
>> 		state->content_type = val;
>>+	} else if (property == config->colorspace_property) {
>>+		state->colorspace = val;
>> 	} else if (property == connector->scaling_mode_property) {
>> 		state->scaling_mode = val;
>> 	} else if (property == connector->content_protection_property) { @@
>>-795,6 +797,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 == config->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 aa18b1d..5ad52a0 100644
>>--- a/drivers/gpu/drm/drm_connector.c
>>+++ b/drivers/gpu/drm/drm_connector.c
>>@@ -826,6 +826,38 @@ 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 colorspace[] = {
>>+	/* Standard Definition Colorimetry based on CEA 861 */
>>+	{ COLORIMETRY_ITU_601, "601" },
>>+	{ COLORIMETRY_ITU_709, "709" },
>>+	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
>>+	{ COLORIMETRY_XV_YCC_601, "601" },
>>+	/* High Definition Colorimetry based on IEC 61966-2-4 */
>>+	{ COLORIMETRY_XV_YCC_709, "709" },
>>+	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>>+	{ COLORIMETRY_S_YCC_601, "s601" },
>>+	/* Colorimetry based on IEC 61966-2-5 [33] */
>>+	{ COLORIMETRY_ADOBE_YCC_601, "adobe601" },
>>+	/* Colorimetry based on IEC 61966-2-5 */
>>+	{ COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
>>+	/* Colorimetry based on ITU-R BT.2020 */
>>+	{ COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
>>+	/* Colorimetry based on ITU-R BT.2020 */
>>+	{ COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
>>+	/* Colorimetry based on ITU-R BT.2020 */
>>+	{ COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
>>+	/* DP MSA Colorimetry */
>>+	{ COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" },
>>+	{ COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" },
>>+	{ COLORIMETRY_SRGB, "SRGB" },
>>+	{ COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" },
>>+	{ COLORIMETRY_SCRGB, "SCRGB" },
>>+	{ COLORIMETRY_DCI_P3, "DCIP3" },
>>+	{ COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" },
>>+	/* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */
>>+	{ COLORIMETRY_DEFAULT, "Default: ITU_709" }, };
>>+
>> /**
>>  * DOC: standard connector properties
>>  *
>>@@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct
>drm_display_info *info,
>>  *	can also expose this property to external outputs, in which case they
>>  *	must support "None", which should be the default (since external screens
>>  *	have a built-in scaler).
>>+ *
>>+ * colorspace:
>
>Is it "colorspace" or "Colorspace"? The code says capital-C, the doc (and
>whatever little convention there is) says lower-case.

I will update this. It should be "Colorspace".

>>+ *	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.
>>  */
>>
>> int drm_connector_create_standard_properties(struct drm_device *dev)
>>@@ -1020,6 +1058,12 @@ int
>drm_connector_create_standard_properties(struct drm_device *dev)
>> 		return -ENOMEM;
>> 	dev->mode_config.non_desktop_property = prop;
>>
>>+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>"Colorspace",
>>+					colorspace, ARRAY_SIZE(colorspace));
>
>Similar to what others have said: I'm not sure it's a great idea to expose the full
>set by default. At a minimum, actually applying this property requires some
>control over the HDMI/DP transmitter doesn't it? And I believe they vary in
>capabilities?
>
>Further, I'd expect some of them to require more extensive changes further
>through the pipe (e.g. scRGB), though I admit I don't know much about how Sinks
>handle this stuff.

I am planning to separate out HDMI and DP as separate properties (same name for
userspace), but with connector specific colorspaces in the list. Will float the v4 soon, please
have a look and share your feedback.

Regards,
Uma Shankar

>Do you have a subset which you have a real user and use-case for which we might
>settle on before throwing them all in and making it UAPI?
>
>Cheers,
>-Brian
>
>
>>+	if (!prop)
>>+		return -ENOMEM;
>>+	dev->mode_config.colorspace_property = prop;
>>+
>> 	return 0;
>> }
>>
>>diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>index dd0552c..b7f5373 100644
>>--- a/include/drm/drm_connector.h
>>+++ b/include/drm/drm_connector.h
>>@@ -497,6 +497,13 @@ struct drm_connector_state {
>> 	unsigned int content_protection;
>>
>> 	/**
>>+	 * @colorspace: Connector property to request colorspace
>>+	 * change. This is most commonly used to switch to wider color
>>+	 * gamuts like BT2020.
>>+	 */
>>+	enum encoder_colorimetry colorspace;
>>+
>>+	/**
>> 	 * @writeback_job: Writeback job for writeback connectors
>> 	 *
>> 	 * Holds the framebuffer and out-fence for a writeback connector. As
>>diff --git a/include/drm/drm_mode_config.h
>>b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644
>>--- a/include/drm/drm_mode_config.h
>>+++ b/include/drm/drm_mode_config.h
>>@@ -863,6 +863,12 @@ struct drm_mode_config {
>> 	uint32_t cursor_width, cursor_height;
>>
>> 	/**
>>+	 * @colorspace_property: Connector property to set the suitable
>>+	 * colorspace supported by the sink.
>>+	 */
>>+	struct drm_property *colorspace_property;
>>+
>>+	/**
>> 	 * @suspend_state:
>> 	 *
>> 	 * Atomic state when suspended.
>>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>index d3e0fe3..831cc77 100644
>>--- a/include/uapi/drm/drm_mode.h
>>+++ b/include/uapi/drm/drm_mode.h
>>@@ -210,6 +210,30 @@
>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>
>>+enum encoder_colorimetry {
>>+	/* CEA 861 Normal Colorimetry options */
>>+	COLORIMETRY_ITU_601 = 0,
>>+	COLORIMETRY_ITU_709,
>>+	/* CEA 861 Extended Colorimetry Options */
>>+	COLORIMETRY_XV_YCC_601,
>>+	COLORIMETRY_XV_YCC_709,
>>+	COLORIMETRY_S_YCC_601,
>>+	COLORIMETRY_ADOBE_YCC_601,
>>+	COLORIMETRY_ADOBE_RGB,
>>+	COLORIMETRY_BT2020_RGB,
>>+	COLORIMETRY_BT2020_YCC,
>>+	COLORIMETRY_BT2020_CYCC,
>>+	/* DP MSA Colorimetry Options */
>>+	COLORIMETRY_Y_CBCR_ITU_601,
>>+	COLORIMETRY_Y_CBCR_ITU_709,
>>+	COLORIMETRY_SRGB,
>>+	COLORIMETRY_RGB_WIDE_GAMUT,
>>+	COLORIMETRY_SCRGB,
>>+	COLORIMETRY_DCI_P3,
>>+	COLORIMETRY_CUSTOM_COLOR_PROFILE,
>>+	COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, };
>>+
>> struct drm_mode_modeinfo {
>> 	__u32 clock;
>> 	__u16 hdisplay;
>>--
>>1.9.1
>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Uma Shankar]

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.

v2: Merged the changes of creating infoframe as well to this
patch as per Maarten's suggestion.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c | 1 +
 drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index a5a2c8f..35ef70a 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -125,6 +125,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_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 129b880..8a41fb3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
 	else
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
 
+	frame.avi.extended_colorimetry = conn_state->colorspace;
+
 	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
 					   crtc_state->limited_color_range ?
 					   HDMI_QUANTIZATION_RANGE_LIMITED :
@@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 	intel_attach_broadcast_rgb_property(connector);
 	intel_attach_aspect_ratio_property(connector);
 	drm_connector_attach_content_type_property(connector);
+	drm_object_attach_property(&connector->base,
+			connector->dev->mode_config.colorspace_property,
+			COLORIMETRY_ITU_709);
 	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 }
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

Op 31-10-18 om 13:05 schreef Uma Shankar:
> 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.
>
> v2: Merged the changes of creating infoframe as well to this
> patch as per Maarten's suggestion.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index a5a2c8f..35ef70a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -125,6 +125,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_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 129b880..8a41fb3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  
> +	frame.avi.extended_colorimetry = conn_state->colorspace;
> +
>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>  					   crtc_state->limited_color_range ?
>  					   HDMI_QUANTIZATION_RANGE_LIMITED :
> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  	intel_attach_broadcast_rgb_property(connector);
>  	intel_attach_aspect_ratio_property(connector);
>  	drm_connector_attach_content_type_property(connector);
> +	drm_object_attach_property(&connector->base,
> +			connector->dev->mode_config.colorspace_property,
> +			COLORIMETRY_ITU_709);
Just put 0 here..
If you want to init the default colorspace, put it in the first patch.

We should perhaps hide color spaces that are not supported on HDMI?
>  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>  }
>  


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shankar, Uma]

>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>Sent: Friday, November 2, 2018 2:53 PM
>To: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
>intel-gfx@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; ajax@redhat.com; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable
>modeset
>
>Op 31-10-18 om 13:05 schreef Uma Shankar:
>> 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.
>>
>> v2: Merged the changes of creating infoframe as well to this patch as
>> per Maarten's suggestion.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
>>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index a5a2c8f..35ef70a 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -125,6 +125,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_hdmi.c
>> b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 129b880..8a41fb3 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct
>intel_encoder *encoder,
>>  	else
>>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>
>> +	frame.avi.extended_colorimetry = conn_state->colorspace;
>> +
>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>>  					   crtc_state->limited_color_range ?
>>
>HDMI_QUANTIZATION_RANGE_LIMITED :
>> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector
>*connector)
>>  	intel_attach_broadcast_rgb_property(connector);
>>  	intel_attach_aspect_ratio_property(connector);
>>  	drm_connector_attach_content_type_property(connector);
>> +	drm_object_attach_property(&connector->base,
>> +			connector->dev->mode_config.colorspace_property,
>> +			COLORIMETRY_ITU_709);
>Just put 0 here..
>If you want to init the default colorspace, put it in the first patch.

Ok, will update this.

>We should perhaps hide color spaces that are not supported on HDMI?

Currently the supported colorspaces will be picked from edid by userspace and
they should use the current property interface to set the one which is supported.
Even on HDMI, some connectors may not support certain colorspace, so keeping
it on userspace to set the one which is supported by the particular connector. Hope
this approach is fine ?

Regards,
Uma Shankar

>>  	connector->state->picture_aspect_ratio =
>HDMI_PICTURE_ASPECT_NONE;
>> }
>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable modeset

[Ville Syrjälä]

On Fri, Nov 02, 2018 at 02:18:42PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >Sent: Friday, November 2, 2018 2:53 PM
> >To: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
> >intel-gfx@lists.freedesktop.org
> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; ajax@redhat.com; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable
> >modeset
> >
> >Op 31-10-18 om 13:05 schreef Uma Shankar:
> >> 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.
> >>
> >> v2: Merged the changes of creating infoframe as well to this patch as
> >> per Maarten's suggestion.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
> >>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
> >>  2 files changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >> index a5a2c8f..35ef70a 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -125,6 +125,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_hdmi.c
> >> b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index 129b880..8a41fb3 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct
> >intel_encoder *encoder,
> >>  	else
> >>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >>
> >> +	frame.avi.extended_colorimetry = conn_state->colorspace;
> >> +
> >>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
> >>  					   crtc_state->limited_color_range ?
> >>
> >HDMI_QUANTIZATION_RANGE_LIMITED :
> >> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector
> >*connector)
> >>  	intel_attach_broadcast_rgb_property(connector);
> >>  	intel_attach_aspect_ratio_property(connector);
> >>  	drm_connector_attach_content_type_property(connector);
> >> +	drm_object_attach_property(&connector->base,
> >> +			connector->dev->mode_config.colorspace_property,
> >> +			COLORIMETRY_ITU_709);
> >Just put 0 here..
> >If you want to init the default colorspace, put it in the first patch.
> 
> Ok, will update this.
> 
> >We should perhaps hide color spaces that are not supported on HDMI?
> 
> Currently the supported colorspaces will be picked from edid by userspace and
> they should use the current property interface to set the one which is supported.
> Even on HDMI, some connectors may not support certain colorspace, so keeping
> it on userspace to set the one which is supported by the particular connector. Hope
> this approach is fine ?

I think we want to trim the list to whatever the infoframe vs. MSA/VSC
SDP can carry. So HDMI will have one list, DP another. And I guess for
lspcon we want to go with the HDMI definition since we populate the
infoframe by hand.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

Op 02-11-18 om 16:41 schreef Ville Syrjälä:
> On Fri, Nov 02, 2018 at 02:18:42PM +0000, Shankar, Uma wrote:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>> Sent: Friday, November 2, 2018 2:53 PM
>>> To: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
>>> intel-gfx@lists.freedesktop.org
>>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; ajax@redhat.com; Lankhorst, Maarten
>>> <maarten.lankhorst@intel.com>
>>> Subject: Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable
>>> modeset
>>>
>>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>>>> 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.
>>>>
>>>> v2: Merged the changes of creating infoframe as well to this patch as
>>>> per Maarten's suggestion.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
>>>>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index a5a2c8f..35ef70a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -125,6 +125,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_hdmi.c
>>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index 129b880..8a41fb3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct
>>> intel_encoder *encoder,
>>>>  	else
>>>>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>
>>>> +	frame.avi.extended_colorimetry = conn_state->colorspace;
>>>> +
>>>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>>>>  					   crtc_state->limited_color_range ?
>>>>
>>> HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector
>>> *connector)
>>>>  	intel_attach_broadcast_rgb_property(connector);
>>>>  	intel_attach_aspect_ratio_property(connector);
>>>>  	drm_connector_attach_content_type_property(connector);
>>>> +	drm_object_attach_property(&connector->base,
>>>> +			connector->dev->mode_config.colorspace_property,
>>>> +			COLORIMETRY_ITU_709);
>>> Just put 0 here..
>>> If you want to init the default colorspace, put it in the first patch.
>> Ok, will update this.
>>
>>> We should perhaps hide color spaces that are not supported on HDMI?
>> Currently the supported colorspaces will be picked from edid by userspace and
>> they should use the current property interface to set the one which is supported.
>> Even on HDMI, some connectors may not support certain colorspace, so keeping
>> it on userspace to set the one which is supported by the particular connector. Hope
>> this approach is fine ?
> I think we want to trim the list to whatever the infoframe vs. MSA/VSC
> SDP can carry. So HDMI will have one list, DP another. And I guess for
> lspcon we want to go with the HDMI definition since we populate the
> infoframe by hand.
>
What about passive DP to HDMI convertors?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Nov 02, 2018 at 04:44:10PM +0100, Maarten Lankhorst wrote:
> Op 02-11-18 om 16:41 schreef Ville Syrjälä:
> > On Fri, Nov 02, 2018 at 02:18:42PM +0000, Shankar, Uma wrote:
> >>
> >>> -----Original Message-----
> >>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >>> Sent: Friday, November 2, 2018 2:53 PM
> >>> To: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
> >>> intel-gfx@lists.freedesktop.org
> >>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; ajax@redhat.com; Lankhorst, Maarten
> >>> <maarten.lankhorst@intel.com>
> >>> Subject: Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable
> >>> modeset
> >>>
> >>> Op 31-10-18 om 13:05 schreef Uma Shankar:
> >>>> 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.
> >>>>
> >>>> v2: Merged the changes of creating infoframe as well to this patch as
> >>>> per Maarten's suggestion.
> >>>>
> >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
> >>>>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
> >>>>  2 files changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >>>> b/drivers/gpu/drm/i915/intel_atomic.c
> >>>> index a5a2c8f..35ef70a 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>>> @@ -125,6 +125,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_hdmi.c
> >>>> b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> index 129b880..8a41fb3 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct
> >>> intel_encoder *encoder,
> >>>>  	else
> >>>>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >>>>
> >>>> +	frame.avi.extended_colorimetry = conn_state->colorspace;
> >>>> +
> >>>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
> >>>>  					   crtc_state->limited_color_range ?
> >>>>
> >>> HDMI_QUANTIZATION_RANGE_LIMITED :
> >>>> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector
> >>> *connector)
> >>>>  	intel_attach_broadcast_rgb_property(connector);
> >>>>  	intel_attach_aspect_ratio_property(connector);
> >>>>  	drm_connector_attach_content_type_property(connector);
> >>>> +	drm_object_attach_property(&connector->base,
> >>>> +			connector->dev->mode_config.colorspace_property,
> >>>> +			COLORIMETRY_ITU_709);
> >>> Just put 0 here..
> >>> If you want to init the default colorspace, put it in the first patch.
> >> Ok, will update this.
> >>
> >>> We should perhaps hide color spaces that are not supported on HDMI?
> >> Currently the supported colorspaces will be picked from edid by userspace and
> >> they should use the current property interface to set the one which is supported.
> >> Even on HDMI, some connectors may not support certain colorspace, so keeping
> >> it on userspace to set the one which is supported by the particular connector. Hope
> >> this approach is fine ?
> > I think we want to trim the list to whatever the infoframe vs. MSA/VSC
> > SDP can carry. So HDMI will have one list, DP another. And I guess for
> > lspcon we want to go with the HDMI definition since we populate the
> > infoframe by hand.
> >
> What about passive DP to HDMI convertors?

By passive you mean DP++? Those are HDMI for us.

External DP->HDMI protocol converters are perhaps more tricky, but
since we treat those as pure DP now and don't send any infoframes I
think we should do the same when it comes to the exposed valeus for
the property.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

[-- Attachment #1.1: Type: text/plain, Size: 3068 bytes --]

Regards

Shashank


On 10/31/2018 5:35 PM, 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.
>
> v2: Merged the changes of creating infoframe as well to this
> patch as per Maarten's suggestion.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_atomic.c | 1 +
>   drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
>   2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index a5a2c8f..35ef70a 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -125,6 +125,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_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 129b880..8a41fb3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct intel_encoder *encoder,
>   	else
>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>   
> +	frame.avi.extended_colorimetry = conn_state->colorspace;
> +
We must also set the date byte 2 bits C1-C0 (colorimetry part) to 
indicate the use of extended colorimetry bits, so that monitor will 
refer to extended colorimetry data, else it wont even bother looking at it:

Extended Colorimetry Information Valid (colorimetry indicated in bits 
EC0, EC1, and EC2)


Its right now set to dafault 0-0 I guess, indicating No data We should 
also set it to a default value now, when we have started bothering about 
gamut.

Regards
Shashank
>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>   					   crtc_state->limited_color_range ?
>   					   HDMI_QUANTIZATION_RANGE_LIMITED :
> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>   	intel_attach_broadcast_rgb_property(connector);
>   	intel_attach_aspect_ratio_property(connector);
>   	drm_connector_attach_content_type_property(connector);
> +	drm_object_attach_property(&connector->base,
> +			connector->dev->mode_config.colorspace_property,
> +			COLORIMETRY_ITU_709);
>   	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>   }
>   


[-- Attachment #1.2: Type: text/html, Size: 6149 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for Add Colorspace connector property interface (rev2)

[Patchwork]

== Series Details ==

Series: Add Colorspace connector property interface (rev2)
URL   : https://patchwork.freedesktop.org/series/47132/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b716d24947ce drm: Add colorspace property
33d07ea8810b drm/i915: Attach colorspace property and enable modeset
-:50: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#50: FILE: drivers/gpu/drm/i915/intel_hdmi.c:2131:
+	drm_object_attach_property(&connector->base,
+			connector->dev->mode_config.colorspace_property,

total: 0 errors, 0 warnings, 1 checks, 24 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for Add Colorspace connector property interface (rev2)

[Patchwork]

== Series Details ==

Series: Add Colorspace connector property interface (rev2)
URL   : https://patchwork.freedesktop.org/series/47132/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm: Add colorspace property
Okay!

Commit: drm/i915: Attach colorspace property and enable modeset
+drivers/gpu/drm/i915/intel_hdmi.c:489:52:     int enum encoder_colorimetry  versus
+drivers/gpu/drm/i915/intel_hdmi.c:489:52:     int enum hdmi_extended_colorimetry 
+drivers/gpu/drm/i915/intel_hdmi.c:489:52: warning: mixing different enum types

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Add Colorspace connector property interface (rev2)

[Patchwork]

== Series Details ==

Series: Add Colorspace connector property interface (rev2)
URL   : https://patchwork.freedesktop.org/series/47132/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5062 -> Patchwork_10663 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10663 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10663, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47132/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10663:

  === IGT changes ===

    ==== Possible regressions ====

    igt@debugfs_test@read_all_entries:
      fi-skl-iommu:       PASS -> INCOMPLETE
      fi-ivb-3770:        PASS -> INCOMPLETE
      fi-cfl-s3:          PASS -> INCOMPLETE
      fi-hsw-peppy:       PASS -> INCOMPLETE
      fi-skl-6260u:       PASS -> INCOMPLETE
      fi-icl-u:           PASS -> INCOMPLETE
      fi-snb-2600:        PASS -> INCOMPLETE
      fi-hsw-4770r:       PASS -> INCOMPLETE
      fi-kbl-guc:         PASS -> FAIL
      fi-bsw-kefka:       PASS -> INCOMPLETE
      fi-skl-6700hq:      PASS -> INCOMPLETE
      fi-kbl-x1275:       PASS -> FAIL
      fi-kbl-7500u:       PASS -> INCOMPLETE
      fi-blb-e6850:       PASS -> INCOMPLETE
      fi-bwr-2160:        PASS -> INCOMPLETE
      fi-bdw-5557u:       PASS -> INCOMPLETE
      fi-kbl-r:           PASS -> INCOMPLETE
      fi-kbl-7567u:       PASS -> INCOMPLETE
      fi-cnl-u:           PASS -> INCOMPLETE
      fi-gdg-551:         PASS -> INCOMPLETE
      fi-cfl-8109u:       PASS -> INCOMPLETE
      fi-skl-6600u:       PASS -> INCOMPLETE
      fi-cfl-8700k:       PASS -> INCOMPLETE
      fi-whl-u:           PASS -> INCOMPLETE
      fi-pnv-d510:        PASS -> INCOMPLETE
      fi-ivb-3520m:       PASS -> INCOMPLETE
      fi-bsw-n3050:       PASS -> INCOMPLETE
      fi-skl-6700k2:      PASS -> INCOMPLETE
      fi-hsw-4770:        PASS -> INCOMPLETE
      fi-kbl-soraka:      PASS -> INCOMPLETE
      fi-skl-6770hq:      PASS -> INCOMPLETE

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-x1275:       PASS -> INCOMPLETE

    
    ==== Warnings ====

    igt@debugfs_test@read_all_entries:
      fi-icl-u2:          DMESG-WARN (fdo#108070) -> INCOMPLETE

    
== Known issues ==

  Here are the changes found in Patchwork_10663 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-glk-dsi:         PASS -> INCOMPLETE (fdo#103359, k.org#198133)
      fi-skl-gvtdvm:      PASS -> INCOMPLETE (fdo#105600)
      fi-bdw-gvtdvm:      PASS -> INCOMPLETE (fdo#105600)
      fi-bxt-j4205:       PASS -> INCOMPLETE (fdo#103927)
      fi-skl-guc:         PASS -> INCOMPLETE (fdo#106693)
      fi-apl-guc:         PASS -> INCOMPLETE (fdo#106693)
      fi-glk-j4005:       PASS -> INCOMPLETE (fdo#103359, k.org#198133)
      fi-byt-clapper:     PASS -> INCOMPLETE (fdo#102657)
      fi-byt-j1900:       PASS -> INCOMPLETE (fdo#102657)
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)
      fi-kbl-7560u:       PASS -> INCOMPLETE (fdo#103665)
      fi-cfl-guc:         PASS -> INCOMPLETE (fdo#106693)

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-guc:         PASS -> INCOMPLETE (fdo#106693)

    
  fdo#102657 https://bugs.freedesktop.org/show_bug.cgi?id=102657
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105600 https://bugs.freedesktop.org/show_bug.cgi?id=105600
  fdo#106693 https://bugs.freedesktop.org/show_bug.cgi?id=106693
  fdo#108070 https://bugs.freedesktop.org/show_bug.cgi?id=108070
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (49 -> 44) ==

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5062 -> Patchwork_10663

  CI_DRM_5062: 3aa71a0d803ee01605f9a3026ddd989a591a73c6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4703: f882a542a3eb24e78e51aa6410a3a67c0efb4e97 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10663: 33d07ea8810b104ee7f473d476386f8d662b3305 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

33d07ea8810b drm/i915: Attach colorspace property and enable modeset
b716d24947ce drm: Add colorspace property

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10663/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 0/2] Add Colorspace connector property interface

[Sharma, Shashank]

Hello Uma,

My comments inline.


On 10/31/2018 5:35 PM, Uma Shankar wrote:
> This patch series creates 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. Kernel will not give the supported colorspaces since
> this is panel dependent and our curremt property infrastructure is
> not supporting it.
I am not very sure about this part, consider a situation where, I have 
connected a HDMI 2.0 monitor which supports BT2020 gamut, to a GEN9 
device, running an user-space which can also support BT2020 playback / 
HDR playback. Now, both monitor and user-space supports BT2020 playback, 
but the HW doesn't, and this will create a problem during playback. So I 
gess we need to expose a kernel Cap or property to showcase if the HW 
can support BT2020/DCI-P3 gamut space or not.

- Shashank
> Have tested this using xrandr by using below command:
> xrandr --output HDMI2 --set "Colorspace" "BT2020_rgb"
>
> v2: Addressed Ville and Maarten's review comments. Merged the 2nd
> and 3rd patch into one common logical patch.
>
> Uma Shankar (2):
>    drm: Add colorspace property
>    drm/i915: Attach colorspace property and enable modeset
>
>   drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
>   drivers/gpu/drm/drm_connector.c     | 44 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_atomic.c |  1 +
>   drivers/gpu/drm/i915/intel_hdmi.c   |  5 +++++
>   include/drm/drm_connector.h         |  7 ++++++
>   include/drm/drm_mode_config.h       |  6 +++++
>   include/uapi/drm/drm_mode.h         | 24 ++++++++++++++++++++
>   7 files changed, 91 insertions(+)
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx