[PATCH v2 00/21] Enable Colorspace connector property in amdgpu

[PATCH v2 00/21] Enable Colorspace connector property in amdgpu

[Harry Wentland]

This patchset enables the DP and HDMI infoframe properties
in amdgpu.

The first two patches are not completely related to the rest. The
first patch allows for HDR_OUTPUT_METADATA with EOTFs that are
unknown in the kernel.

The second one prints a connector's max_bpc as part of the atomic
state debugfs print.

The following patches rework the connector colorspace code to
1) allow for easy printing of the colorspace in the drm_atomic
   state debugfs, and
2) allow drivers to specify the supported colorspaces on a
   connector.

The rest of the patches deal with the Colorspace enablement
in amdgpu.

Why do drivers need to specify supported colorspaces? The amdgpu
driver needs support for RGB-to-YCbCr conversion when we drive
the display in YCbCr. This is currently not implemented for all
colorspaces.

Since the Colorspace property didn't have an IGT test I added
one to kms_hdr. The relevant patchset can be found on the IGT
mailing list or on
https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry

We tested v1 of the patchset and confirmed that the infoframes
are as expected for both DP and HDMI when running the IGT
colorimetry tests.

Open Items
----------

A couple comments from Pekka about colorspace documentation are
left unaddressed. I hope they won't block merging this set but
should still be addressed separately.

Pekka's questions really got me thinking of how this colorspace
property should be used and working with it more closely with
Joshua who is enabling HDR in gamescope made me wonder even more.

Uma, is there a (canonical, upstream) userspace that uses this
property that I can look at to understand more?

One of the key challenges that is currently not addressed is that
userspace is expected to pick a colorspace format straight from the
list of definitions out of the DP or HDMI spec. But the kernel
driver are the ones deciding on the output encoding (RGB, YCBCR444,
YCBCR420, etc.). So there is no way for userspace to decide correctly
between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC.

So we end up in a scenario where gamescope sets BT2020_RGB but we
output YCBCR444 so have to correct the colorspace value to
BT2020_YCC. This in turn breaks the colorspace IGT tests I
wrote. I don't think "fixing" the IGT tests to accept this is
the right thing to do.

The way it stands this patchset allows us to specify the output
colorspace on amdgpu and we try to do the right thing, but I don't
thing the way the colorspace property is defined is right. We're trying
to expose things to userspace that should be under driver control. A
much better approach would be to give userspace options for colorspace
that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc.,
and have the driver do the right thing to fill the infoframe, e.g., by
picking BT2020_YCC if the requested colorspace is BT2020 and the
is YCBCR444.

If no upstream userspace currently makes use of this property I
can make that change, i.e., no longer tie the colorspace property
directly to the infoframe and reduce the options to sRGB, BT709,
BT601, and BT2020 (and possibly opRGB).

v2:
- Tested with DP and HDMI analyzers
- Confirmed driver will fallback to lower bpc when needed
- Dropped hunk to set HDMI AVI infoframe as it was a no-op
- Fixed BT.2020 YCbCr colorimetry (JoshuaAshton)
- Simplify initialization of supported colorspaces (Jani)
- Fix kerneldoc (kernel test robot)

Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Michel Dänzer <michel.daenzer@mailbox.org>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org

Harry Wentland (16):
  drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF
  drm/connector: print max_requested_bpc in state debugfs
  drm/connector: Drop COLORIMETRY_NO_DATA
  drm/connector: Convert DRM_MODE_COLORIMETRY to enum
  drm/connector: Pull out common create_colorspace_property code
  drm/connector: Allow drivers to pass list of supported colorspaces
  drm/connector: Print connector colorspace in state debugfs
  drm/amd/display: Always pass connector_state to stream validation
  drm/amd/display: Register Colorspace property for DP and HDMI
  drm/amd/display: Signal mode_changed if colorspace changed
  drm/amd/display: Send correct DP colorspace infopacket
  drm/amd/display: Add support for explicit BT601_YCC
  drm/amd/display: Add debugfs for testing output colorspace
  drm/amd/display: Add default case for output_color_space switch
  drm/amd/display: Don't restrict bpc to 8 bpc
  drm/amd/display: Format input and output CSC matrix

Joshua Ashton (5):
  drm/amd/display: Always set crtcinfo from create_stream_for_sink
  drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not
    RGB
  drm/amd/display: Refactor avi_info_frame colorimetry determination
  drm/amd/display: Calculate output_color_space after pixel encoding
    adjustment
  drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  90 ++++++---
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
 .../drm/amd/display/dc/core/dc_hw_sequencer.c |  38 ++--
 .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 ++-
 drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   |  54 +++--
 drivers/gpu/drm/display/drm_hdmi_helper.c     |   8 +-
 drivers/gpu/drm/drm_atomic.c                  |   2 +
 drivers/gpu/drm/drm_connector.c               | 189 ++++++++++--------
 .../gpu/drm/i915/display/intel_connector.c    |   4 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
 include/drm/display/drm_dp.h                  |   2 +-
 include/drm/drm_connector.h                   |  57 +++---
 12 files changed, 345 insertions(+), 186 deletions(-)

--
2.39.0

[PATCH v2 01/21] drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF

[Harry Wentland]

The EDID of an HDR display defines EOTFs that are supported
by the display and can be set in the HDR metadata infoframe.
Userspace is expected to read the EDID and set an appropriate
HDR_OUTPUT_METADATA.

In drm_parse_hdr_metadata_block the kernel reads the supported
EOTFs from the EDID and stores them in the
drm_connector->hdr_sink_metadata. While doing so it also
filters the EOTFs to the EOTFs the kernel knows about.
When an HDR_OUTPUT_METADATA is set it then checks to
make sure the EOTF is a supported EOTF. In cases where
the kernel doesn't know about a new EOTF this check will
fail, even if the EDID advertises support.

Since it is expected that userspace reads the EDID to understand
what the display supports it doesn't make sense for DRM to block
an HDR_OUTPUT_METADATA if it contains an EOTF the kernel doesn't
understand.

This comes with the added benefit of future-proofing metadata
support. If the spec defines a new EOTF there is no need to
update DRM and an compositor can immediately make use of it.

Fixes: https://gitlab.freedesktop.org/wayland/weston/-/issues/609

v2: Distinguish EOTFs defind in kernel and ones defined
    in EDID in the commit description (Pekka)

v3: Rebase; drm_hdmi_infoframe_set_hdr_metadata moved
    to drm_hdmi_helper.c

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index 0264abe55278..faf5e9efa7d3 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -44,10 +44,8 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
 
 	/* Sink EOTF is Bit map while infoframe is absolute values */
 	if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf,
-	    connector->hdr_sink_metadata.hdmi_type1.eotf)) {
-		DRM_DEBUG_KMS("EOTF Not Supported\n");
-		return -EINVAL;
-	}
+	    connector->hdr_sink_metadata.hdmi_type1.eotf))
+		DRM_DEBUG_KMS("Unknown EOTF %d\n", hdr_metadata->hdmi_metadata_type1.eotf);
 
 	err = hdmi_drm_infoframe_init(frame);
 	if (err < 0)
-- 
2.39.0

[PATCH v2 02/21] drm/connector: print max_requested_bpc in state debugfs

[Harry Wentland]

This is useful to understand the bpc defaults and
support of a driver.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/drm_atomic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..c0dc5858a723 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1070,6 +1070,7 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector->name);
 	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
 	drm_printf(p, "\tself_refresh_aware=%d\n", state->self_refresh_aware);
+	drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc);
 
 	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
 		if (state->writeback_job && state->writeback_job->fb)
-- 
2.39.0

[PATCH v2 03/21] drm/connector: Drop COLORIMETRY_NO_DATA

[Harry Wentland]

The value is the same as DEFAULT. The HDMI_COLORIMETRY_NO_DATA
makes sense for the infopacket but it's meaningless for the
connector colorspace. or, in otherwise, just means to go with
driver default.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 2 +-
 include/drm/drm_connector.h               | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index faf5e9efa7d3..c1e6851b2606 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -103,7 +103,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
 #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER	(C(3) | EC(7) | ACE(1))
 
 static const u32 hdmi_colorimetry_val[] = {
-	[DRM_MODE_COLORIMETRY_NO_DATA] = HDMI_COLORIMETRY_NO_DATA,
+	[DRM_MODE_COLORIMETRY_DEFAULT] = HDMI_COLORIMETRY_NO_DATA,
 	[DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = HDMI_COLORIMETRY_SMPTE_170M_YCC,
 	[DRM_MODE_COLORIMETRY_BT709_YCC] = HDMI_COLORIMETRY_BT709_YCC,
 	[DRM_MODE_COLORIMETRY_XVYCC_601] = HDMI_COLORIMETRY_XVYCC_601,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 4d830fc55a3d..62c814241828 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -375,7 +375,6 @@ enum drm_privacy_screen_status {
 /* For Default case, driver will set the colorspace */
 #define DRM_MODE_COLORIMETRY_DEFAULT			0
 /* CEA 861 Normal Colorimetry options */
-#define DRM_MODE_COLORIMETRY_NO_DATA			0
 #define DRM_MODE_COLORIMETRY_SMPTE_170M_YCC		1
 #define DRM_MODE_COLORIMETRY_BT709_YCC			2
 /* CEA 861 Extended Colorimetry Options */
-- 
2.39.0

[PATCH v2 04/21] drm/connector: Convert DRM_MODE_COLORIMETRY to enum

[Harry Wentland]

This allows us to use strongly typed arguments.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 include/drm/display/drm_dp.h |  2 +-
 include/drm/drm_connector.h  | 47 ++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index ed10e6b6f99d..28899a03245c 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -1623,7 +1623,7 @@ enum dp_pixelformat {
  *
  * This enum is used to indicate DP VSC SDP Colorimetry formats.
  * It is based on DP 1.4 spec [Table 2-117: VSC SDP Payload for DB16 through
- * DB18] and a name of enum member follows DRM_MODE_COLORIMETRY definition.
+ * DB18] and a name of enum member follows &enum drm_colorimetry definition.
  *
  * @DP_COLORIMETRY_DEFAULT: sRGB (IEC 61966-2-1) or
  *                          ITU-R BT.601 colorimetry format
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 62c814241828..edef65388c29 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -371,28 +371,29 @@ enum drm_privacy_screen_status {
  * a colorspace property which will be created and exposed to
  * userspace.
  */
-
-/* For Default case, driver will set the colorspace */
-#define DRM_MODE_COLORIMETRY_DEFAULT			0
-/* CEA 861 Normal Colorimetry options */
-#define DRM_MODE_COLORIMETRY_SMPTE_170M_YCC		1
-#define DRM_MODE_COLORIMETRY_BT709_YCC			2
-/* CEA 861 Extended Colorimetry Options */
-#define DRM_MODE_COLORIMETRY_XVYCC_601			3
-#define DRM_MODE_COLORIMETRY_XVYCC_709			4
-#define DRM_MODE_COLORIMETRY_SYCC_601			5
-#define DRM_MODE_COLORIMETRY_OPYCC_601			6
-#define DRM_MODE_COLORIMETRY_OPRGB			7
-#define DRM_MODE_COLORIMETRY_BT2020_CYCC		8
-#define DRM_MODE_COLORIMETRY_BT2020_RGB			9
-#define DRM_MODE_COLORIMETRY_BT2020_YCC			10
-/* Additional Colorimetry extension added as part of CTA 861.G */
-#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65		11
-#define DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER		12
-/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
-#define DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED		13
-#define DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT		14
-#define DRM_MODE_COLORIMETRY_BT601_YCC			15
+enum drm_colorspace {
+	/* For Default case, driver will set the colorspace */
+	DRM_MODE_COLORIMETRY_DEFAULT,
+	/* CEA 861 Normal Colorimetry options */
+	DRM_MODE_COLORIMETRY_SMPTE_170M_YCC,
+	DRM_MODE_COLORIMETRY_BT709_YCC,
+	/* CEA 861 Extended Colorimetry Options */
+	DRM_MODE_COLORIMETRY_XVYCC_601,
+	DRM_MODE_COLORIMETRY_XVYCC_709,
+	DRM_MODE_COLORIMETRY_SYCC_601,
+	DRM_MODE_COLORIMETRY_OPYCC_601,
+	DRM_MODE_COLORIMETRY_OPRGB,
+	DRM_MODE_COLORIMETRY_BT2020_CYCC,
+	DRM_MODE_COLORIMETRY_BT2020_RGB,
+	DRM_MODE_COLORIMETRY_BT2020_YCC,
+	/* Additional Colorimetry extension added as part of CTA 861.G */
+	DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65,
+	DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER,
+	/* Additional Colorimetry Options added for DP 1.4a VSC Colorimetry Format */
+	DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
+	DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
+	DRM_MODE_COLORIMETRY_BT601_YCC,
+};
 
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info
@@ -825,7 +826,7 @@ struct drm_connector_state {
 	 * colorspace change on Sink. This is most commonly used to switch
 	 * to wider color gamuts like BT2020.
 	 */
-	u32 colorspace;
+	enum drm_colorspace colorspace;
 
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
-- 
2.39.0

Re: [PATCH v2 04/21] drm/connector: Convert DRM_MODE_COLORIMETRY to enum

[Simon Ser]

Reviewed-by: Simon Ser <contact@emersion.fr>

[PATCH v2 05/21] drm/connector: Pull out common create_colorspace_property code

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/drm_connector.c | 54 ++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 61c29ce74b03..ddba0b9fcc17 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1971,33 +1971,44 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
  * drm_mode_create_dp_colorspace_property() is used for DP connector.
  */
 
-/**
- * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
- * @connector: connector to create the Colorspace property on.
- *
- * Called by a driver the first time it's needed, must be attached to desired
- * HDMI connectors.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector)
+static int drm_mode_create_colorspace_property(struct drm_connector *connector,
+					const struct drm_prop_enum_list *colorspaces,
+					int size)
 {
 	struct drm_device *dev = connector->dev;
 
 	if (connector->colorspace_property)
 		return 0;
 
+	if (!colorspaces)
+		return 0;
+
 	connector->colorspace_property =
 		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
-					 hdmi_colorspaces,
-					 ARRAY_SIZE(hdmi_colorspaces));
+					colorspaces,
+					size);
 
 	if (!connector->colorspace_property)
 		return -ENOMEM;
 
 	return 0;
 }
+/**
+ * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
+ * @connector: connector to create the Colorspace property on.
+ *
+ * Called by a driver the first time it's needed, must be attached to desired
+ * HDMI connectors.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector)
+{
+	return drm_mode_create_colorspace_property(connector,
+						   hdmi_colorspaces,
+						   ARRAY_SIZE(hdmi_colorspaces));
+}
 EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
 
 /**
@@ -2012,20 +2023,9 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
  */
 int drm_mode_create_dp_colorspace_property(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-
-	if (connector->colorspace_property)
-		return 0;
-
-	connector->colorspace_property =
-		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
-					 dp_colorspaces,
-					 ARRAY_SIZE(dp_colorspaces));
-
-	if (!connector->colorspace_property)
-		return -ENOMEM;
-
-	return 0;
+	return drm_mode_create_colorspace_property(connector,
+						   dp_colorspaces,
+						   ARRAY_SIZE(dp_colorspaces));
 }
 EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property);
 
-- 
2.39.0

[PATCH v2 06/21] drm/connector: Allow drivers to pass list of supported colorspaces

[Harry Wentland]

Drivers might not support all colorspaces defined in
dp_colorspaces and hdmi_colorspaces. This results in
undefined behavior when userspace is setting an
unsupported colorspace.

Allow drivers to pass the list of supported colorspaces
when creating the colorspace property.

v2:
 - Use 0 to indicate support for all colorspaces (Jani)
 - Print drm_dbg_kms message when drivers pass 0
   to signal that drivers should specify supported
   colorspaecs explicity (Jani)

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/drm_connector.c               | 148 ++++++++++--------
 .../gpu/drm/i915/display/intel_connector.c    |   4 +-
 drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
 include/drm/drm_connector.h                   |   8 +-
 4 files changed, 91 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index ddba0b9fcc17..8e81105fb2ab 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1012,64 +1012,57 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 		 drm_dp_subconnector_enum_list)
 
-static const struct drm_prop_enum_list hdmi_colorspaces[] = {
-	/* For Default case, driver will set the colorspace */
-	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
-	/* Standard Definition Colorimetry based on CEA 861 */
-	{ DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" },
-	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
-	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
-	{ DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
-	/* High Definition Colorimetry based on IEC 61966-2-4 */
-	{ DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
-	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
-	{ DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
-	/* Colorimetry based on IEC 61966-2-5 [33] */
-	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
-	/* Colorimetry based on IEC 61966-2-5 */
-	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
-	/* Added as part of Additional Colorimetry Extension in 861.G */
-	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
-	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
+static const char * const colorspace_names[] = {
+	[DRM_MODE_COLORIMETRY_DEFAULT] = "Default",
+	[DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC",
+	[DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC",
+	[DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601",
+	[DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709",
+	[DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601",
+	[DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601",
+	[DRM_MODE_COLORIMETRY_OPRGB] = "opRGB",
+	[DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC",
+	[DRM_MODE_COLORIMETRY_BT2020_RGB] = "BT2020_RGB",
+	[DRM_MODE_COLORIMETRY_BT2020_YCC] = "BT2020_YCC",
+	[DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "P3_RGB_D65",
+	[DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "P3_RGB_Theater",
+	[DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED",
+	[DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT",
+	[DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
 };
 
+static const u32 hdmi_colorspaces =
+	BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
+	BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_OPRGB) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
+	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
+
 /*
  * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry
  * Format Table 2-120
  */
-static const struct drm_prop_enum_list dp_colorspaces[] = {
-	/* For Default case, driver will set the colorspace */
-	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
-	{ DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, "RGB_Wide_Gamut_Fixed_Point" },
-	/* Colorimetry based on scRGB (IEC 61966-2-2) */
-	{ DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT, "RGB_Wide_Gamut_Floating_Point" },
-	/* Colorimetry based on IEC 61966-2-5 */
-	{ DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
-	/* Colorimetry based on SMPTE RP 431-2 */
-	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
-	{ DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
-	{ DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
-	/* Standard Definition Colorimetry based on IEC 61966-2-4 */
-	{ DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
-	/* High Definition Colorimetry based on IEC 61966-2-4 */
-	{ DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
-	/* Colorimetry based on IEC 61966-2-1/Amendment 1 */
-	{ DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
-	/* Colorimetry based on IEC 61966-2-5 [33] */
-	{ DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
-	/* Colorimetry based on ITU-R BT.2020 */
-	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
-};
+static const u32 dp_colorspaces =
+	BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED) |
+	BIT(DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT) |
+	BIT(DRM_MODE_COLORIMETRY_OPRGB) |
+	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
+	BIT(DRM_MODE_COLORIMETRY_BT601_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
+	BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC);
 
 /**
  * DOC: standard connector properties
@@ -1972,30 +1965,49 @@ EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
  */
 
 static int drm_mode_create_colorspace_property(struct drm_connector *connector,
-					const struct drm_prop_enum_list *colorspaces,
-					int size)
+					u32 supported_colorspaces)
 {
 	struct drm_device *dev = connector->dev;
+	u32 colorspaces = supported_colorspaces | BIT(DRM_MODE_COLORIMETRY_DEFAULT);
+	struct drm_prop_enum_list enum_list[DRM_MODE_COLORIMETRY_MAX];
+	int i, len;
 
 	if (connector->colorspace_property)
 		return 0;
 
-	if (!colorspaces)
-		return 0;
+	if (!supported_colorspaces)
+		drm_dbg_kms(dev, "Driver is not passing supported colorspaces on [CONNECTOR:%d:%s]\n",
+			    connector->base.id, connector->name);
+
+	if ((supported_colorspaces & -BIT(DRM_MODE_COLORIMETRY_MAX)) != 0)
+		return -EINVAL;
+
+	len = 0;
+	for (i = 0; i < DRM_MODE_COLORIMETRY_MAX; i++) {
+		if (supported_colorspaces != 0 && (colorspaces & BIT(i)) == 0)
+			continue;
+
+		enum_list[len].type = i;
+		enum_list[len].name = colorspace_names[i];
+		len++;
+	}
 
 	connector->colorspace_property =
 		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace",
-					colorspaces,
-					size);
+					enum_list,
+					len);
 
 	if (!connector->colorspace_property)
 		return -ENOMEM;
 
 	return 0;
 }
+
 /**
  * drm_mode_create_hdmi_colorspace_property - create hdmi colorspace property
  * @connector: connector to create the Colorspace property on.
+ * @supported_colorspaces: A bitfield of supported colorspaces or 0 for all
+ *                         HDMI colorspaces
  *
  * Called by a driver the first time it's needed, must be attached to desired
  * HDMI connectors.
@@ -2003,17 +2015,20 @@ static int drm_mode_create_colorspace_property(struct drm_connector *connector,
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector)
+int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
+					     u32 supported_colorspaces)
 {
-	return drm_mode_create_colorspace_property(connector,
-						   hdmi_colorspaces,
-						   ARRAY_SIZE(hdmi_colorspaces));
+	u32 colorspaces = supported_colorspaces & hdmi_colorspaces;
+
+	return drm_mode_create_colorspace_property(connector, colorspaces);
 }
 EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
 
 /**
  * drm_mode_create_dp_colorspace_property - create dp colorspace property
  * @connector: connector to create the Colorspace property on.
+ * @supported_colorspaces: A bitfield of supported colorspaces or 0 for all
+ *                         DP colorspaces
  *
  * Called by a driver the first time it's needed, must be attached to desired
  * DP connectors.
@@ -2021,11 +2036,12 @@ EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int drm_mode_create_dp_colorspace_property(struct drm_connector *connector)
+int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
+					   u32 supported_colorspaces)
 {
-	return drm_mode_create_colorspace_property(connector,
-						   dp_colorspaces,
-						   ARRAY_SIZE(dp_colorspaces));
+	u32 colorspaces = supported_colorspaces & dp_colorspaces;
+
+	return drm_mode_create_colorspace_property(connector, colorspaces);
 }
 EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property);
 
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 6d5cbeb8df4d..9e4b054266ea 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -283,13 +283,13 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
 void
 intel_attach_hdmi_colorspace_property(struct drm_connector *connector)
 {
-	if (!drm_mode_create_hdmi_colorspace_property(connector))
+	if (!drm_mode_create_hdmi_colorspace_property(connector, 0))
 		drm_connector_attach_colorspace_property(connector);
 }
 
 void
 intel_attach_dp_colorspace_property(struct drm_connector *connector)
 {
-	if (!drm_mode_create_dp_colorspace_property(connector))
+	if (!drm_mode_create_dp_colorspace_property(connector, 0))
 		drm_connector_attach_colorspace_property(connector);
 }
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 9e145690c480..95d73b817b05 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -605,7 +605,7 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	ret = drm_mode_create_hdmi_colorspace_property(connector);
+	ret = drm_mode_create_hdmi_colorspace_property(connector, 0);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index edef65388c29..5825c6ab969b 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -30,6 +30,7 @@
 #include <linux/notifier.h>
 #include <drm/drm_mode_object.h>
 #include <drm/drm_util.h>
+#include <drm/drm_property.h>
 
 #include <uapi/drm/drm_mode.h>
 
@@ -393,6 +394,7 @@ enum drm_colorspace {
 	DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED,
 	DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
 	DRM_MODE_COLORIMETRY_BT601_YCC,
+	DRM_MODE_COLORIMETRY_MAX
 };
 
 /**
@@ -1818,8 +1820,10 @@ int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
 					     struct drm_connector_state *new_state);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
-int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector);
-int drm_mode_create_dp_colorspace_property(struct drm_connector *connector);
+int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
+					     u32 supported_colorspaces);
+int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
+					   u32 supported_colorspaces);
 int drm_mode_create_content_type_property(struct drm_device *dev);
 int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
 
-- 
2.39.0

[PATCH v2 07/21] drm/connector: Print connector colorspace in state debugfs

[Harry Wentland]

v3: Fix kerneldocs (kernel test robot)

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/drm_atomic.c    |  1 +
 drivers/gpu/drm/drm_connector.c | 15 +++++++++++++++
 include/drm/drm_connector.h     |  1 +
 3 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c0dc5858a723..d6d04c4ccfc0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1071,6 +1071,7 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
 	drm_printf(p, "\tself_refresh_aware=%d\n", state->self_refresh_aware);
 	drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc);
+	drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace));
 
 	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
 		if (state->writeback_job && state->writeback_job->fb)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 8e81105fb2ab..913e50a8bb38 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1031,6 +1031,21 @@ static const char * const colorspace_names[] = {
 	[DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
 };
 
+/**
+ * drm_get_colorspace_name - return a string for color encoding
+ * @colorspace: color space to compute name of
+ *
+ * In contrast to the other drm_get_*_name functions this one here returns a
+ * const pointer and hence is threadsafe.
+ */
+const char *drm_get_colorspace_name(enum drm_colorspace colorspace)
+{
+	if (WARN_ON(colorspace >= ARRAY_SIZE(colorspace_names)))
+		return "unknown";
+
+	return colorspace_names[colorspace];
+}
+
 static const u32 hdmi_colorspaces =
 	BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) |
 	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5825c6ab969b..545eb6eb456a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1906,6 +1906,7 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
 
 bool drm_connector_has_possible_encoder(struct drm_connector *connector,
 					struct drm_encoder *encoder);
+const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
 
 /**
  * drm_for_each_connector_iter - connector_list iterator macro
-- 
2.39.0

[PATCH v2 08/21] drm/amd/display: Always pass connector_state to stream validation

[Harry Wentland]

We need the connector_state for colorspace and scaling information
and can get it from connector->state.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b8638f0508b0..bc10ac5e772d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5776,15 +5776,14 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 {
 	struct drm_display_mode *preferred_mode = NULL;
 	struct drm_connector *drm_connector;
-	const struct drm_connector_state *con_state =
-		dm_state ? &dm_state->base : NULL;
+	const struct drm_connector_state *con_state = &dm_state->base;
 	struct dc_stream_state *stream = NULL;
 	struct drm_display_mode mode;
 	struct drm_display_mode saved_mode;
 	struct drm_display_mode *freesync_mode = NULL;
 	bool native_mode_found = false;
 	bool recalculate_timing = false;
-	bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
+	bool scale = dm_state->scaling != RMX_OFF;
 	int mode_refresh;
 	int preferred_refresh = 0;
 	enum color_transfer_func tf = TRANSFER_FUNC_UNKNOWN;
@@ -6397,7 +6396,9 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec
 		goto fail;
 	}
 
-	stream = create_validate_stream_for_sink(aconnector, mode, NULL, NULL);
+	stream = create_validate_stream_for_sink(aconnector, mode,
+						 to_dm_connector_state(connector->state),
+						 NULL);
 	if (stream) {
 		dc_stream_release(stream);
 		result = MODE_OK;
-- 
2.39.0

[PATCH v2 09/21] drm/amd/display: Register Colorspace property for DP and HDMI

[Harry Wentland]

We want compositors to be able to set the output
colorspace on DP and HDMI outputs, based on the
caps reported from the receiver via EDID.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index bc10ac5e772d..c311135f1e6f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7035,6 +7035,12 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 	return amdgpu_dm_connector->num_modes;
 }
 
+static const u32 supported_colorspaces =
+	BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
+	BIT(DRM_MODE_COLORIMETRY_OPRGB) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
+	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC);
+
 void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				     struct amdgpu_dm_connector *aconnector,
 				     int connector_type,
@@ -7112,6 +7118,15 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.abm_level_property, 0);
 	}
 
+	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
+		if (!drm_mode_create_hdmi_colorspace_property(&aconnector->base, supported_colorspaces))
+			drm_connector_attach_colorspace_property(&aconnector->base);
+	} else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
+		   connector_type == DRM_MODE_CONNECTOR_eDP) {
+		if (!drm_mode_create_dp_colorspace_property(&aconnector->base, supported_colorspaces))
+			drm_connector_attach_colorspace_property(&aconnector->base);
+	}
+
 	if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
 	    connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
 	    connector_type == DRM_MODE_CONNECTOR_eDP) {
-- 
2.39.0

[PATCH v2 10/21] drm/amd/display: Signal mode_changed if colorspace changed

[Harry Wentland]

We need to signal mode_changed to make sure we update the output
colorspace.

v2: No need to call drm_hdmi_avi_infoframe_colorimetry as DC does its
    own infoframe packing.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c311135f1e6f..f74462c282a6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6492,6 +6492,14 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
 	if (!crtc)
 		return 0;
 
+	if (new_con_state->colorspace != old_con_state->colorspace) {
+		new_crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(new_crtc_state))
+			return PTR_ERR(new_crtc_state);
+
+		new_crtc_state->mode_changed = true;
+	}
+
 	if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state)) {
 		struct dc_info_packet hdr_infopacket;
 
@@ -6514,7 +6522,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
 		 * set is permissible, however. So only force a
 		 * modeset if we're entering or exiting HDR.
 		 */
-		new_crtc_state->mode_changed =
+		new_crtc_state->mode_changed = new_crtc_state->mode_changed ||
 			!old_con_state->hdr_output_metadata ||
 			!new_con_state->hdr_output_metadata;
 	}
-- 
2.39.0

Re: [PATCH v2 10/21] drm/amd/display: Signal mode_changed if colorspace changed

[Leo Li]

On 1/13/23 11:24, Harry Wentland wrote:
> We need to signal mode_changed to make sure we update the output
> colorspace.
> 
> v2: No need to call drm_hdmi_avi_infoframe_colorimetry as DC does its
>      own infoframe packing.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org

Reviewed-by: Leo Li <sunpeng.li@amd.com>

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index c311135f1e6f..f74462c282a6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6492,6 +6492,14 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
>   	if (!crtc)
>   		return 0;
>   
> +	if (new_con_state->colorspace != old_con_state->colorspace) {
> +		new_crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(new_crtc_state))
> +			return PTR_ERR(new_crtc_state);
> +
> +		new_crtc_state->mode_changed = true;
> +	}
> +
>   	if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state)) {
>   		struct dc_info_packet hdr_infopacket;
>   
> @@ -6514,7 +6522,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
>   		 * set is permissible, however. So only force a
>   		 * modeset if we're entering or exiting HDR.
>   		 */
> -		new_crtc_state->mode_changed =
> +		new_crtc_state->mode_changed = new_crtc_state->mode_changed ||
>   			!old_con_state->hdr_output_metadata ||
>   			!new_con_state->hdr_output_metadata;
>   	}

[PATCH v2 11/21] drm/amd/display: Send correct DP colorspace infopacket

[Harry Wentland]

Look at connector->colorimetry to determine output colorspace.

We don't want to impact current SDR behavior, so
DRM_MODE_COLORIMETRY_DEFAULT preserves current behavior.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++++++++++--------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index f74462c282a6..a31f71f2feca 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5162,21 +5162,21 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
 }
 
 static enum dc_color_space
-get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
+get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
+		       const struct drm_connector_state *connector_state)
 {
 	enum dc_color_space color_space = COLOR_SPACE_SRGB;
 
-	switch (dc_crtc_timing->pixel_encoding)	{
-	case PIXEL_ENCODING_YCBCR422:
-	case PIXEL_ENCODING_YCBCR444:
-	case PIXEL_ENCODING_YCBCR420:
-	{
+	switch (connector_state->colorspace) {
+	case DRM_MODE_COLORIMETRY_DEFAULT: // ITU601
+		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) {
+			color_space = COLOR_SPACE_SRGB;
 		/*
 		 * 27030khz is the separation point between HDTV and SDTV
 		 * according to HDMI spec, we use YCbCr709 and YCbCr601
 		 * respectively
 		 */
-		if (dc_crtc_timing->pix_clk_100hz > 270300) {
+		} else if (dc_crtc_timing->pix_clk_100hz > 270300) {
 			if (dc_crtc_timing->flags.Y_ONLY)
 				color_space =
 					COLOR_SPACE_YCBCR709_LIMITED;
@@ -5189,15 +5189,21 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
 			else
 				color_space = COLOR_SPACE_YCBCR601;
 		}
-
-	}
-	break;
-	case PIXEL_ENCODING_RGB:
-		color_space = COLOR_SPACE_SRGB;
 		break;
-
-	default:
-		WARN_ON(1);
+	case DRM_MODE_COLORIMETRY_BT709_YCC:
+		if (dc_crtc_timing->flags.Y_ONLY)
+			color_space = COLOR_SPACE_YCBCR709_LIMITED;
+		else
+			color_space = COLOR_SPACE_YCBCR709;
+		break;
+	case DRM_MODE_COLORIMETRY_OPRGB:
+		color_space = COLOR_SPACE_ADOBERGB;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_RGB:
+		color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_YCC:
+		color_space = COLOR_SPACE_2020_YCBCR;
 		break;
 	}
 
@@ -5325,7 +5331,7 @@ static void fill_stream_properties_from_drm_display_mode(
 
 	timing_out->aspect_ratio = get_aspect_ratio(mode_in);
 
-	stream->output_color_space = get_output_color_space(timing_out);
+	stream->output_color_space = get_output_color_space(timing_out, connector_state);
 
 	stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
 	stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
-- 
2.39.0

[PATCH v2 12/21] drm/amd/display: Always set crtcinfo from create_stream_for_sink

[Harry Wentland]

From: Joshua Ashton <joshua@froggi.es>

Given that we always pass dm_state into here now, this won't ever
trigger anymore.

This is needed for we will always fail mode validation with invalid
clocks or link bandwidth errors.

Signed-off-by: Joshua Ashton <joshua@froggi.es>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a31f71f2feca..fc94f4872397 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5870,7 +5870,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 
 	if (recalculate_timing)
 		drm_mode_set_crtcinfo(&saved_mode, 0);
-	else if (!dm_state)
+	else
 		drm_mode_set_crtcinfo(&mode, 0);
 
 	/*
-- 
2.39.0

[PATCH v2 13/21] drm/amd/display: Add support for explicit BT601_YCC

[Harry Wentland]

We use this by default but if userspace passes this explicitly
we should respect it.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index fc94f4872397..d2921d2179cc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5190,6 +5190,12 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
 				color_space = COLOR_SPACE_YCBCR601;
 		}
 		break;
+	case DRM_MODE_COLORIMETRY_BT601_YCC:
+		if (dc_crtc_timing->flags.Y_ONLY)
+			color_space = COLOR_SPACE_YCBCR601_LIMITED;
+		else
+			color_space = COLOR_SPACE_YCBCR601;
+		break;
 	case DRM_MODE_COLORIMETRY_BT709_YCC:
 		if (dc_crtc_timing->flags.Y_ONLY)
 			color_space = COLOR_SPACE_YCBCR709_LIMITED;
-- 
2.39.0

[PATCH v2 14/21] drm/amd/display: Add debugfs for testing output colorspace

[Harry Wentland]

In order to IGT test colorspace we'll want to print
the currently enabled colorspace on a stream. We add
a new debugfs to do so, using the same scheme as
current bpc reporting.

This might also come in handy when debugging display
issues.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index a29952cd8f22..5473f022d9ed 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -935,6 +935,61 @@ static int amdgpu_current_bpc_show(struct seq_file *m, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(amdgpu_current_bpc);
 
+/*
+ * Returns the current bpc for the crtc.
+ * Example usage: cat /sys/kernel/debug/dri/0/crtc-0/amdgpu_current_colorspace
+ */
+static int amdgpu_current_colorspace_show(struct seq_file *m, void *data)
+{
+	struct drm_crtc *crtc = m->private;
+	struct drm_device *dev = crtc->dev;
+	struct dm_crtc_state *dm_crtc_state = NULL;
+	int res = -ENODEV;
+
+	mutex_lock(&dev->mode_config.mutex);
+	drm_modeset_lock(&crtc->mutex, NULL);
+	if (crtc->state == NULL)
+		goto unlock;
+
+	dm_crtc_state = to_dm_crtc_state(crtc->state);
+	if (dm_crtc_state->stream == NULL)
+		goto unlock;
+
+	switch (dm_crtc_state->stream->output_color_space) {
+	case COLOR_SPACE_SRGB:
+		seq_printf(m, "RGB");
+		break;
+	case COLOR_SPACE_YCBCR601:
+	case COLOR_SPACE_YCBCR601_LIMITED:
+		seq_printf(m, "BT601_YCC");
+		break;
+	case COLOR_SPACE_YCBCR709:
+	case COLOR_SPACE_YCBCR709_LIMITED:
+		seq_printf(m, "BT709_YCC");
+		break;
+	case COLOR_SPACE_ADOBERGB:
+		seq_printf(m, "opRGB");
+		break;
+	case COLOR_SPACE_2020_RGB_FULLRANGE:
+		seq_printf(m, "BT2020_RGB");
+		break;
+	case COLOR_SPACE_2020_YCBCR:
+		seq_printf(m, "BT2020_YCC");
+		break;
+	default:
+		goto unlock;
+	}
+	res = 0;
+
+unlock:
+	drm_modeset_unlock(&crtc->mutex);
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return res;
+}
+DEFINE_SHOW_ATTRIBUTE(amdgpu_current_colorspace);
+
+
 /*
  * Example usage:
  * Disable dsc passthrough, i.e.,: have dsc decoding at converver, not external RX
@@ -3304,6 +3359,8 @@ void crtc_debugfs_init(struct drm_crtc *crtc)
 #endif
 	debugfs_create_file("amdgpu_current_bpc", 0644, crtc->debugfs_entry,
 			    crtc, &amdgpu_current_bpc_fops);
+	debugfs_create_file("amdgpu_current_colorspace", 0644, crtc->debugfs_entry,
+			    crtc, &amdgpu_current_colorspace_fops);
 }
 
 /*
-- 
2.39.0

[PATCH v2 15/21] drm/amd/display: Add default case for output_color_space switch

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton <joshua@froggi.es>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 43 ++++++++++---------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index d2921d2179cc..73a98e6e1867 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5168,7 +5168,29 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
 	enum dc_color_space color_space = COLOR_SPACE_SRGB;
 
 	switch (connector_state->colorspace) {
+	case DRM_MODE_COLORIMETRY_BT601_YCC:
+		if (dc_crtc_timing->flags.Y_ONLY)
+			color_space = COLOR_SPACE_YCBCR601_LIMITED;
+		else
+			color_space = COLOR_SPACE_YCBCR601;
+		break;
+	case DRM_MODE_COLORIMETRY_BT709_YCC:
+		if (dc_crtc_timing->flags.Y_ONLY)
+			color_space = COLOR_SPACE_YCBCR709_LIMITED;
+		else
+			color_space = COLOR_SPACE_YCBCR709;
+		break;
+	case DRM_MODE_COLORIMETRY_OPRGB:
+		color_space = COLOR_SPACE_ADOBERGB;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_RGB:
+		color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		break;
+	case DRM_MODE_COLORIMETRY_BT2020_YCC:
+		color_space = COLOR_SPACE_2020_YCBCR;
+		break;
 	case DRM_MODE_COLORIMETRY_DEFAULT: // ITU601
+	default:
 		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) {
 			color_space = COLOR_SPACE_SRGB;
 		/*
@@ -5190,27 +5212,6 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
 				color_space = COLOR_SPACE_YCBCR601;
 		}
 		break;
-	case DRM_MODE_COLORIMETRY_BT601_YCC:
-		if (dc_crtc_timing->flags.Y_ONLY)
-			color_space = COLOR_SPACE_YCBCR601_LIMITED;
-		else
-			color_space = COLOR_SPACE_YCBCR601;
-		break;
-	case DRM_MODE_COLORIMETRY_BT709_YCC:
-		if (dc_crtc_timing->flags.Y_ONLY)
-			color_space = COLOR_SPACE_YCBCR709_LIMITED;
-		else
-			color_space = COLOR_SPACE_YCBCR709;
-		break;
-	case DRM_MODE_COLORIMETRY_OPRGB:
-		color_space = COLOR_SPACE_ADOBERGB;
-		break;
-	case DRM_MODE_COLORIMETRY_BT2020_RGB:
-		color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
-		break;
-	case DRM_MODE_COLORIMETRY_BT2020_YCC:
-		color_space = COLOR_SPACE_2020_YCBCR;
-		break;
 	}
 
 	return color_space;
-- 
2.39.0

[PATCH v2 16/21] drm/amd/display: Don't restrict bpc to 8 bpc

[Harry Wentland]

This will let us pass the kms_hdr.bpc_switch IGT
test.

The reason the bpc restriction was required is
historical. At one point in time we were not falling
back to a lower bpc when we didn't have enough
bandwidth for the maximum bpc reported by a display.
This meant that we couldn't enable some high refresh
modes unless we limitted the bpc.

Starting with this patch the issue is fixed:
cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color")

This patch implemented a fallback mechanism if mode
validation failed at the max bpc. This means users
now automatically get all modes that can be supported
by at least 6 bpc. The driver will enable the mode
with the highest possible bpc that is supported by
the display.

v2:
 - explain why this is no longer needed (Michel)
 - refer to commit that fixed bpc fallback (Michel)

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Cc: Michel Dänzer <michel.daenzer@mailbox.org>
Reviewed-By: Joshua Ashton <joshua@froggi.es>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 73a98e6e1867..f74b125af31f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7130,7 +7130,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
 
 	/* This defaults to the max in the range, but we want 8bpc for non-edp. */
-	aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8;
+	aconnector->base.state->max_bpc = 16;
 	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
 
 	if (connector_type == DRM_MODE_CONNECTOR_eDP &&
-- 
2.39.0

[PATCH v2 17/21] drm/amd/display: Format input and output CSC matrix

[Harry Wentland]

Format the input and output CSC matrix so they
look like 3x4 matrixes. This will make parsing them
much easier and allows us to quickly spot potential
mistakes.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
---
 .../drm/amd/display/dc/core/dc_hw_sequencer.c | 38 ++++++++-----
 drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   | 54 +++++++++++--------
 2 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
index 471078fc3900..a70f045fc5c1 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
@@ -73,28 +73,38 @@ struct out_csc_color_matrix_type {
 
 static const struct out_csc_color_matrix_type output_csc_matrix[] = {
 	{ COLOR_SPACE_RGB_TYPE,
-		{ 0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
+		{ 0x2000, 0,      0,      0,
+		  0,      0x2000, 0,      0,
+		  0,      0,      0x2000, 0} },
 	{ COLOR_SPACE_RGB_LIMITED_TYPE,
-		{ 0x1B67, 0, 0, 0x201, 0, 0x1B67, 0, 0x201, 0, 0, 0x1B67, 0x201} },
+		{ 0x1B67, 0,      0,      0x201,
+		  0,      0x1B67, 0,      0x201,
+		  0,      0,      0x1B67, 0x201} },
 	{ COLOR_SPACE_YCBCR601_TYPE,
-		{ 0xE04, 0xF444, 0xFDB9, 0x1004, 0x831, 0x1016, 0x320, 0x201, 0xFB45,
-				0xF6B7, 0xE04, 0x1004} },
+		{ 0xE04,  0xF444, 0xFDB9, 0x1004,
+		  0x831,  0x1016, 0x320,  0x201,
+		  0xFB45, 0xF6B7, 0xE04,  0x1004} },
 	{ COLOR_SPACE_YCBCR709_TYPE,
-		{ 0xE04, 0xF345, 0xFEB7, 0x1004, 0x5D3, 0x1399, 0x1FA,
-				0x201, 0xFCCA, 0xF533, 0xE04, 0x1004} },
+		{ 0xE04,  0xF345, 0xFEB7, 0x1004,
+		  0x5D3,  0x1399, 0x1FA,  0x201,
+		  0xFCCA, 0xF533, 0xE04,  0x1004} },
 	/* TODO: correct values below */
 	{ COLOR_SPACE_YCBCR601_LIMITED_TYPE,
-		{ 0xE00, 0xF447, 0xFDB9, 0x1000, 0x991,
-				0x12C9, 0x3A6, 0x200, 0xFB47, 0xF6B9, 0xE00, 0x1000} },
+		{ 0xE00,  0xF447, 0xFDB9, 0x1000,
+		  0x991,  0x12C9, 0x3A6,  0x200,
+		  0xFB47, 0xF6B9, 0xE00,  0x1000} },
 	{ COLOR_SPACE_YCBCR709_LIMITED_TYPE,
-		{ 0xE00, 0xF349, 0xFEB7, 0x1000, 0x6CE, 0x16E3,
-				0x24F, 0x200, 0xFCCB, 0xF535, 0xE00, 0x1000} },
+		{ 0xE00, 0xF349, 0xFEB7, 0x1000,
+		  0x6CE, 0x16E3, 0x24F,  0x200,
+		  0xFCCB, 0xF535, 0xE00, 0x1000} },
 	{ COLOR_SPACE_YCBCR2020_TYPE,
-		{ 0x1000, 0xF149, 0xFEB7, 0x0000, 0x0868, 0x15B2,
-				0x01E6, 0x0000, 0xFB88, 0xF478, 0x1000, 0x0000} },
+		{ 0x1000, 0xF149, 0xFEB7, 0x0000,
+		  0x0868, 0x15B2, 0x01E6, 0x0000,
+		  0xFB88, 0xF478, 0x1000, 0x0000} },
 	{ COLOR_SPACE_YCBCR709_BLACK_TYPE,
-		{ 0x0000, 0x0000, 0x0000, 0x1000, 0x0000, 0x0000,
-				0x0000, 0x0200, 0x0000, 0x0000, 0x0000, 0x1000} },
+		{ 0x0000, 0x0000, 0x0000, 0x1000,
+		  0x0000, 0x0000, 0x0000, 0x0200,
+		  0x0000, 0x0000, 0x0000, 0x1000} },
 };
 
 static bool is_rgb_type(
diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
index 131fcfa28bca..f4aa76e02518 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
@@ -70,28 +70,38 @@ struct dpp_input_csc_matrix {
 };
 
 static const struct dpp_input_csc_matrix __maybe_unused dpp_input_csc_matrix[] = {
-	{COLOR_SPACE_SRGB,
-		{0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
-	{COLOR_SPACE_SRGB_LIMITED,
-		{0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
-	{COLOR_SPACE_YCBCR601,
-		{0x2cdd, 0x2000, 0, 0xe991, 0xe926, 0x2000, 0xf4fd, 0x10ef,
-						0, 0x2000, 0x38b4, 0xe3a6} },
-	{COLOR_SPACE_YCBCR601_LIMITED,
-		{0x3353, 0x2568, 0, 0xe400, 0xe5dc, 0x2568, 0xf367, 0x1108,
-						0, 0x2568, 0x40de, 0xdd3a} },
-	{COLOR_SPACE_YCBCR709,
-		{0x3265, 0x2000, 0, 0xe6ce, 0xf105, 0x2000, 0xfa01, 0xa7d, 0,
-						0x2000, 0x3b61, 0xe24f} },
-	{COLOR_SPACE_YCBCR709_LIMITED,
-		{0x39a6, 0x2568, 0, 0xe0d6, 0xeedd, 0x2568, 0xf925, 0x9a8, 0,
-						0x2568, 0x43ee, 0xdbb2} },
-	{COLOR_SPACE_2020_YCBCR,
-		{0x2F30, 0x2000, 0, 0xE869, 0xEDB7, 0x2000, 0xFABC, 0xBC6, 0,
-						0x2000, 0x3C34, 0xE1E6} },
-	{COLOR_SPACE_2020_RGB_LIMITEDRANGE,
-		{0x35E0, 0x255F, 0, 0xE2B3, 0xEB20, 0x255F, 0xF9FD, 0xB1E, 0,
-						0x255F, 0x44BD, 0xDB43} }
+	{ COLOR_SPACE_SRGB,
+		{ 0x2000, 0,      0,      0,
+		  0,      0x2000, 0,      0,
+		  0,      0,      0x2000, 0 } },
+	{ COLOR_SPACE_SRGB_LIMITED,
+		{ 0x2000, 0,      0,      0,
+		  0,      0x2000, 0,      0,
+		  0,      0,      0x2000, 0 } },
+	{ COLOR_SPACE_YCBCR601,
+		{ 0x2cdd, 0x2000, 0,      0xe991,
+		  0xe926, 0x2000, 0xf4fd, 0x10ef,
+		  0,      0x2000, 0x38b4, 0xe3a6 } },
+	{ COLOR_SPACE_YCBCR601_LIMITED,
+		{ 0x3353, 0x2568, 0,      0xe400,
+		  0xe5dc, 0x2568, 0xf367, 0x1108,
+		  0,      0x2568, 0x40de, 0xdd3a } },
+	{ COLOR_SPACE_YCBCR709,
+		{ 0x3265, 0x2000, 0,      0xe6ce,
+		  0xf105, 0x2000, 0xfa01, 0xa7d,
+		  0,      0x2000, 0x3b61, 0xe24f } },
+	{ COLOR_SPACE_YCBCR709_LIMITED,
+		{ 0x39a6, 0x2568, 0,      0xe0d6,
+		  0xeedd, 0x2568, 0xf925, 0x9a8,
+		  0,      0x2568, 0x43ee, 0xdbb2 } },
+	{ COLOR_SPACE_2020_YCBCR,
+		{ 0x2F30, 0x2000, 0,      0xE869,
+		  0xEDB7, 0x2000, 0xFABC, 0xBC6,
+		  0,      0x2000, 0x3C34, 0xE1E6 } },
+	{ COLOR_SPACE_2020_RGB_LIMITEDRANGE,
+		{ 0x35E0, 0x255F, 0,      0xE2B3,
+		  0xEB20, 0x255F, 0xF9FD, 0xB1E,
+		  0,      0x255F, 0x44BD, 0xDB43 } }
 };
 
 struct dpp_grph_csc_adjustment {
-- 
2.39.0

Re: [PATCH v2 17/21] drm/amd/display: Format input and output CSC matrix

[Leo Li]

On 1/13/23 11:24, Harry Wentland wrote:
> Format the input and output CSC matrix so they
> look like 3x4 matrixes. This will make parsing them
> much easier and allows us to quickly spot potential
> mistakes.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org

Reviewed-by: Leo Li <sunpeng.li@amd.com>

> ---
>   .../drm/amd/display/dc/core/dc_hw_sequencer.c | 38 ++++++++-----
>   drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   | 54 +++++++++++--------
>   2 files changed, 56 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
> index 471078fc3900..a70f045fc5c1 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
> @@ -73,28 +73,38 @@ struct out_csc_color_matrix_type {
>   
>   static const struct out_csc_color_matrix_type output_csc_matrix[] = {
>   	{ COLOR_SPACE_RGB_TYPE,
> -		{ 0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
> +		{ 0x2000, 0,      0,      0,
> +		  0,      0x2000, 0,      0,
> +		  0,      0,      0x2000, 0} },
>   	{ COLOR_SPACE_RGB_LIMITED_TYPE,
> -		{ 0x1B67, 0, 0, 0x201, 0, 0x1B67, 0, 0x201, 0, 0, 0x1B67, 0x201} },
> +		{ 0x1B67, 0,      0,      0x201,
> +		  0,      0x1B67, 0,      0x201,
> +		  0,      0,      0x1B67, 0x201} },
>   	{ COLOR_SPACE_YCBCR601_TYPE,
> -		{ 0xE04, 0xF444, 0xFDB9, 0x1004, 0x831, 0x1016, 0x320, 0x201, 0xFB45,
> -				0xF6B7, 0xE04, 0x1004} },
> +		{ 0xE04,  0xF444, 0xFDB9, 0x1004,
> +		  0x831,  0x1016, 0x320,  0x201,
> +		  0xFB45, 0xF6B7, 0xE04,  0x1004} },
>   	{ COLOR_SPACE_YCBCR709_TYPE,
> -		{ 0xE04, 0xF345, 0xFEB7, 0x1004, 0x5D3, 0x1399, 0x1FA,
> -				0x201, 0xFCCA, 0xF533, 0xE04, 0x1004} },
> +		{ 0xE04,  0xF345, 0xFEB7, 0x1004,
> +		  0x5D3,  0x1399, 0x1FA,  0x201,
> +		  0xFCCA, 0xF533, 0xE04,  0x1004} },
>   	/* TODO: correct values below */
>   	{ COLOR_SPACE_YCBCR601_LIMITED_TYPE,
> -		{ 0xE00, 0xF447, 0xFDB9, 0x1000, 0x991,
> -				0x12C9, 0x3A6, 0x200, 0xFB47, 0xF6B9, 0xE00, 0x1000} },
> +		{ 0xE00,  0xF447, 0xFDB9, 0x1000,
> +		  0x991,  0x12C9, 0x3A6,  0x200,
> +		  0xFB47, 0xF6B9, 0xE00,  0x1000} },
>   	{ COLOR_SPACE_YCBCR709_LIMITED_TYPE,
> -		{ 0xE00, 0xF349, 0xFEB7, 0x1000, 0x6CE, 0x16E3,
> -				0x24F, 0x200, 0xFCCB, 0xF535, 0xE00, 0x1000} },
> +		{ 0xE00, 0xF349, 0xFEB7, 0x1000,
> +		  0x6CE, 0x16E3, 0x24F,  0x200,
> +		  0xFCCB, 0xF535, 0xE00, 0x1000} },
>   	{ COLOR_SPACE_YCBCR2020_TYPE,
> -		{ 0x1000, 0xF149, 0xFEB7, 0x0000, 0x0868, 0x15B2,
> -				0x01E6, 0x0000, 0xFB88, 0xF478, 0x1000, 0x0000} },
> +		{ 0x1000, 0xF149, 0xFEB7, 0x0000,
> +		  0x0868, 0x15B2, 0x01E6, 0x0000,
> +		  0xFB88, 0xF478, 0x1000, 0x0000} },
>   	{ COLOR_SPACE_YCBCR709_BLACK_TYPE,
> -		{ 0x0000, 0x0000, 0x0000, 0x1000, 0x0000, 0x0000,
> -				0x0000, 0x0200, 0x0000, 0x0000, 0x0000, 0x1000} },
> +		{ 0x0000, 0x0000, 0x0000, 0x1000,
> +		  0x0000, 0x0000, 0x0000, 0x0200,
> +		  0x0000, 0x0000, 0x0000, 0x1000} },
>   };
>   
>   static bool is_rgb_type(
> diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
> index 131fcfa28bca..f4aa76e02518 100644
> --- a/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
> +++ b/drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h
> @@ -70,28 +70,38 @@ struct dpp_input_csc_matrix {
>   };
>   
>   static const struct dpp_input_csc_matrix __maybe_unused dpp_input_csc_matrix[] = {
> -	{COLOR_SPACE_SRGB,
> -		{0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
> -	{COLOR_SPACE_SRGB_LIMITED,
> -		{0x2000, 0, 0, 0, 0, 0x2000, 0, 0, 0, 0, 0x2000, 0} },
> -	{COLOR_SPACE_YCBCR601,
> -		{0x2cdd, 0x2000, 0, 0xe991, 0xe926, 0x2000, 0xf4fd, 0x10ef,
> -						0, 0x2000, 0x38b4, 0xe3a6} },
> -	{COLOR_SPACE_YCBCR601_LIMITED,
> -		{0x3353, 0x2568, 0, 0xe400, 0xe5dc, 0x2568, 0xf367, 0x1108,
> -						0, 0x2568, 0x40de, 0xdd3a} },
> -	{COLOR_SPACE_YCBCR709,
> -		{0x3265, 0x2000, 0, 0xe6ce, 0xf105, 0x2000, 0xfa01, 0xa7d, 0,
> -						0x2000, 0x3b61, 0xe24f} },
> -	{COLOR_SPACE_YCBCR709_LIMITED,
> -		{0x39a6, 0x2568, 0, 0xe0d6, 0xeedd, 0x2568, 0xf925, 0x9a8, 0,
> -						0x2568, 0x43ee, 0xdbb2} },
> -	{COLOR_SPACE_2020_YCBCR,
> -		{0x2F30, 0x2000, 0, 0xE869, 0xEDB7, 0x2000, 0xFABC, 0xBC6, 0,
> -						0x2000, 0x3C34, 0xE1E6} },
> -	{COLOR_SPACE_2020_RGB_LIMITEDRANGE,
> -		{0x35E0, 0x255F, 0, 0xE2B3, 0xEB20, 0x255F, 0xF9FD, 0xB1E, 0,
> -						0x255F, 0x44BD, 0xDB43} }
> +	{ COLOR_SPACE_SRGB,
> +		{ 0x2000, 0,      0,      0,
> +		  0,      0x2000, 0,      0,
> +		  0,      0,      0x2000, 0 } },
> +	{ COLOR_SPACE_SRGB_LIMITED,
> +		{ 0x2000, 0,      0,      0,
> +		  0,      0x2000, 0,      0,
> +		  0,      0,      0x2000, 0 } },
> +	{ COLOR_SPACE_YCBCR601,
> +		{ 0x2cdd, 0x2000, 0,      0xe991,
> +		  0xe926, 0x2000, 0xf4fd, 0x10ef,
> +		  0,      0x2000, 0x38b4, 0xe3a6 } },
> +	{ COLOR_SPACE_YCBCR601_LIMITED,
> +		{ 0x3353, 0x2568, 0,      0xe400,
> +		  0xe5dc, 0x2568, 0xf367, 0x1108,
> +		  0,      0x2568, 0x40de, 0xdd3a } },
> +	{ COLOR_SPACE_YCBCR709,
> +		{ 0x3265, 0x2000, 0,      0xe6ce,
> +		  0xf105, 0x2000, 0xfa01, 0xa7d,
> +		  0,      0x2000, 0x3b61, 0xe24f } },
> +	{ COLOR_SPACE_YCBCR709_LIMITED,
> +		{ 0x39a6, 0x2568, 0,      0xe0d6,
> +		  0xeedd, 0x2568, 0xf925, 0x9a8,
> +		  0,      0x2568, 0x43ee, 0xdbb2 } },
> +	{ COLOR_SPACE_2020_YCBCR,
> +		{ 0x2F30, 0x2000, 0,      0xE869,
> +		  0xEDB7, 0x2000, 0xFABC, 0xBC6,
> +		  0,      0x2000, 0x3C34, 0xE1E6 } },
> +	{ COLOR_SPACE_2020_RGB_LIMITEDRANGE,
> +		{ 0x35E0, 0x255F, 0,      0xE2B3,
> +		  0xEB20, 0x255F, 0xF9FD, 0xB1E,
> +		  0,      0x255F, 0x44BD, 0xDB43 } }
>   };
>   
>   struct dpp_grph_csc_adjustment {

[PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Harry Wentland]

From: Joshua Ashton <joshua@froggi.es>

Userspace might not aware whether we're sending RGB or YCbCr
data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
requested but the output encoding is YCbCr we should
send COLOR_SPACE_2020_YCBCR.

Signed-off-by: Joshua Ashton <joshua@froggi.es>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index f74b125af31f..16940ea61b59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
 		color_space = COLOR_SPACE_ADOBERGB;
 		break;
 	case DRM_MODE_COLORIMETRY_BT2020_RGB:
-		color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
+			color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
+		else
+			color_space = COLOR_SPACE_2020_YCBCR;
 		break;
 	case DRM_MODE_COLORIMETRY_BT2020_YCC:
 		color_space = COLOR_SPACE_2020_YCBCR;
-- 
2.39.0

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Sebastian Wick]

A new property to control YCC and subsampling would be the more
complete path here. If we actually want to fix this in the short-term
though, we should handle the YCC and RGB Colorspace values as
equivalent, everywhere. Technically we're breaking the user space API
here so it should be documented on the KMS property and other drivers
must be adjusted accordingly as well.

On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
> From: Joshua Ashton <joshua@froggi.es>
>
> Userspace might not aware whether we're sending RGB or YCbCr
> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> requested but the output encoding is YCbCr we should
> send COLOR_SPACE_2020_YCBCR.
>
> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f74b125af31f..16940ea61b59 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>                 color_space = COLOR_SPACE_ADOBERGB;
>                 break;
>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> +               else
> +                       color_space = COLOR_SPACE_2020_YCBCR;
>                 break;
>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>                 color_space = COLOR_SPACE_2020_YCBCR;
> --
> 2.39.0
>

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Harry Wentland]

On 1/23/23 15:30, Sebastian Wick wrote:
> A new property to control YCC and subsampling would be the more
> complete path here. If we actually want to fix this in the short-term
> though, we should handle the YCC and RGB Colorspace values as
> equivalent, everywhere. Technically we're breaking the user space API
> here so it should be documented on the KMS property and other drivers
> must be adjusted accordingly as well.
> 

Could someone point me to a userspace that uses this currently?

Harry

> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
>>
>> From: Joshua Ashton <joshua@froggi.es>
>>
>> Userspace might not aware whether we're sending RGB or YCbCr
>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
>> requested but the output encoding is YCbCr we should
>> send COLOR_SPACE_2020_YCBCR.
>>
>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Vitaly.Prosyak@amd.com
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: amd-gfx@lists.freedesktop.org
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index f74b125af31f..16940ea61b59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>                 color_space = COLOR_SPACE_ADOBERGB;
>>                 break;
>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               else
>> +                       color_space = COLOR_SPACE_2020_YCBCR;
>>                 break;
>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>>                 color_space = COLOR_SPACE_2020_YCBCR;
>> --
>> 2.39.0
>>
> 

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Harry Wentland]

On 1/24/23 10:37, Harry Wentland wrote:
> 
> 
> On 1/23/23 15:30, Sebastian Wick wrote:
>> A new property to control YCC and subsampling would be the more
>> complete path here. If we actually want to fix this in the short-term
>> though, we should handle the YCC and RGB Colorspace values as
>> equivalent, everywhere. Technically we're breaking the user space API
>> here so it should be documented on the KMS property and other drivers
>> must be adjusted accordingly as well.
>>
> 
> Could someone point me to a userspace that uses this currently?
> 

To elaborate a bit more...

A driver has always had the ability to pick the wire format, whether
it'd be RGB or YCbCr (444, or 420). In some cases that selection
is required in order to satisfy bandwidth requirements. In others
we follow a certain policy to ensure similar behaviors between our
Windows and Linux drivers. I don't think it makes sense for userspace
to control this.

Based on what I see I am not convinced the entirety of the
colorspace definition has a corresponding implementation in an
upstream, canonical userspace, hence my question. Not even an IGT
test existed when I started looking at this. In the absence of a
missing userspace implementation I am not convinced we're breaking
anything. Even then, this was never implemented in amdgpu so
there is no way this regresses any existing behavior.

Harry

> Harry
> 
>> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
>>>
>>> From: Joshua Ashton <joshua@froggi.es>
>>>
>>> Userspace might not aware whether we're sending RGB or YCbCr
>>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
>>> requested but the output encoding is YCbCr we should
>>> send COLOR_SPACE_2020_YCBCR.
>>>
>>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>>> Cc: Vitaly.Prosyak@amd.com
>>> Cc: Joshua Ashton <joshua@froggi.es>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index f74b125af31f..16940ea61b59 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>>                 color_space = COLOR_SPACE_ADOBERGB;
>>>                 break;
>>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
>>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
>>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>>> +               else
>>> +                       color_space = COLOR_SPACE_2020_YCBCR;
>>>                 break;
>>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
>>>                 color_space = COLOR_SPACE_2020_YCBCR;
>>> --
>>> 2.39.0
>>>
>>
> 

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Sebastian Wick]

On Tue, Jan 24, 2023 at 7:57 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
>
> On 1/24/23 10:37, Harry Wentland wrote:
> >
> >
> > On 1/23/23 15:30, Sebastian Wick wrote:
> >> A new property to control YCC and subsampling would be the more
> >> complete path here. If we actually want to fix this in the short-term
> >> though, we should handle the YCC and RGB Colorspace values as
> >> equivalent, everywhere. Technically we're breaking the user space API
> >> here so it should be documented on the KMS property and other drivers
> >> must be adjusted accordingly as well.
> >>
> >
> > Could someone point me to a userspace that uses this currently?
> >
>
> To elaborate a bit more...
>
> A driver has always had the ability to pick the wire format, whether
> it'd be RGB or YCbCr (444, or 420). In some cases that selection
> is required in order to satisfy bandwidth requirements. In others
> we follow a certain policy to ensure similar behaviors between our
> Windows and Linux drivers. I don't think it makes sense for userspace
> to control this.

I disagree. The subsampling is degrading the image considerably in
some cases. We need control over this.

It does mean that user space has to be smart and try to reduce the
bandwidth if a KMS commit fails, but the same is true for resolution
and refresh rate right now and will be true for a min bpc property as
well.

> Based on what I see I am not convinced the entirety of the
> colorspace definition has a corresponding implementation in an
> upstream, canonical userspace, hence my question. Not even an IGT
> test existed when I started looking at this. In the absence of a
> missing userspace implementation I am not convinced we're breaking
> anything. Even then, this was never implemented in amdgpu so
> there is no way this regresses any existing behavior.

I don't think this breaks anything in practice. The change would only
break use cases where you want to set the infoframe to a variant which
does not match the wire format and that would be broken. So yes, I
agree.

We can't just remove the enum values though. If we deprecate the YCC
variants that must be documented and user space has to understand that
choosing the RGB variant will result in the kernel just doing the
"right thing".

>
> Harry
>
> > Harry
> >
> >> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
> >>>
> >>> From: Joshua Ashton <joshua@froggi.es>
> >>>
> >>> Userspace might not aware whether we're sending RGB or YCbCr
> >>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> >>> requested but the output encoding is YCbCr we should
> >>> send COLOR_SPACE_2020_YCBCR.
> >>>
> >>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> >>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >>> Cc: Vitaly.Prosyak@amd.com
> >>> Cc: Joshua Ashton <joshua@froggi.es>
> >>> Cc: dri-devel@lists.freedesktop.org
> >>> Cc: amd-gfx@lists.freedesktop.org
> >>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> >>> ---
> >>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>> index f74b125af31f..16940ea61b59 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> >>>                 color_space = COLOR_SPACE_ADOBERGB;
> >>>                 break;
> >>>         case DRM_MODE_COLORIMETRY_BT2020_RGB:
> >>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> >>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >>> +               else
> >>> +                       color_space = COLOR_SPACE_2020_YCBCR;
> >>>                 break;
> >>>         case DRM_MODE_COLORIMETRY_BT2020_YCC:
> >>>                 color_space = COLOR_SPACE_2020_YCBCR;
> >>> --
> >>> 2.39.0
> >>>
> >>
> >
>

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Joshua Ashton]

On 1/23/23 20:30, Sebastian Wick wrote:
> A new property to control YCC and subsampling would be the more
> complete path here. If we actually want to fix this in the short-term
> though, we should handle the YCC and RGB Colorspace values as
> equivalent, everywhere. Technically we're breaking the user space API
> here so it should be documented on the KMS property and other drivers
> must be adjusted accordingly as well.

I am happy with treating 2020_YCC and 2020_RGB as the same.

I think having the YCC/RGB split here in Colorspace is a mistake.
Pixel encoding should be completely separate from colorspace from a uAPI 
perspective when we want to expose that.
It's just a design flaw from when it was added as it just mirrors the 
values in the colorimetry packets 1:1. I understand why this happened, 
and I don't think it's a big deal for us to correct ourselves now:

I suggest we deprecate the _YCC variants, treat them the same as the RGB 
enum to avoid any potential uAPI breakage and key the split entirely off 
the pixel_encoding value.

That way when we do want to plumb more explicit pixel encoding down the 
line, userspace only has to manage one thing. There's no advantage for 
anything more here.

- Joshie 🐸✨

> 
> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
>>
>> From: Joshua Ashton <joshua@froggi.es>
>>
>> Userspace might not aware whether we're sending RGB or YCbCr
>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
>> requested but the output encoding is YCbCr we should
>> send COLOR_SPACE_2020_YCBCR.
>>
>> Signed-off-by: Joshua Ashton <joshua@froggi.es>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Vitaly.Prosyak@amd.com
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: amd-gfx@lists.freedesktop.org
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index f74b125af31f..16940ea61b59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>                  color_space = COLOR_SPACE_ADOBERGB;
>>                  break;
>>          case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
>> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
>> +               else
>> +                       color_space = COLOR_SPACE_2020_YCBCR;
>>                  break;
>>          case DRM_MODE_COLORIMETRY_BT2020_YCC:
>>                  color_space = COLOR_SPACE_2020_YCBCR;
>> --
>> 2.39.0
>>
> 

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Sebastian Wick]

On Wed, Jan 25, 2023 at 2:00 PM Joshua Ashton <joshua@froggi.es> wrote:
>
>
>
> On 1/23/23 20:30, Sebastian Wick wrote:
> > A new property to control YCC and subsampling would be the more
> > complete path here. If we actually want to fix this in the short-term
> > though, we should handle the YCC and RGB Colorspace values as
> > equivalent, everywhere. Technically we're breaking the user space API
> > here so it should be documented on the KMS property and other drivers
> > must be adjusted accordingly as well.
>
> I am happy with treating 2020_YCC and 2020_RGB as the same.
>
> I think having the YCC/RGB split here in Colorspace is a mistake.
> Pixel encoding should be completely separate from colorspace from a uAPI
> perspective when we want to expose that.
> It's just a design flaw from when it was added as it just mirrors the
> values in the colorimetry packets 1:1. I understand why this happened,
> and I don't think it's a big deal for us to correct ourselves now:
>
> I suggest we deprecate the _YCC variants, treat them the same as the RGB
> enum to avoid any potential uAPI breakage and key the split entirely off
> the pixel_encoding value.

Yeah, I agree. The kernel must know the wire encoding and can thus
always choose the correct variant. If anyone wants to provide a patch
I'll review it.

> That way when we do want to plumb more explicit pixel encoding down the
> line, userspace only has to manage one thing. There's no advantage for
> anything more here.
>
> - Joshie 🐸✨
>
> >
> > On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:
> >>
> >> From: Joshua Ashton <joshua@froggi.es>
> >>
> >> Userspace might not aware whether we're sending RGB or YCbCr
> >> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> >> requested but the output encoding is YCbCr we should
> >> send COLOR_SPACE_2020_YCBCR.
> >>
> >> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> >> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >> Cc: Vitaly.Prosyak@amd.com
> >> Cc: Joshua Ashton <joshua@froggi.es>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: amd-gfx@lists.freedesktop.org
> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index f74b125af31f..16940ea61b59 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> >>                  color_space = COLOR_SPACE_ADOBERGB;
> >>                  break;
> >>          case DRM_MODE_COLORIMETRY_BT2020_RGB:
> >> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> >> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >> +               else
> >> +                       color_space = COLOR_SPACE_2020_YCBCR;
> >>                  break;
> >>          case DRM_MODE_COLORIMETRY_BT2020_YCC:
> >>                  color_space = COLOR_SPACE_2020_YCBCR;
> >> --
> >> 2.39.0
> >>
> >
>

Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3426 bytes --]

On Wed, 25 Jan 2023 12:59:53 +0000
Joshua Ashton <joshua@froggi.es> wrote:

> On 1/23/23 20:30, Sebastian Wick wrote:
> > A new property to control YCC and subsampling would be the more
> > complete path here. If we actually want to fix this in the short-term
> > though, we should handle the YCC and RGB Colorspace values as
> > equivalent, everywhere. Technically we're breaking the user space API
> > here so it should be documented on the KMS property and other drivers
> > must be adjusted accordingly as well.  
> 
> I am happy with treating 2020_YCC and 2020_RGB as the same.
> 
> I think having the YCC/RGB split here in Colorspace is a mistake.
> Pixel encoding should be completely separate from colorspace from a uAPI 
> perspective when we want to expose that.
> It's just a design flaw from when it was added as it just mirrors the 
> values in the colorimetry packets 1:1. I understand why this happened, 
> and I don't think it's a big deal for us to correct ourselves now:
> 
> I suggest we deprecate the _YCC variants, treat them the same as the RGB 
> enum to avoid any potential uAPI breakage and key the split entirely off 
> the pixel_encoding value.
> 
> That way when we do want to plumb more explicit pixel encoding down the 
> line, userspace only has to manage one thing. There's no advantage for 
> anything more here.

Sounds good to me!


Thanks,
pq

> 
> - Joshie 🐸✨
> 
> > 
> > On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland <harry.wentland@amd.com> wrote:  
> >>
> >> From: Joshua Ashton <joshua@froggi.es>
> >>
> >> Userspace might not aware whether we're sending RGB or YCbCr
> >> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is
> >> requested but the output encoding is YCbCr we should
> >> send COLOR_SPACE_2020_YCBCR.
> >>
> >> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> >> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >> Cc: Vitaly.Prosyak@amd.com
> >> Cc: Joshua Ashton <joshua@froggi.es>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: amd-gfx@lists.freedesktop.org
> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index f74b125af31f..16940ea61b59 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> >>                  color_space = COLOR_SPACE_ADOBERGB;
> >>                  break;
> >>          case DRM_MODE_COLORIMETRY_BT2020_RGB:
> >> -               color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >> +               if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> >> +                       color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> >> +               else
> >> +                       color_space = COLOR_SPACE_2020_YCBCR;
> >>                  break;
> >>          case DRM_MODE_COLORIMETRY_BT2020_YCC:
> >>                  color_space = COLOR_SPACE_2020_YCBCR;
> >> --
> >> 2.39.0
> >>  
> >   


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 19/21] drm/amd/display: Refactor avi_info_frame colorimetry determination

[Harry Wentland]

From: Joshua Ashton <joshua@froggi.es>

Replace the messy two if-else chains here that were
on the same value with a switch on the enum.

Signed-off-by: Joshua Ashton <joshua@froggi.es>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 .../gpu/drm/amd/display/dc/core/dc_resource.c | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
index 06b5f49e0954..151981217c5f 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
@@ -3010,23 +3010,29 @@ static void set_avi_info_frame(
 	hdmi_info.bits.S0_S1 = scan_type;
 
 	/* C0, C1 : Colorimetry */
-	if (color_space == COLOR_SPACE_YCBCR709 ||
-			color_space == COLOR_SPACE_YCBCR709_LIMITED)
+	switch (color_space) {
+	case COLOR_SPACE_YCBCR709:
+	case COLOR_SPACE_YCBCR709_LIMITED:
 		hdmi_info.bits.C0_C1 = COLORIMETRY_ITU709;
-	else if (color_space == COLOR_SPACE_YCBCR601 ||
-			color_space == COLOR_SPACE_YCBCR601_LIMITED)
+		break;
+	case COLOR_SPACE_YCBCR601:
+	case COLOR_SPACE_YCBCR601_LIMITED:
 		hdmi_info.bits.C0_C1 = COLORIMETRY_ITU601;
-	else {
-		hdmi_info.bits.C0_C1 = COLORIMETRY_NO_DATA;
-	}
-	if (color_space == COLOR_SPACE_2020_RGB_FULLRANGE ||
-			color_space == COLOR_SPACE_2020_RGB_LIMITEDRANGE ||
-			color_space == COLOR_SPACE_2020_YCBCR) {
+		break;
+	case COLOR_SPACE_2020_RGB_FULLRANGE:
+	case COLOR_SPACE_2020_RGB_LIMITEDRANGE:
+	case COLOR_SPACE_2020_YCBCR:
 		hdmi_info.bits.EC0_EC2 = COLORIMETRYEX_BT2020RGBYCBCR;
 		hdmi_info.bits.C0_C1   = COLORIMETRY_EXTENDED;
-	} else if (color_space == COLOR_SPACE_ADOBERGB) {
+		break;
+	case COLOR_SPACE_ADOBERGB:
 		hdmi_info.bits.EC0_EC2 = COLORIMETRYEX_ADOBERGB;
 		hdmi_info.bits.C0_C1   = COLORIMETRY_EXTENDED;
+		break;
+	case COLOR_SPACE_SRGB:
+	default:
+		hdmi_info.bits.C0_C1 = COLORIMETRY_NO_DATA;
+		break;
 	}
 
 	if (pixel_encoding && color_space == COLOR_SPACE_2020_YCBCR &&
-- 
2.39.0

[PATCH v2 20/21] drm/amd/display: Calculate output_color_space after pixel encoding adjustment

[Harry Wentland]

From: Joshua Ashton <joshua@froggi.es>

Code in get_output_color_space depends on knowing the pixel encoding to
determine whether to pick between eg. COLOR_SPACE_SRGB or
COLOR_SPACE_YCBCR709 for transparent RGB -> YCbCr 4:4:4 in the driver.

Signed-off-by: Joshua Ashton <joshua@froggi.es>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 16940ea61b59..eb188487f0a7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5341,8 +5341,6 @@ static void fill_stream_properties_from_drm_display_mode(
 
 	timing_out->aspect_ratio = get_aspect_ratio(mode_in);
 
-	stream->output_color_space = get_output_color_space(timing_out, connector_state);
-
 	stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
 	stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
 	if (stream->signal == SIGNAL_TYPE_HDMI_TYPE_A) {
@@ -5353,6 +5351,8 @@ static void fill_stream_properties_from_drm_display_mode(
 			adjust_colour_depth_from_display_info(timing_out, info);
 		}
 	}
+
+	stream->output_color_space = get_output_color_space(timing_out, connector_state);
 }
 
 static void fill_audio_info(struct audio_info *audio_info,
-- 
2.39.0

[PATCH v2 21/21] drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix

[Harry Wentland]

From: Joshua Ashton <joshua@froggi.es>

Signed-off-by: Joshua Ashton <joshua@froggi.es>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Vitaly.Prosyak@amd.com
Cc: Joshua Ashton <joshua@froggi.es>
Cc: dri-devel@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
index a70f045fc5c1..2acbf692193f 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
@@ -98,9 +98,9 @@ static const struct out_csc_color_matrix_type output_csc_matrix[] = {
 		  0x6CE, 0x16E3, 0x24F,  0x200,
 		  0xFCCB, 0xF535, 0xE00, 0x1000} },
 	{ COLOR_SPACE_YCBCR2020_TYPE,
-		{ 0x1000, 0xF149, 0xFEB7, 0x0000,
-		  0x0868, 0x15B2, 0x01E6, 0x0000,
-		  0xFB88, 0xF478, 0x1000, 0x0000} },
+		{ 0x1000, 0xF149, 0xFEB7, 0x1004,
+		  0x0868, 0x15B2, 0x01E6, 0x201,
+		  0xFB88, 0xF478, 0x1000, 0x1004} },
 	{ COLOR_SPACE_YCBCR709_BLACK_TYPE,
 		{ 0x0000, 0x0000, 0x0000, 0x1000,
 		  0x0000, 0x0000, 0x0000, 0x0200,
-- 
2.39.0

Re: [PATCH v2 00/21] Enable Colorspace connector property in amdgpu

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 8110 bytes --]

On Fri, 13 Jan 2023 11:24:07 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> This patchset enables the DP and HDMI infoframe properties
> in amdgpu.
> 
> The first two patches are not completely related to the rest. The
> first patch allows for HDR_OUTPUT_METADATA with EOTFs that are
> unknown in the kernel.
> 
> The second one prints a connector's max_bpc as part of the atomic
> state debugfs print.
> 
> The following patches rework the connector colorspace code to
> 1) allow for easy printing of the colorspace in the drm_atomic
>    state debugfs, and
> 2) allow drivers to specify the supported colorspaces on a
>    connector.
> 
> The rest of the patches deal with the Colorspace enablement
> in amdgpu.
> 
> Why do drivers need to specify supported colorspaces? The amdgpu
> driver needs support for RGB-to-YCbCr conversion when we drive
> the display in YCbCr. This is currently not implemented for all
> colorspaces.
> 
> Since the Colorspace property didn't have an IGT test I added
> one to kms_hdr. The relevant patchset can be found on the IGT
> mailing list or on
> https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry
> 
> We tested v1 of the patchset and confirmed that the infoframes
> are as expected for both DP and HDMI when running the IGT
> colorimetry tests.
> 
> Open Items
> ----------
> 
> A couple comments from Pekka about colorspace documentation are
> left unaddressed. I hope they won't block merging this set but
> should still be addressed separately.
> 
> Pekka's questions really got me thinking of how this colorspace
> property should be used and working with it more closely with
> Joshua who is enabling HDR in gamescope made me wonder even more.
> 
> Uma, is there a (canonical, upstream) userspace that uses this
> property that I can look at to understand more?
> 
> One of the key challenges that is currently not addressed is that
> userspace is expected to pick a colorspace format straight from the
> list of definitions out of the DP or HDMI spec. But the kernel
> driver are the ones deciding on the output encoding (RGB, YCBCR444,
> YCBCR420, etc.). So there is no way for userspace to decide correctly
> between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC.
> 
> So we end up in a scenario where gamescope sets BT2020_RGB but we
> output YCBCR444 so have to correct the colorspace value to
> BT2020_YCC. This in turn breaks the colorspace IGT tests I
> wrote. I don't think "fixing" the IGT tests to accept this is
> the right thing to do.
> 
> The way it stands this patchset allows us to specify the output
> colorspace on amdgpu and we try to do the right thing, but I don't
> thing the way the colorspace property is defined is right. We're trying
> to expose things to userspace that should be under driver control. A
> much better approach would be to give userspace options for colorspace
> that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc.,
> and have the driver do the right thing to fill the infoframe, e.g., by
> picking BT2020_YCC if the requested colorspace is BT2020 and the
> is YCBCR444.

Hi Harry,

well explained.

Indeed, if you want to keep the driver in control of the encoding on
the monitor cable, then your suggestion seems correct (ignoring the
question whether it breaks something existing).

I do recall something about letting userspace control the encoding on
the cable though, particularly when it affects performance or quality.
E.g. 4:2:0 sub-sampling might be wanted in some cases and unwanted in
others. It's a bit similar to bpc. The trade-off may be frame rate or
resolution. It might better to know that the hardware cannot do what
you ask, than to silently degrade. E.g. if you use sub-pixel rendering,
you really do not want 4:2:0.

That's compatible with your suggestion on changing the Colorspace
property, I think it would complement it. Cable encoding parameters
could be other properties, which might also be on "auto".

If Colorspace property cannot be changed, then options I haven't seen
discussed yet are have it force the cable encoding parameters, or
another new property replacing it.

> If no upstream userspace currently makes use of this property I
> can make that change, i.e., no longer tie the colorspace property
> directly to the infoframe and reduce the options to sRGB, BT709,
> BT601, and BT2020 (and possibly opRGB).
> 
> v2:
> - Tested with DP and HDMI analyzers
> - Confirmed driver will fallback to lower bpc when needed
> - Dropped hunk to set HDMI AVI infoframe as it was a no-op
> - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton)
> - Simplify initialization of supported colorspaces (Jani)
> - Fix kerneldoc (kernel test robot)

I recall saying this before, but in the series there are occurrences
where sRGB is spelled as "RGB". I find that very confusing that "RGB"
would imply anything about colorimetry when it's just a color model.

Sometimes it might not be sRGB because the "default RGB" has probably
not been sRGB for many years now, depending on monitor settings. See
also https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/17 . Then
one might ask the philosophical question whether the signal on the
cable is sRGB but it's just the monitor that does a gamut mapping
instead.


Thanks,
pq

> 
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Michel Dänzer <michel.daenzer@mailbox.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> 
> Harry Wentland (16):
>   drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF
>   drm/connector: print max_requested_bpc in state debugfs
>   drm/connector: Drop COLORIMETRY_NO_DATA
>   drm/connector: Convert DRM_MODE_COLORIMETRY to enum
>   drm/connector: Pull out common create_colorspace_property code
>   drm/connector: Allow drivers to pass list of supported colorspaces
>   drm/connector: Print connector colorspace in state debugfs
>   drm/amd/display: Always pass connector_state to stream validation
>   drm/amd/display: Register Colorspace property for DP and HDMI
>   drm/amd/display: Signal mode_changed if colorspace changed
>   drm/amd/display: Send correct DP colorspace infopacket
>   drm/amd/display: Add support for explicit BT601_YCC
>   drm/amd/display: Add debugfs for testing output colorspace
>   drm/amd/display: Add default case for output_color_space switch
>   drm/amd/display: Don't restrict bpc to 8 bpc
>   drm/amd/display: Format input and output CSC matrix
> 
> Joshua Ashton (5):
>   drm/amd/display: Always set crtcinfo from create_stream_for_sink
>   drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not
>     RGB
>   drm/amd/display: Refactor avi_info_frame colorimetry determination
>   drm/amd/display: Calculate output_color_space after pixel encoding
>     adjustment
>   drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  90 ++++++---
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
>  .../drm/amd/display/dc/core/dc_hw_sequencer.c |  38 ++--
>  .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 ++-
>  drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   |  54 +++--
>  drivers/gpu/drm/display/drm_hdmi_helper.c     |   8 +-
>  drivers/gpu/drm/drm_atomic.c                  |   2 +
>  drivers/gpu/drm/drm_connector.c               | 189 ++++++++++--------
>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>  include/drm/display/drm_dp.h                  |   2 +-
>  include/drm/drm_connector.h                   |  57 +++---
>  12 files changed, 345 insertions(+), 186 deletions(-)
> 
> --
> 2.39.0
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 00/21] Enable Colorspace connector property in amdgpu

[Hans Verkuil]

On 07/02/2023 13:47, Pekka Paalanen wrote:
> On Fri, 13 Jan 2023 11:24:07 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> This patchset enables the DP and HDMI infoframe properties
>> in amdgpu.
>>
>> The first two patches are not completely related to the rest. The
>> first patch allows for HDR_OUTPUT_METADATA with EOTFs that are
>> unknown in the kernel.
>>
>> The second one prints a connector's max_bpc as part of the atomic
>> state debugfs print.
>>
>> The following patches rework the connector colorspace code to
>> 1) allow for easy printing of the colorspace in the drm_atomic
>>    state debugfs, and
>> 2) allow drivers to specify the supported colorspaces on a
>>    connector.
>>
>> The rest of the patches deal with the Colorspace enablement
>> in amdgpu.
>>
>> Why do drivers need to specify supported colorspaces? The amdgpu
>> driver needs support for RGB-to-YCbCr conversion when we drive
>> the display in YCbCr. This is currently not implemented for all
>> colorspaces.
>>
>> Since the Colorspace property didn't have an IGT test I added
>> one to kms_hdr. The relevant patchset can be found on the IGT
>> mailing list or on
>> https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry
>>
>> We tested v1 of the patchset and confirmed that the infoframes
>> are as expected for both DP and HDMI when running the IGT
>> colorimetry tests.
>>
>> Open Items
>> ----------
>>
>> A couple comments from Pekka about colorspace documentation are
>> left unaddressed. I hope they won't block merging this set but
>> should still be addressed separately.
>>
>> Pekka's questions really got me thinking of how this colorspace
>> property should be used and working with it more closely with
>> Joshua who is enabling HDR in gamescope made me wonder even more.
>>
>> Uma, is there a (canonical, upstream) userspace that uses this
>> property that I can look at to understand more?
>>
>> One of the key challenges that is currently not addressed is that
>> userspace is expected to pick a colorspace format straight from the
>> list of definitions out of the DP or HDMI spec. But the kernel
>> driver are the ones deciding on the output encoding (RGB, YCBCR444,
>> YCBCR420, etc.). So there is no way for userspace to decide correctly
>> between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC.
>>
>> So we end up in a scenario where gamescope sets BT2020_RGB but we
>> output YCBCR444 so have to correct the colorspace value to
>> BT2020_YCC. This in turn breaks the colorspace IGT tests I
>> wrote. I don't think "fixing" the IGT tests to accept this is
>> the right thing to do.
>>
>> The way it stands this patchset allows us to specify the output
>> colorspace on amdgpu and we try to do the right thing, but I don't
>> thing the way the colorspace property is defined is right. We're trying
>> to expose things to userspace that should be under driver control. A
>> much better approach would be to give userspace options for colorspace
>> that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc.,
>> and have the driver do the right thing to fill the infoframe, e.g., by
>> picking BT2020_YCC if the requested colorspace is BT2020 and the
>> is YCBCR444.
> 
> Hi Harry,
> 
> well explained.
> 
> Indeed, if you want to keep the driver in control of the encoding on
> the monitor cable, then your suggestion seems correct (ignoring the
> question whether it breaks something existing).
> 
> I do recall something about letting userspace control the encoding on
> the cable though, particularly when it affects performance or quality.
> E.g. 4:2:0 sub-sampling might be wanted in some cases and unwanted in
> others. It's a bit similar to bpc. The trade-off may be frame rate or
> resolution. It might better to know that the hardware cannot do what
> you ask, than to silently degrade. E.g. if you use sub-pixel rendering,
> you really do not want 4:2:0.
> 
> That's compatible with your suggestion on changing the Colorspace
> property, I think it would complement it. Cable encoding parameters
> could be other properties, which might also be on "auto".
> 
> If Colorspace property cannot be changed, then options I haven't seen
> discussed yet are have it force the cable encoding parameters, or
> another new property replacing it.
> 
>> If no upstream userspace currently makes use of this property I
>> can make that change, i.e., no longer tie the colorspace property
>> directly to the infoframe and reduce the options to sRGB, BT709,
>> BT601, and BT2020 (and possibly opRGB).
>>
>> v2:
>> - Tested with DP and HDMI analyzers
>> - Confirmed driver will fallback to lower bpc when needed
>> - Dropped hunk to set HDMI AVI infoframe as it was a no-op
>> - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton)
>> - Simplify initialization of supported colorspaces (Jani)
>> - Fix kerneldoc (kernel test robot)
> 
> I recall saying this before, but in the series there are occurrences
> where sRGB is spelled as "RGB". I find that very confusing that "RGB"
> would imply anything about colorimetry when it's just a color model.
> 
> Sometimes it might not be sRGB because the "default RGB" has probably
> not been sRGB for many years now, depending on monitor settings. See
> also https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/17 . Then
> one might ask the philosophical question whether the signal on the
> cable is sRGB but it's just the monitor that does a gamut mapping
> instead.

FYI: the CTA-861.6 addendum adds support for explicit sRGB or defaultRGB
signaling. See chapter 8 in that standard.

The colorimetry that video sources us when transmitting RGB over HDMI is
usually sRGB, except for MacBooks, they use the colorimetry defined in the
EDID Base Block. And there was no way to signal exactly what a source was
using, or what a display expects.

Strictly speaking the approach MacBooks took was correct, but 1) not all
hardware can convert sRGB to a custom colorimetry setting, and 2) this
'requirement' was hidden in a footnote of a very long table in the spec.

In addition, the colorimetry information in the EDID Base Block was usually
identical or close to that of sRGB, but with extended gamut displays this
tends to diverge more and more in modern displays.

Regards,

	Hans

> 
> 
> Thanks,
> pq
> 
>>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Vitaly.Prosyak@amd.com
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Michel Dänzer <michel.daenzer@mailbox.org>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: amd-gfx@lists.freedesktop.org
>>
>> Harry Wentland (16):
>>   drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF
>>   drm/connector: print max_requested_bpc in state debugfs
>>   drm/connector: Drop COLORIMETRY_NO_DATA
>>   drm/connector: Convert DRM_MODE_COLORIMETRY to enum
>>   drm/connector: Pull out common create_colorspace_property code
>>   drm/connector: Allow drivers to pass list of supported colorspaces
>>   drm/connector: Print connector colorspace in state debugfs
>>   drm/amd/display: Always pass connector_state to stream validation
>>   drm/amd/display: Register Colorspace property for DP and HDMI
>>   drm/amd/display: Signal mode_changed if colorspace changed
>>   drm/amd/display: Send correct DP colorspace infopacket
>>   drm/amd/display: Add support for explicit BT601_YCC
>>   drm/amd/display: Add debugfs for testing output colorspace
>>   drm/amd/display: Add default case for output_color_space switch
>>   drm/amd/display: Don't restrict bpc to 8 bpc
>>   drm/amd/display: Format input and output CSC matrix
>>
>> Joshua Ashton (5):
>>   drm/amd/display: Always set crtcinfo from create_stream_for_sink
>>   drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not
>>     RGB
>>   drm/amd/display: Refactor avi_info_frame colorimetry determination
>>   drm/amd/display: Calculate output_color_space after pixel encoding
>>     adjustment
>>   drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix
>>
>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  90 ++++++---
>>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
>>  .../drm/amd/display/dc/core/dc_hw_sequencer.c |  38 ++--
>>  .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 ++-
>>  drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   |  54 +++--
>>  drivers/gpu/drm/display/drm_hdmi_helper.c     |   8 +-
>>  drivers/gpu/drm/drm_atomic.c                  |   2 +
>>  drivers/gpu/drm/drm_connector.c               | 189 ++++++++++--------
>>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>>  include/drm/display/drm_dp.h                  |   2 +-
>>  include/drm/drm_connector.h                   |  57 +++---
>>  12 files changed, 345 insertions(+), 186 deletions(-)
>>
>> --
>> 2.39.0
>>
> 

Re: [PATCH v2 00/21] Enable Colorspace connector property in amdgpu

[Sebastian Wick]

FWIW, I still think this series is good (minus the UAPI changes) and
would allow us to work on user space HDR support without custom
kernels.

On Fri, Jan 13, 2023 at 5:24 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
> This patchset enables the DP and HDMI infoframe properties
> in amdgpu.
>
> The first two patches are not completely related to the rest. The
> first patch allows for HDR_OUTPUT_METADATA with EOTFs that are
> unknown in the kernel.
>
> The second one prints a connector's max_bpc as part of the atomic
> state debugfs print.
>
> The following patches rework the connector colorspace code to
> 1) allow for easy printing of the colorspace in the drm_atomic
>    state debugfs, and
> 2) allow drivers to specify the supported colorspaces on a
>    connector.
>
> The rest of the patches deal with the Colorspace enablement
> in amdgpu.
>
> Why do drivers need to specify supported colorspaces? The amdgpu
> driver needs support for RGB-to-YCbCr conversion when we drive
> the display in YCbCr. This is currently not implemented for all
> colorspaces.
>
> Since the Colorspace property didn't have an IGT test I added
> one to kms_hdr. The relevant patchset can be found on the IGT
> mailing list or on
> https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry
>
> We tested v1 of the patchset and confirmed that the infoframes
> are as expected for both DP and HDMI when running the IGT
> colorimetry tests.
>
> Open Items
> ----------
>
> A couple comments from Pekka about colorspace documentation are
> left unaddressed. I hope they won't block merging this set but
> should still be addressed separately.
>
> Pekka's questions really got me thinking of how this colorspace
> property should be used and working with it more closely with
> Joshua who is enabling HDR in gamescope made me wonder even more.
>
> Uma, is there a (canonical, upstream) userspace that uses this
> property that I can look at to understand more?
>
> One of the key challenges that is currently not addressed is that
> userspace is expected to pick a colorspace format straight from the
> list of definitions out of the DP or HDMI spec. But the kernel
> driver are the ones deciding on the output encoding (RGB, YCBCR444,
> YCBCR420, etc.). So there is no way for userspace to decide correctly
> between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC.
>
> So we end up in a scenario where gamescope sets BT2020_RGB but we
> output YCBCR444 so have to correct the colorspace value to
> BT2020_YCC. This in turn breaks the colorspace IGT tests I
> wrote. I don't think "fixing" the IGT tests to accept this is
> the right thing to do.
>
> The way it stands this patchset allows us to specify the output
> colorspace on amdgpu and we try to do the right thing, but I don't
> thing the way the colorspace property is defined is right. We're trying
> to expose things to userspace that should be under driver control. A
> much better approach would be to give userspace options for colorspace
> that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc.,
> and have the driver do the right thing to fill the infoframe, e.g., by
> picking BT2020_YCC if the requested colorspace is BT2020 and the
> is YCBCR444.
>
> If no upstream userspace currently makes use of this property I
> can make that change, i.e., no longer tie the colorspace property
> directly to the infoframe and reduce the options to sRGB, BT709,
> BT601, and BT2020 (and possibly opRGB).
>
> v2:
> - Tested with DP and HDMI analyzers
> - Confirmed driver will fallback to lower bpc when needed
> - Dropped hunk to set HDMI AVI infoframe as it was a no-op
> - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton)
> - Simplify initialization of supported colorspaces (Jani)
> - Fix kerneldoc (kernel test robot)
>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Vitaly.Prosyak@amd.com
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Michel Dänzer <michel.daenzer@mailbox.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
>
> Harry Wentland (16):
>   drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF
>   drm/connector: print max_requested_bpc in state debugfs
>   drm/connector: Drop COLORIMETRY_NO_DATA
>   drm/connector: Convert DRM_MODE_COLORIMETRY to enum
>   drm/connector: Pull out common create_colorspace_property code
>   drm/connector: Allow drivers to pass list of supported colorspaces
>   drm/connector: Print connector colorspace in state debugfs
>   drm/amd/display: Always pass connector_state to stream validation
>   drm/amd/display: Register Colorspace property for DP and HDMI
>   drm/amd/display: Signal mode_changed if colorspace changed
>   drm/amd/display: Send correct DP colorspace infopacket
>   drm/amd/display: Add support for explicit BT601_YCC
>   drm/amd/display: Add debugfs for testing output colorspace
>   drm/amd/display: Add default case for output_color_space switch
>   drm/amd/display: Don't restrict bpc to 8 bpc
>   drm/amd/display: Format input and output CSC matrix
>
> Joshua Ashton (5):
>   drm/amd/display: Always set crtcinfo from create_stream_for_sink
>   drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not
>     RGB
>   drm/amd/display: Refactor avi_info_frame colorimetry determination
>   drm/amd/display: Calculate output_color_space after pixel encoding
>     adjustment
>   drm/amd/display: Fix COLOR_SPACE_YCBCR2020_TYPE matrix
>
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  90 ++++++---
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c |  57 ++++++
>  .../drm/amd/display/dc/core/dc_hw_sequencer.c |  38 ++--
>  .../gpu/drm/amd/display/dc/core/dc_resource.c |  28 ++-
>  drivers/gpu/drm/amd/display/dc/inc/hw/dpp.h   |  54 +++--
>  drivers/gpu/drm/display/drm_hdmi_helper.c     |   8 +-
>  drivers/gpu/drm/drm_atomic.c                  |   2 +
>  drivers/gpu/drm/drm_connector.c               | 189 ++++++++++--------
>  .../gpu/drm/i915/display/intel_connector.c    |   4 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c                |   2 +-
>  include/drm/display/drm_dp.h                  |   2 +-
>  include/drm/drm_connector.h                   |  57 +++---
>  12 files changed, 345 insertions(+), 186 deletions(-)
>
> --
> 2.39.0
>