[RFC 0/3] Add Colorspace connector property interface

[RFC 0/3] 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 dependant 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"

Please provide comments on this current approach. This is just an RFC
to get some feedback. Will refine the series based on inputs and
feedback.

Uma Shankar (3):
  drm: Add colorspace property
  drm/i915: Attach colorspace property and enable modeset
  drm/i915: Set colorspace by enabling Infoframe

 drivers/gpu/drm/drm_atomic.c        |  4 ++++
 drivers/gpu/drm/drm_connector.c     | 31 +++++++++++++++++++++++++++++++
 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         | 11 +++++++++++
 7 files changed, 65 insertions(+)

-- 
1.9.1

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

[RFC 1/3] 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.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3eb061e..f065e6f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1397,6 +1397,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) {
@@ -1502,6 +1504,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 		*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 5ada064..cfe1d79 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -805,6 +805,25 @@ 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 IEC 61966-2-4 */
+	{ EXTENDED_COLORIMETRY_XV_YCC_601, "601" },
+	/* High Definition Colorimetry based on IEC 61966-2-4 */
+	{ EXTENDED_COLORIMETRY_XV_YCC_709, "709" },
+	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
+	{ EXTENDED_COLORIMETRY_S_YCC_601, "s601" },
+	/* Colorimetry based on IEC 61966-2-5 [33] */
+	{ EXTENDED_COLORIMETRY_ADOBE_YCC_601, "adobe601" },
+	/* Colorimetry based on IEC 61966-2-5 */
+	{ EXTENDED_COLORIMETRY_ADOBE_RGB, "adobe_rgb" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ EXTENDED_COLORIMETRY_BT2020_RGB, "BT2020_rgb" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ EXTENDED_COLORIMETRY_BT2020_YCC, "BT2020_ycc" },
+	/* Colorimetry based on ITU-R BT.2020 */
+	{ EXTENDED_COLORIMETRY_BT2020_CYCC, "BT2020_cycc" },
+};
+
 /**
  * DOC: standard connector properties
  *
@@ -951,6 +970,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)
@@ -999,6 +1024,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 a5179eb..306b536 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -443,6 +443,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 extended_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 a0b202e..3afe30b 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -841,6 +841,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 8d67243..c9d14ca 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -209,6 +209,17 @@
 #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
 #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
 
+enum extended_colorimetry {
+	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
+	EXTENDED_COLORIMETRY_XV_YCC_709,
+	EXTENDED_COLORIMETRY_S_YCC_601,
+	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
+	EXTENDED_COLORIMETRY_ADOBE_RGB,
+	EXTENDED_COLORIMETRY_BT2020_RGB,
+	EXTENDED_COLORIMETRY_BT2020_YCC,
+	EXTENDED_COLORIMETRY_BT2020_CYCC,
+};
+
 struct drm_mode_modeinfo {
 	__u32 clock;
 	__u16 hdisplay;
-- 
1.9.1

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

[RFC 2/3] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b04952b..8e7d540 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 d06cf42..7fb96e2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2110,6 +2110,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,
+			EXTENDED_COLORIMETRY_XV_YCC_601);
 	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

[RFC 3/3] drm/i915: Set colorspace by enabling Infoframe

[Uma Shankar]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 7fb96e2..319da1b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_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 :
-- 
1.9.1

_______________________________________________
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

[Patchwork]

== Series Details ==

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

== Summary ==

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

total: 0 errors, 0 warnings, 1 checks, 16 lines checked
bfaa2413cf37 drm/i915: Set colorspace by enabling Infoframe

_______________________________________________
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

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm: Add colorspace property
Okay!

Commit: drm/i915: Attach colorspace property and enable modeset
Okay!

Commit: drm/i915: Set colorspace by enabling Infoframe
+drivers/gpu/drm/i915/intel_hdmi.c:494:52:     int enum extended_colorimetry  versus
+drivers/gpu/drm/i915/intel_hdmi.c:494:52:     int enum hdmi_extended_colorimetry 
+drivers/gpu/drm/i915/intel_hdmi.c:494: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: success for Add Colorspace connector property interface

[Patchwork]

== Series Details ==

Series: Add Colorspace connector property interface
URL   : https://patchwork.freedesktop.org/series/47132/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4535 -> Patchwork_9757 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725)

    igt@kms_chamelium@common-hpd-after-suspend:
      fi-skl-6700k2:      PASS -> INCOMPLETE (fdo#104108, k.org#199541, fdo#105524)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097, fdo#106000)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL (fdo#100368)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_coherency:
      fi-byt-j1900:       DMESG-FAIL -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105524 https://bugs.freedesktop.org/show_bug.cgi?id=105524
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  k.org#199541 https://bugzilla.kernel.org/show_bug.cgi?id=199541


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

  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-skl-caroline fi-byt-clapper fi-bdw-samus 


== Build changes ==

    * Linux: CI_DRM_4535 -> Patchwork_9757

  CI_DRM_4535: 79a6a05d5beb28987746b0ea61837783a4af4e82 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4573: 2884f91dd6d7682ea738ef6f0943cc591643dda2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9757: bfaa2413cf37fbf7ef53ddaa67b122e20f4edd2e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

bfaa2413cf37 drm/i915: Set colorspace by enabling Infoframe
d223f56b07d5 drm/i915: Attach colorspace property and enable modeset
6ef31f2983ca drm: Add colorspace property

== Logs ==

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

✓ Fi.CI.IGT: success for Add Colorspace connector property interface

[Patchwork]

== Series Details ==

Series: Add Colorspace connector property interface
URL   : https://patchwork.freedesktop.org/series/47132/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4535_full -> Patchwork_9757_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_flush@basic-wb-rw-default:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip@plain-flip-ts-check:
      shard-glk:          PASS -> FAIL (fdo#103928) +1

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#102887) -> PASS

    igt@kms_rotation_crc@sprite-rotation-270:
      shard-apl:          FAIL (fdo#103925) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4535 -> Patchwork_9757

  CI_DRM_4535: 79a6a05d5beb28987746b0ea61837783a4af4e82 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4573: 2884f91dd6d7682ea738ef6f0943cc591643dda2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9757: bfaa2413cf37fbf7ef53ddaa67b122e20f4edd2e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [RFC 1/3] drm: Add colorspace property

[Adam Jackson]

On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:

> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -209,6 +209,17 @@
>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>  
> +enum extended_colorimetry {
> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
> +	EXTENDED_COLORIMETRY_XV_YCC_709,
> +	EXTENDED_COLORIMETRY_S_YCC_601,
> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
> +	EXTENDED_COLORIMETRY_BT2020_RGB,
> +	EXTENDED_COLORIMETRY_BT2020_YCC,
> +	EXTENDED_COLORIMETRY_BT2020_CYCC,
> +};

This doesn't give any way to distinguish "not set" from BT.601, which
I'm not sure I like.

Is this enum simply built to match the values you're injecting into the
InfoFrame? Would we need a different enum for DisplayPort?

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

RE: [RFC 1/3] drm: Add colorspace property

[Shankar, Uma]

>-----Original Message-----
>From: Adam Jackson [mailto:ajax@redhat.com]
>Sent: Wednesday, August 1, 2018 1:24 AM
>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>dri-devel@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [RFC 1/3] drm: Add colorspace property
>
>On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:
>
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -209,6 +209,17 @@
>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>
>> +enum extended_colorimetry {
>> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
>> +	EXTENDED_COLORIMETRY_XV_YCC_709,
>> +	EXTENDED_COLORIMETRY_S_YCC_601,
>> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
>> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
>> +	EXTENDED_COLORIMETRY_BT2020_RGB,
>> +	EXTENDED_COLORIMETRY_BT2020_YCC,
>> +	EXTENDED_COLORIMETRY_BT2020_CYCC,
>> +};
>
>This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure
>I like.

This enum gives a list of all possible colorspace which can be set on the sink device.
The compositors/userspace can choose one of them, based on the capabilities of sink
as well as based on rendering/blending policies which are designed to take advantage
of hardware resources available.

If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this
enum list.

>
>Is this enum simply built to match the values you're injecting into the InfoFrame?
Yes they are as per HDMI SPEC defined Infoframe. This can directly be assigned to create
the equivalent infoframe.

>Would we need a different enum for DisplayPort?
DP will define a SDP packet to pass this info. From userspace, we can still pass this
enum value, as part of SDP packet creation DP driver can pick equivalent value as per DP spec
(which may be different from this enum value). But driver will still know as to what
colorspace is requested by userspace. 

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

Re: [RFC 1/3] drm: Add colorspace property

[Maarten Lankhorst]

Op 01-08-18 om 16:01 schreef Shankar, Uma:
>
>> -----Original Message-----
>> From: Adam Jackson [mailto:ajax@redhat.com]
>> Sent: Wednesday, August 1, 2018 1:24 AM
>> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>> dri-devel@lists.freedesktop.org
>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> <maarten.lankhorst@intel.com>
>> Subject: Re: [RFC 1/3] drm: Add colorspace property
>>
>> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:
>>
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -209,6 +209,17 @@
>>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>>
>>> +enum extended_colorimetry {
>>> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
>>> +	EXTENDED_COLORIMETRY_XV_YCC_709,
>>> +	EXTENDED_COLORIMETRY_S_YCC_601,
>>> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
>>> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
>>> +	EXTENDED_COLORIMETRY_BT2020_RGB,
>>> +	EXTENDED_COLORIMETRY_BT2020_YCC,
>>> +	EXTENDED_COLORIMETRY_BT2020_CYCC,
>>> +};
>> This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure
>> I like.
> This enum gives a list of all possible colorspace which can be set on the sink device.
> The compositors/userspace can choose one of them, based on the capabilities of sink
> as well as based on rendering/blending policies which are designed to take advantage
> of hardware resources available.
>
> If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this
> enum list.
I would add a default, but not sure I would introduce a new enum.
hdmi_extended_colorimetry is already available.

I would argue to keep it as it is, Or really keep it as it is, since we have to set it
to *something* when sending the infoframe. And we currently send this value.

Hm maybe for DP it could be useful, if it's optional to put it in the SDP packet there.

>> Is this enum simply built to match the values you're injecting into the InfoFrame?
> Yes they are as per HDMI SPEC defined Infoframe. This can directly be assigned to create
> the equivalent infoframe.
Yes, in fact I would take the one from hdmi.h instead of redefining it. :)
We already do for the other props.
>> Would we need a different enum for DisplayPort?
> DP will define a SDP packet to pass this info. From userspace, we can still pass this
> enum value, as part of SDP packet creation DP driver can pick equivalent value as per DP spec
> (which may be different from this enum value). But driver will still know as to what
> colorspace is requested by userspace. 
Sounds good. :)

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

Re: [RFC 3/3] drm/i915: Set colorspace by enabling Infoframe

[Maarten Lankhorst]

Op 24-07-18 om 17:45 schreef Uma Shankar:
> 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.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 7fb96e2..319da1b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_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 :

I would merge this with the previous patch, to be honest.

If it was non-trivial I would suggest splitting it up. :)

~Maarten

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

Re: [RFC 1/3] drm: Add colorspace property

[Ville Syrjälä]

On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote:
> Op 01-08-18 om 16:01 schreef Shankar, Uma:
> >
> >> -----Original Message-----
> >> From: Adam Jackson [mailto:ajax@redhat.com]
> >> Sent: Wednesday, August 1, 2018 1:24 AM
> >> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >> dri-devel@lists.freedesktop.org
> >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> <maarten.lankhorst@intel.com>
> >> Subject: Re: [RFC 1/3] drm: Add colorspace property
> >>
> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:
> >>
> >>> --- a/include/uapi/drm/drm_mode.h
> >>> +++ b/include/uapi/drm/drm_mode.h
> >>> @@ -209,6 +209,17 @@
> >>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> >>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> >>>
> >>> +enum extended_colorimetry {
> >>> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
> >>> +	EXTENDED_COLORIMETRY_XV_YCC_709,
> >>> +	EXTENDED_COLORIMETRY_S_YCC_601,
> >>> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
> >>> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
> >>> +	EXTENDED_COLORIMETRY_BT2020_RGB,
> >>> +	EXTENDED_COLORIMETRY_BT2020_YCC,
> >>> +	EXTENDED_COLORIMETRY_BT2020_CYCC,
> >>> +};
> >> This doesn't give any way to distinguish "not set" from BT.601, which I'm not sure
> >> I like.
> > This enum gives a list of all possible colorspace which can be set on the sink device.
> > The compositors/userspace can choose one of them, based on the capabilities of sink
> > as well as based on rendering/blending policies which are designed to take advantage
> > of hardware resources available.
> >
> > If you suggest to add something like NO_COLORSPACE_SET = -1, I can add that to this
> > enum list.
> I would add a default, but not sure I would introduce a new enum.
> hdmi_extended_colorimetry is already available.

Yeah, there should be a default entry for "driver automagically picks
something suitable".

I think the enum prop should also be some kind of superset of all
CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP
colorimetry options. The current list seems a bit incomplete to me.

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

Re: [RFC 1/3] drm: Add colorspace property

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, September 26, 2018 3:12 PM
>To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Shankar, Uma <uma.shankar@intel.com>; Adam Jackson
><ajax@redhat.com>; intel-gfx@lists.freedesktop.org; dri-
>devel@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst,
>Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
>
>On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote:
>> Op 01-08-18 om 16:01 schreef Shankar, Uma:
>> >
>> >> -----Original Message-----
>> >> From: Adam Jackson [mailto:ajax@redhat.com]
>> >> Sent: Wednesday, August 1, 2018 1:24 AM
>> >> To: Shankar, Uma <uma.shankar@intel.com>;
>> >> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
>> >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
>> >> <maarten.lankhorst@intel.com>
>> >> Subject: Re: [RFC 1/3] drm: Add colorspace property
>> >>
>> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:
>> >>
>> >>> --- a/include/uapi/drm/drm_mode.h
>> >>> +++ b/include/uapi/drm/drm_mode.h
>> >>> @@ -209,6 +209,17 @@
>> >>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> >>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>> >>>
>> >>> +enum extended_colorimetry {
>> >>> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
>> >>> +	EXTENDED_COLORIMETRY_XV_YCC_709,
>> >>> +	EXTENDED_COLORIMETRY_S_YCC_601,
>> >>> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
>> >>> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
>> >>> +	EXTENDED_COLORIMETRY_BT2020_RGB,
>> >>> +	EXTENDED_COLORIMETRY_BT2020_YCC,
>> >>> +	EXTENDED_COLORIMETRY_BT2020_CYCC, };
>> >> This doesn't give any way to distinguish "not set" from BT.601,
>> >> which I'm not sure I like.
>> > This enum gives a list of all possible colorspace which can be set on the sink
>device.
>> > The compositors/userspace can choose one of them, based on the
>> > capabilities of sink as well as based on rendering/blending policies
>> > which are designed to take advantage of hardware resources available.
>> >
>> > If you suggest to add something like NO_COLORSPACE_SET = -1, I can
>> > add that to this enum list.
>> I would add a default, but not sure I would introduce a new enum.
>> hdmi_extended_colorimetry is already available.
>
>Yeah, there should be a default entry for "driver automagically picks something
>suitable".
>

Ok got it, will add a default option to the list.

>I think the enum prop should also be some kind of superset of all
>CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP colorimetry
>options. The current list seems a bit incomplete to me.
>

So should I keep this enum and append DP MSA +VSC SDP options to the list ?
We can then have encoder specific enums separate from this global colorpsace
enum. 

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

RE: [Intel-gfx] [RFC 3/3] drm/i915: Set colorspace by enabling Infoframe

[Shankar, Uma]

>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>Sent: Wednesday, September 26, 2018 2:41 PM
>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>dri-devel@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915: Set colorspace by enabling Infoframe
>
>Op 24-07-18 om 17:45 schreef Uma Shankar:
>> 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.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_hdmi.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
>> b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 7fb96e2..319da1b 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct
>drm_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 :
>
>I would merge this with the previous patch, to be honest.
>
>If it was non-trivial I would suggest splitting it up. :)

Ok Sure, will merge them.

>~Maarten

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

Re: [RFC 1/3] drm: Add colorspace property

[Ville Syrjälä]

On Thu, Sep 27, 2018 at 04:29:52AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Wednesday, September 26, 2018 3:12 PM
> >To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; Adam Jackson
> ><ajax@redhat.com>; intel-gfx@lists.freedesktop.org; dri-
> >devel@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst,
> >Maarten <maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [RFC 1/3] drm: Add colorspace property
> >
> >On Wed, Sep 26, 2018 at 11:08:37AM +0200, Maarten Lankhorst wrote:
> >> Op 01-08-18 om 16:01 schreef Shankar, Uma:
> >> >
> >> >> -----Original Message-----
> >> >> From: Adam Jackson [mailto:ajax@redhat.com]
> >> >> Sent: Wednesday, August 1, 2018 1:24 AM
> >> >> To: Shankar, Uma <uma.shankar@intel.com>;
> >> >> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> >> >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >> >> <maarten.lankhorst@intel.com>
> >> >> Subject: Re: [RFC 1/3] drm: Add colorspace property
> >> >>
> >> >> On Tue, 2018-07-24 at 21:15 +0530, Uma Shankar wrote:
> >> >>
> >> >>> --- a/include/uapi/drm/drm_mode.h
> >> >>> +++ b/include/uapi/drm/drm_mode.h
> >> >>> @@ -209,6 +209,17 @@
> >> >>>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> >> >>>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> >> >>>
> >> >>> +enum extended_colorimetry {
> >> >>> +	EXTENDED_COLORIMETRY_XV_YCC_601 = 0,
> >> >>> +	EXTENDED_COLORIMETRY_XV_YCC_709,
> >> >>> +	EXTENDED_COLORIMETRY_S_YCC_601,
> >> >>> +	EXTENDED_COLORIMETRY_ADOBE_YCC_601,
> >> >>> +	EXTENDED_COLORIMETRY_ADOBE_RGB,
> >> >>> +	EXTENDED_COLORIMETRY_BT2020_RGB,
> >> >>> +	EXTENDED_COLORIMETRY_BT2020_YCC,
> >> >>> +	EXTENDED_COLORIMETRY_BT2020_CYCC, };
> >> >> This doesn't give any way to distinguish "not set" from BT.601,
> >> >> which I'm not sure I like.
> >> > This enum gives a list of all possible colorspace which can be set on the sink
> >device.
> >> > The compositors/userspace can choose one of them, based on the
> >> > capabilities of sink as well as based on rendering/blending policies
> >> > which are designed to take advantage of hardware resources available.
> >> >
> >> > If you suggest to add something like NO_COLORSPACE_SET = -1, I can
> >> > add that to this enum list.
> >> I would add a default, but not sure I would introduce a new enum.
> >> hdmi_extended_colorimetry is already available.
> >
> >Yeah, there should be a default entry for "driver automagically picks something
> >suitable".
> >
> 
> Ok got it, will add a default option to the list.
> 
> >I think the enum prop should also be some kind of superset of all
> >CEA-861 normal+extended colorimetry options and DP MSA+VSC SDP colorimetry
> >options. The current list seems a bit incomplete to me.
> >
> 
> So should I keep this enum and append DP MSA +VSC SDP options to the list ?
> We can then have encoder specific enums separate from this global colorpsace
> enum. 

My original idea was one enum and each encoder type can then pick
the ones it can support by passing in the appropriate bitmask. That's
assuming we can reconcile any differences between DP and HDMI in
a nice way.

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