[PATCH v3 0/8] Add new general DRM property "color format"

[PATCH v3 0/8] Add new general DRM property "color format"

[Marius Vlad]

Hi,

This is the v3 patch series, a follow-up from Andri's v2
patch series posted at
https://lore.kernel.org/dri-devel/20240115160554.720247-1-andri@yngvason.is/ where
a new DRM connector property has been added allowing users to
force a particular color format. 

Andri's changes on their own are actually a follow-up from
Werner Sembach's posted at 
https://lore.kernel.org/dri-devel/20210630151018.330354-1-wse@tuxedocomputers.com/

I've basically picked-up where Andri left and since v2 the following
changes have been made:

- renamed the property to just 'color format'
- the property is added dynamically similar to the Colorspace property
- a key point from previous comments was that drivers should advertise
  the color formats they support and userspace would query EDID and
  perform an intersection from those color formats which users can
  further use. With this patch set each driver that adds this property
  has such list of hard-coded color formats, but fundamentally the idea
  is that driver can query the HW and do that on its own. The
  infrastructure is now in place to allow to do that
- by default the 'AUTO' color format is set. With this patch series that
  has been introduced as a fallback to RGB. Drivers could further
  customize this behavour and could perform additional checks on the sink
  to pick another suitable color format they'd like for AUTO
- drm_bridge bridge code has been improved to allow initialization with 
  the same color formats list as the DRM connector property. Similarly, bpc
  pick-up now takes the color format into consideration when deciding
  which bpc to choose from
- The new DRM color format re-uses HDMI_COLORPSACE enum and provides an 
  enum translations between the two to avoid touching all other drivers that 
  use HDMI_COLORPSACE enum. I believe at this point that this allows the
  least amount of disruption and avoids a massive bike shedding around
  that part
- a rockchip implementation has been by my colleague Derek Foreman
- YUV444 color format has been added in i915
- address comment about "Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A
  check" where aconnector might be invalid

With that in mind we have an implementation in Weston at 
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1825 that
adds support for this property. This patch series has been tested against that MR on 
i915 and on rockchip.

But you can also manually test this with modetest like so:

$ modetest -s 115:1920x1080-60@NV12 -w 115:'color format':4

where 115 is the connector ID and '4' is the enum value for a particular
color format.

Feedback and testing is welcome and highly appreciated. Unfortunately
don't have an AMD card and I've picked the changes from Andri as is.

I've kept Andri as the original author and I hope that is fine with
everyone.

Andri Yngvason (3):
  drm: Add new general DRM property "color format"
  drm/i915: Implement the "color format" DRM property
  drm/amdgpu: Implement "color format" DRM property

Derek Foreman (1):
  drm/rockchip: Implement "color format" DRM property

Marius Vlad (3):
  hdmi: Add HDMI_COLORSPACE_AUTO enum option
  drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT
  drm: Pass supported color formats straight onto drm_bridge

Werner Sembach (1):
  drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  78 ++++++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  13 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  |   2 +-
 drivers/gpu/drm/bridge/ite-it6263.c           |   2 +-
 drivers/gpu/drm/bridge/ti-sn65dsi86.c         |   3 +-
 .../gpu/drm/display/drm_bridge_connector.c    |   4 +-
 .../gpu/drm/display/drm_hdmi_state_helper.c   |   7 +-
 drivers/gpu/drm/drm_atomic_helper.c           |   5 +
 drivers/gpu/drm/drm_atomic_uapi.c             |   4 +
 drivers/gpu/drm/drm_connector.c               | 196 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_connector.c    |  21 ++
 .../gpu/drm/i915/display/intel_connector.h    |   2 +
 .../drm/i915/display/intel_display_types.h    |   1 +
 drivers/gpu/drm/i915/display/intel_dp.c       |  60 +++++-
 drivers/gpu/drm/i915/display/intel_dp.h       |   5 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  58 +++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  58 +++++-
 drivers/gpu/drm/imx/dcss/dcss-kms.c           |   2 +-
 drivers/gpu/drm/mediatek/mtk_dpi.c            |   2 +-
 drivers/gpu/drm/mediatek/mtk_dsi.c            |   2 +-
 drivers/gpu/drm/meson/meson_encoder_cvbs.c    |   3 +-
 drivers/gpu/drm/meson/meson_encoder_hdmi.c    |   4 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c      |   2 +-
 drivers/gpu/drm/msm/dp/dp_drm.c               |   3 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c         |   2 +-
 drivers/gpu/drm/msm/hdmi/hdmi.c               |   2 +-
 .../gpu/drm/renesas/rcar-du/rcar_du_encoder.c |   2 +-
 .../gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  |   2 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c        |   2 +-
 .../gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    |  10 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c          |   8 +
 drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   8 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  |  46 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h  |   2 +
 drivers/gpu/drm/rockchip/rockchip_lvds.c      |   2 +-
 drivers/gpu/drm/tegra/hdmi.c                  |   2 +-
 drivers/gpu/drm/tegra/rgb.c                   |   2 +-
 drivers/gpu/drm/tidss/tidss_encoder.c         |   2 +-
 drivers/video/hdmi.c                          |   4 +-
 include/drm/drm_bridge_connector.h            |   3 +-
 include/drm/drm_connector.h                   |  57 ++++-
 include/linux/hdmi.h                          |   2 +-
 42 files changed, 639 insertions(+), 56 deletions(-)

-- 
2.47.2

[PATCH 1/8] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

[Marius Vlad]

From: Werner Sembach <wse@tuxedocomputers.com>

Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check that was performed in the
drm_mode_is_420_only() case, but not in the drm_mode_is_420_also() &&
force_yuv420_output case.

Without further knowledge if YCbCr 4:2:0 is supported outside of HDMI,
there is no reason to use RGB when the display
reports drm_mode_is_420_only() even on a non HDMI connection.

This patch also moves both checks in the same if-case. This  eliminates an
extra else-if-case.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Tested-by: Andri Yngvason <andri@yngvason.is>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 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 1cdb195deb63..cc19c4bfd623 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6427,12 +6427,8 @@ static void fill_stream_properties_from_drm_display_mode(
 	timing_out->v_border_top = 0;
 	timing_out->v_border_bottom = 0;
 	/* TODO: un-hardcode */
-	if (drm_mode_is_420_only(info, mode_in)
-			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-	else if (drm_mode_is_420_also(info, mode_in)
-			&& aconnector
-			&& aconnector->force_yuv420_output)
+	if (drm_mode_is_420_only(info, mode_in) || (drm_mode_is_420_also(info, mode_in) &&
+	    aconnector && aconnector->force_yuv420_output))
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
 	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
 			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
-- 
2.47.2

[PATCH 2/8] hdmi: Add HDMI_COLORSPACE_AUTO enum option

[Marius Vlad]

Patch does not introduce any functional change but would help out when
introducing DRM_COLOR_FORMAT enum in a sub-sequent patch.

Auto will implictly fallthrough to RGB as that should be further
driver-defined.

Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 3 +++
 drivers/gpu/drm/drm_connector.c                 | 1 +
 drivers/video/hdmi.c                            | 4 ++--
 include/linux/hdmi.h                            | 2 +-
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index a561f124be99..f9aa922d25d3 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -417,6 +417,9 @@ sink_supports_format_bpc(const struct drm_connector *connector,
 	}
 
 	switch (format) {
+	case HDMI_COLORSPACE_AUTO:
+		drm_dbg_kms(dev, "AUTO format. Fallback to RGB.\n");
+		fallthrough;
 	case HDMI_COLORSPACE_RGB:
 		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea47..24edba01f2f0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1424,6 +1424,7 @@ static const char * const output_format_str[] = {
 	[HDMI_COLORSPACE_YUV420]	= "YUV 4:2:0",
 	[HDMI_COLORSPACE_YUV422]	= "YUV 4:2:2",
 	[HDMI_COLORSPACE_YUV444]	= "YUV 4:4:4",
+	[HDMI_COLORSPACE_AUTO]		= "AUTO",
 };
 
 /*
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 45b42f14a750..74fe925c69a2 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -1031,8 +1031,8 @@ static const char *hdmi_colorspace_get_name(enum hdmi_colorspace colorspace)
 		return "YCbCr 4:4:4";
 	case HDMI_COLORSPACE_YUV420:
 		return "YCbCr 4:2:0";
-	case HDMI_COLORSPACE_RESERVED4:
-		return "Reserved (4)";
+	case HDMI_COLORSPACE_AUTO:
+		return "Auto";
 	case HDMI_COLORSPACE_RESERVED5:
 		return "Reserved (5)";
 	case HDMI_COLORSPACE_RESERVED6:
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 96bda41d9148..045402033763 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -82,7 +82,7 @@ enum hdmi_colorspace {
 	HDMI_COLORSPACE_YUV422,
 	HDMI_COLORSPACE_YUV444,
 	HDMI_COLORSPACE_YUV420,
-	HDMI_COLORSPACE_RESERVED4,
+	HDMI_COLORSPACE_AUTO,
 	HDMI_COLORSPACE_RESERVED5,
 	HDMI_COLORSPACE_RESERVED6,
 	HDMI_COLORSPACE_IDO_DEFINED,
-- 
2.47.2

[PATCH 3/8] drm: Add new general DRM property "color format"

[Marius Vlad]

From: Andri Yngvason <andri@yngvason.is>

Adds a new general DRM property named "color format" which can be used by
userspace to force the display driver output a particular color format.

Possible options are:
    - auto (setup by default, driver internally picks the color format)
    - rgb
    - ycbcr444
    - ycbcr422
    - ycbcr420

Drivers should advertise from this list the formats which they support.
Together with this list and EDID data from the sink we should be able
to relay a list of usable color formats to users to pick from.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |   5 +
 drivers/gpu/drm/drm_atomic_uapi.c   |   4 +
 drivers/gpu/drm/drm_connector.c     | 177 ++++++++++++++++++++++++++++
 include/drm/drm_connector.h         |  54 ++++++++-
 4 files changed, 235 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5ebe6ea0acb..03aae1adf540 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -736,6 +736,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->max_requested_bpc !=
 			    new_connector_state->max_requested_bpc)
 				new_crtc_state->connectors_changed = true;
+
+			if (old_connector_state->color_format !=
+			    new_connector_state->color_format)
+				new_crtc_state->connectors_changed = true;
+
 		}
 
 		if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 85dbdaa4a2e2..23092868407e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -780,6 +780,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->privacy_screen_sw_state = val;
 	} else if (property == connector->broadcast_rgb_property) {
 		state->hdmi.broadcast_rgb = val;
+	} else if (property == connector->color_format_property) {
+		state->color_format = drm_color_format_enum_to_color_format(val);
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -865,6 +867,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->privacy_screen_sw_state;
 	} else if (property == connector->broadcast_rgb_property) {
 		*val = state->hdmi.broadcast_rgb;
+	} else if (property == connector->color_format_property) {
+		*val = drm_color_format_to_color_format_enum(state->color_format);
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 24edba01f2f0..96e95047248e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1348,6 +1348,55 @@ static const char * const colorspace_names[] = {
 	[DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
 };
 
+u32
+drm_color_format_to_color_format_enum(enum drm_color_format fmt)
+{
+	switch (fmt) {
+	default:
+	case DRM_COLOR_FORMAT_AUTO:
+		return DRM_MODE_COLOR_FORMAT_AUTO;
+	case DRM_COLOR_FORMAT_RGB444:
+		return DRM_MODE_COLOR_FORMAT_RGB444;
+	case DRM_COLOR_FORMAT_YCBCR444:
+		return DRM_MODE_COLOR_FORMAT_YCBCR444;
+	case DRM_COLOR_FORMAT_YCBCR422:
+		return DRM_MODE_COLOR_FORMAT_YCBCR422;
+	case DRM_COLOR_FORMAT_YCBCR420:
+		return DRM_MODE_COLOR_FORMAT_YCBCR420;
+	}
+}
+
+u32
+drm_color_format_enum_to_color_format(enum drm_color_format_enum fmt_enum)
+{
+	switch (fmt_enum) {
+	default:
+	case DRM_MODE_COLOR_FORMAT_RGB444:
+		return DRM_COLOR_FORMAT_RGB444;
+	case DRM_MODE_COLOR_FORMAT_AUTO:
+		return DRM_COLOR_FORMAT_AUTO;
+	case DRM_MODE_COLOR_FORMAT_YCBCR444:
+		return DRM_COLOR_FORMAT_YCBCR444;
+	case DRM_MODE_COLOR_FORMAT_YCBCR422:
+		return DRM_COLOR_FORMAT_YCBCR422;
+	case DRM_MODE_COLOR_FORMAT_YCBCR420:
+		return DRM_COLOR_FORMAT_YCBCR420;
+	}
+}
+
+/**
+ * drm_get_color_format_name - return a string for color format
+ * @colorspace: color format to compute name of
+ *
+ */
+const char *drm_get_color_format_name(enum drm_color_format color_fmt)
+{
+	u32 conv_color_fmt = drm_color_format_to_color_format_enum(color_fmt);
+
+	return drm_hdmi_connector_get_output_format_name(conv_color_fmt);
+}
+EXPORT_SYMBOL(drm_get_color_format_name);
+
 /**
  * drm_get_colorspace_name - return a string for color encoding
  * @colorspace: color space to compute name of
@@ -1377,6 +1426,22 @@ static const u32 hdmi_colorspaces =
 	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
 	BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
 
+/* already bit-shifted */
+static const u32 hdmi_colorformats =
+	DRM_COLOR_FORMAT_AUTO |
+	DRM_COLOR_FORMAT_RGB444 |
+	DRM_COLOR_FORMAT_YCBCR444 |
+	DRM_COLOR_FORMAT_YCBCR422 |
+	DRM_COLOR_FORMAT_YCBCR420;
+
+/* already bit-shifted */
+static const u32 dp_colorformats =
+	DRM_COLOR_FORMAT_AUTO |
+	DRM_COLOR_FORMAT_RGB444 |
+	DRM_COLOR_FORMAT_YCBCR444 |
+	DRM_COLOR_FORMAT_YCBCR422 |
+	DRM_COLOR_FORMAT_YCBCR420;
+
 /*
  * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry
  * Format Table 2-120
@@ -2629,6 +2694,98 @@ int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
 
+static int drm_mode_create_color_format_property(struct drm_connector *connector,
+						 u32 supported_color_formats)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_prop_enum_list enum_list[DRM_MODE_COLOR_FORMAT_COUNT];
+	int i, len;
+
+	if (connector->color_format_property)
+		return 0;
+
+	if (!supported_color_formats) {
+		drm_err(dev, "No supported color formats provded on [CONNECTOR:%d:%s]\n",
+			    connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	if ((supported_color_formats & -BIT(DRM_MODE_COLOR_FORMAT_COUNT)) != 0) {
+		drm_err(dev, "Unknown colorspace provded on [CONNECTOR:%d:%s]\n",
+			    connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	len = 0;
+	for (i = 0; i < DRM_MODE_COLOR_FORMAT_COUNT; i++) {
+		if (!(supported_color_formats & BIT(i)))
+			continue;
+
+		enum_list[len].type = i;
+		enum_list[len].name = output_format_str[i];
+		len++;
+	}
+
+	connector->color_format_property =
+		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "color format",
+					enum_list, len);
+
+	if (!connector->color_format_property)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * drm_mode_create_hdmi_color_format_property - create hdmi color format property
+ * @connector: connector to create the color format property on.
+ * @supported_color_formats: bitmap of supported color formats
+ *
+ * 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_color_format_property(struct drm_connector *connector,
+					       u32 supported_color_formats)
+{
+	u32 color_formats;
+
+	if (supported_color_formats)
+		color_formats = supported_color_formats & hdmi_colorformats;
+	else
+		color_formats = hdmi_colorformats;
+
+	return drm_mode_create_color_format_property(connector, color_formats);
+}
+EXPORT_SYMBOL(drm_mode_create_hdmi_color_format_property);
+
+/**
+ * drm_mode_create_dp_color_format_property - create dp color format property
+ * @connector: connector to create the Colorspace property on.
+ * @supported_color_formats: bitmap of supported color formats
+ *
+ * Called by a driver the first time it's needed, must be attached to desired
+ * DP connectors.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_dp_color_format_property(struct drm_connector *connector,
+					     u32 supported_color_formats)
+{
+	u32 color_formats;
+
+	if (supported_color_formats)
+		color_formats = supported_color_formats & dp_colorformats;
+	else
+		color_formats = dp_colorformats;
+
+	return drm_mode_create_color_format_property(connector, color_formats);
+}
+EXPORT_SYMBOL(drm_mode_create_dp_color_format_property);
+
 /**
  * drm_mode_create_dp_colorspace_property - create dp colorspace property
  * @connector: connector to create the Colorspace property on.
@@ -2846,6 +3003,26 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_color_format_property - attach "force color format" property
+ * @connector: connector to attach force color format property on.
+ *
+ * This is used to add support for selecting a color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_color_format_property(struct drm_connector *connector)
+{
+	struct drm_property *prop = connector->color_format_property;
+
+	drm_object_attach_property(&connector->base, prop, DRM_COLOR_FORMAT_AUTO);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_color_format_property);
+
+
 /**
  * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
  * @connector: connector to attach the property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d..24d760e2fa8f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -556,6 +556,26 @@ enum drm_colorspace {
 	DRM_MODE_COLORIMETRY_COUNT
 };
 
+enum drm_color_format_enum {
+	DRM_MODE_COLOR_FORMAT_RGB444		= HDMI_COLORSPACE_RGB,
+	DRM_MODE_COLOR_FORMAT_YCBCR422		= HDMI_COLORSPACE_YUV422,
+	DRM_MODE_COLOR_FORMAT_YCBCR444		= HDMI_COLORSPACE_YUV444,
+	DRM_MODE_COLOR_FORMAT_YCBCR420		= HDMI_COLORSPACE_YUV420,
+	/* auto case, driver will set the color_format */
+	DRM_MODE_COLOR_FORMAT_AUTO		= HDMI_COLORSPACE_AUTO,
+	DRM_MODE_COLOR_FORMAT_COUNT
+};
+
+enum drm_color_format {
+	DRM_COLOR_FORMAT_NONE			= 0,
+	DRM_COLOR_FORMAT_RGB444			= (1 << 0),
+	DRM_COLOR_FORMAT_YCBCR422		= (1 << 1),
+	DRM_COLOR_FORMAT_YCBCR444		= (1 << 2),
+	DRM_COLOR_FORMAT_YCBCR420		= (1 << 3),
+	/* auto case, driver will set the color_format */
+	DRM_COLOR_FORMAT_AUTO			= (1 << 4),
+};
+
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info
  *
@@ -699,11 +719,6 @@ struct drm_display_info {
 	 */
 	enum subpixel_order subpixel_order;
 
-#define DRM_COLOR_FORMAT_RGB444		(1<<0)
-#define DRM_COLOR_FORMAT_YCBCR444	(1<<1)
-#define DRM_COLOR_FORMAT_YCBCR422	(1<<2)
-#define DRM_COLOR_FORMAT_YCBCR420	(1<<3)
-
 	/**
 	 * @panel_orientation: Read only connector property for built-in panels,
 	 * indicating the orientation of the panel vs the device's casing.
@@ -1107,6 +1122,13 @@ struct drm_connector_state {
 	 */
 	enum drm_colorspace colorspace;
 
+	/**
+	 * @color_format: State variable for Connector property to request
+	 * color format change on Sink. This is most commonly used to switch
+	 * between RGB to YUV and vice-versa.
+	 */
+	enum drm_color_format color_format;
+
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
@@ -2060,6 +2082,12 @@ struct drm_connector {
 	 */
 	struct drm_property *colorspace_property;
 
+	/**
+	 * @color_format_property: Connector property to set the suitable
+	 * color format supported by the sink.
+	 */
+	struct drm_property *color_format_property;
+
 	/**
 	 * @path_blob_ptr:
 	 *
@@ -2461,6 +2489,12 @@ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
 int drm_mode_create_content_type_property(struct drm_device *dev);
 int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
 
+int drm_mode_create_hdmi_color_format_property(struct drm_connector *connector,
+					       u32 supported_color_formats);
+
+int drm_mode_create_dp_color_format_property(struct drm_connector *connector,
+					     u32 supported_color_formats);
+
 int drm_connector_set_path_property(struct drm_connector *connector,
 				    const char *path);
 int drm_connector_set_tile_property(struct drm_connector *connector);
@@ -2542,6 +2576,16 @@ bool drm_connector_has_possible_encoder(struct drm_connector *connector,
 					struct drm_encoder *encoder);
 const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
 
+int drm_connector_attach_color_format_property(struct drm_connector *connector);
+
+const char *drm_get_color_format_name(enum drm_color_format color_fmt);
+
+u32
+drm_color_format_to_color_format_enum(enum drm_color_format fmt);
+
+u32
+drm_color_format_enum_to_color_format(enum drm_color_format_enum fmt_enum);
+
 /**
  * drm_for_each_connector_iter - connector_list iterator macro
  * @connector: &struct drm_connector pointer used as cursor
-- 
2.47.2

[PATCH 4/8] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Marius Vlad]

This would please the compiler to have a enum transformation from one to
another even though the values are the same. It should also make things
obvious that we use different enums.

Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
---
 .../gpu/drm/display/drm_hdmi_state_helper.c    |  4 +++-
 drivers/gpu/drm/drm_connector.c                | 18 ++++++++++++++++++
 include/drm/drm_connector.h                    |  3 +++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index f9aa922d25d3..dc2f32651cb9 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -653,9 +653,11 @@ hdmi_compute_config(const struct drm_connector *connector,
 				       conn_state->max_bpc,
 				       8, connector->max_bpc);
 	int ret;
+	enum hdmi_colorspace hdmi_colorspace =
+		color_format_to_hdmi_colorspace(conn_state->color_format);
 
 	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
-				      HDMI_COLORSPACE_RGB);
+				      hdmi_colorspace);
 	if (ret) {
 		if (connector->ycbcr_420_allowed) {
 			ret = hdmi_compute_format_bpc(connector, conn_state,
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 96e95047248e..c77d770c18c7 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1384,6 +1384,24 @@ drm_color_format_enum_to_color_format(enum drm_color_format_enum fmt_enum)
 	}
 }
 
+enum hdmi_colorspace
+color_format_to_hdmi_colorspace(enum drm_color_format fmt)
+{
+	switch (fmt) {
+	default:
+	case DRM_COLOR_FORMAT_AUTO:
+		return HDMI_COLORSPACE_AUTO;
+	case DRM_COLOR_FORMAT_RGB444:
+		return HDMI_COLORSPACE_RGB;
+	case DRM_COLOR_FORMAT_YCBCR444:
+		return HDMI_COLORSPACE_YUV444;
+	case DRM_COLOR_FORMAT_YCBCR422:
+		return HDMI_COLORSPACE_YUV422;
+	case DRM_COLOR_FORMAT_YCBCR420:
+		return HDMI_COLORSPACE_YUV420;
+	}
+}
+
 /**
  * drm_get_color_format_name - return a string for color format
  * @colorspace: color format to compute name of
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 24d760e2fa8f..844d7a495bed 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2586,6 +2586,9 @@ drm_color_format_to_color_format_enum(enum drm_color_format fmt);
 u32
 drm_color_format_enum_to_color_format(enum drm_color_format_enum fmt_enum);
 
+enum hdmi_colorspace
+color_format_to_hdmi_colorspace(enum drm_color_format fmt);
+
 /**
  * drm_for_each_connector_iter - connector_list iterator macro
  * @connector: &struct drm_connector pointer used as cursor
-- 
2.47.2

[PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Marius Vlad]

Initialize drm_brige with advertised colors formats straight on.

Drivers that make use of DRM helpers would check the
drm_brige::supported_formats bit-field list and refuse to use the color
format passed. Drivers that make use of drm_bridge can pass the
supported color formats in the bridge as well as supported color format
for the DRM color format property.

This includes a fallback to RGB when Auto has been selected.

Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c      | 2 +-
 drivers/gpu/drm/bridge/ite-it6263.c               | 2 +-
 drivers/gpu/drm/bridge/ti-sn65dsi86.c             | 3 ++-
 drivers/gpu/drm/display/drm_bridge_connector.c    | 4 ++--
 drivers/gpu/drm/imx/dcss/dcss-kms.c               | 2 +-
 drivers/gpu/drm/mediatek/mtk_dpi.c                | 2 +-
 drivers/gpu/drm/mediatek/mtk_dsi.c                | 2 +-
 drivers/gpu/drm/meson/meson_encoder_cvbs.c        | 3 ++-
 drivers/gpu/drm/meson/meson_encoder_hdmi.c        | 4 ++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c          | 2 +-
 drivers/gpu/drm/msm/dp/dp_drm.c                   | 3 ++-
 drivers/gpu/drm/msm/dsi/dsi_manager.c             | 2 +-
 drivers/gpu/drm/msm/hdmi/hdmi.c                   | 2 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c | 2 +-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  | 2 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c            | 2 +-
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c          | 2 +-
 drivers/gpu/drm/tegra/hdmi.c                      | 2 +-
 drivers/gpu/drm/tegra/rgb.c                       | 2 +-
 drivers/gpu/drm/tidss/tidss_encoder.c             | 2 +-
 include/drm/drm_bridge_connector.h                | 3 ++-
 22 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 26f8ef482423..f2fea84b6415 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -762,7 +762,7 @@ static int adv7511_connector_init(struct adv7511 *adv)
 	struct drm_bridge *bridge = &adv->bridge;
 	struct drm_connector *connector;
 
-	connector = drm_bridge_connector_init(bridge->dev, bridge->encoder);
+	connector = drm_bridge_connector_init(bridge->dev, bridge->encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		DRM_ERROR("Failed to initialize connector with drm\n");
 		return PTR_ERR(connector);
diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
index cf813672b4ff..b45d2b4913e5 100644
--- a/drivers/gpu/drm/bridge/ite-it6263.c
+++ b/drivers/gpu/drm/bridge/ite-it6263.c
@@ -680,7 +680,7 @@ static int it6263_bridge_attach(struct drm_bridge *bridge,
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
 		return 0;
 
-	connector = drm_bridge_connector_init(bridge->dev, encoder);
+	connector = drm_bridge_connector_init(bridge->dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
 		dev_err(it->dev, "failed to initialize bridge connector: %d\n",
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 464390372b34..14fc58b28de8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -719,7 +719,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return 0;
 
 	pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
-						     pdata->bridge.encoder);
+						     pdata->bridge.encoder,
+						     BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(pdata->connector)) {
 		ret = PTR_ERR(pdata->connector);
 		goto err_initted_aux;
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 54f3f16d64c7..83951aaf1ade 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -634,13 +634,13 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
  * pointer otherwise.
  */
 struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
-						struct drm_encoder *encoder)
+						struct drm_encoder *encoder,
+						unsigned int supported_formats)
 {
 	struct drm_bridge_connector *bridge_connector;
 	struct drm_connector *connector;
 	struct i2c_adapter *ddc = NULL;
 	struct drm_bridge *bridge, *panel_bridge = NULL;
-	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
 	unsigned int max_bpc = 8;
 	bool support_hdcp = false;
 	int connector_type;
diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
index 3633e8f3aff6..2eccf62d6c45 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
@@ -97,7 +97,7 @@ static int dcss_kms_bridge_connector_init(struct dcss_kms_dev *kms)
 	if (ret < 0)
 		return ret;
 
-	kms->connector = drm_bridge_connector_init(ddev, encoder);
+	kms->connector = drm_bridge_connector_init(ddev, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(kms->connector)) {
 		dev_err(ddev->dev, "Unable to create bridge connector.\n");
 		return PTR_ERR(kms->connector);
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 61cab32e213a..15820e6ba057 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -1057,7 +1057,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		goto err_cleanup;
 
-	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder);
+	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(dpi->connector)) {
 		dev_err(dev, "Unable to create bridge connector\n");
 		ret = PTR_ERR(dpi->connector);
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index d7726091819c..91afdbf676f0 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -930,7 +930,7 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
 	if (ret)
 		goto err_cleanup_encoder;
 
-	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder);
+	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(dsi->connector)) {
 		DRM_ERROR("Unable to create bridge connector\n");
 		ret = PTR_ERR(dsi->connector);
diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
index dc374bfc5951..a475fc34ca23 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
@@ -275,7 +275,8 @@ int meson_encoder_cvbs_probe(struct meson_drm *priv)
 	}
 
 	/* Initialize & attach Bridge Connector */
-	connector = drm_bridge_connector_init(priv->drm, &meson_encoder_cvbs->encoder);
+	connector = drm_bridge_connector_init(priv->drm,
+			&meson_encoder_cvbs->encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector))
 		return dev_err_probe(priv->dev, PTR_ERR(connector),
 				     "Unable to create CVBS bridge connector\n");
diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 8205ee56a691..7d157de42d1c 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -425,8 +425,8 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
 	}
 
 	/* Initialize & attach Bridge Connector */
-	meson_encoder_hdmi->connector = drm_bridge_connector_init(priv->drm,
-							&meson_encoder_hdmi->encoder);
+	meson_encoder_hdmi->connector =
+		drm_bridge_connector_init(priv->drm, &meson_encoder_hdmi->encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(meson_encoder_hdmi->connector)) {
 		ret = dev_err_probe(priv->dev,
 				    PTR_ERR(meson_encoder_hdmi->connector),
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 0952c7f18abd..65c51d9f083e 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -226,7 +226,7 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms,
 			return ret;
 		}
 
-		connector = drm_bridge_connector_init(dev, encoder);
+		connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 		if (IS_ERR(connector)) {
 			DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n");
 			return PTR_ERR(connector);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 9a461ab2f32f..8d5299849be6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -368,7 +368,8 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 {
 	struct drm_connector *connector = NULL;
 
-	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
+	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder,
+					      BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector))
 		return connector;
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index ca400924d4ee..4b87f4f78d38 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
 	if (ret)
 		return ret;
 
-	connector = drm_bridge_connector_init(dev, encoder);
+	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		DRM_ERROR("Unable to create bridge connector\n");
 		return PTR_ERR(connector);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 5afac09c0d33..6da03b5143b0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -182,7 +182,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		}
 	}
 
-	hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder);
+	hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(hdmi->connector)) {
 		ret = PTR_ERR(hdmi->connector);
 		DRM_DEV_ERROR(dev->dev, "failed to create HDMI connector: %d\n", ret);
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
index 7ecec7b04a8d..8d903683f6f6 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
@@ -125,7 +125,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	}
 
 	/* Create the connector for the chain of bridges. */
-	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
+	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		dev_err(rcdu->dev,
 			"failed to created connector for output %s (%ld)\n",
diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c
index 5e6dd16705e6..4578cf5b756b 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c
@@ -114,7 +114,7 @@ int rzg2l_du_encoder_init(struct rzg2l_du_device  *rcdu,
 	}
 
 	/* Create the connector for the chain of bridges. */
-	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
+	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		dev_err(rcdu->dev,
 			"failed to created connector for output %s (%ld)\n",
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index b7e3f5dcf8d5..d591d660d059 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1016,7 +1016,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	connector = drm_bridge_connector_init(drm_dev, encoder);
+	connector = drm_bridge_connector_init(drm_dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
 		dev_err(dp->dev, "failed to init bridge connector: %d\n", ret);
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 7d531b6f4c09..58e24669ef34 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -563,7 +563,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	connector = drm_bridge_connector_init(drm, encoder);
+	connector = drm_bridge_connector_init(drm, encoder, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
 		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 2411260db51d..3727035d896e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -619,7 +619,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 		if (ret)
 			goto err_free_bridge;
 
-		connector = drm_bridge_connector_init(lvds->drm_dev, encoder);
+		connector = drm_bridge_connector_init(lvds->drm_dev, encoder, BIT(HDMI_COLORSPACE_RGB));
 		if (IS_ERR(connector)) {
 			drm_err(drm_dev,
 				"failed to initialize bridge connector: %pe\n",
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 8cd2969e7d4b..6419d152c8b3 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1568,7 +1568,7 @@ static int tegra_hdmi_init(struct host1x_client *client)
 			return err;
 		}
 
-		connector = drm_bridge_connector_init(drm, &hdmi->output.encoder);
+		connector = drm_bridge_connector_init(drm, &hdmi->output.encoder, BIT(HDMI_COLORSPACE_RGB));
 		if (IS_ERR(connector)) {
 			dev_err(client->dev,
 				"failed to initialize bridge connector: %pe\n",
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index ff5a749710db..f0b18491fa11 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -348,7 +348,7 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
 		if (err)
 			return err;
 
-		connector = drm_bridge_connector_init(drm, &output->encoder);
+		connector = drm_bridge_connector_init(drm, &output->encoder, BIT(HDMI_COLORSPACE_RGB));
 		if (IS_ERR(connector)) {
 			dev_err(output->dev,
 				"failed to initialize bridge connector: %pe\n",
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index 81a04f767770..8829d995811b 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -115,7 +115,7 @@ int tidss_encoder_create(struct tidss_device *tidss,
 	}
 
 	/* Initializing the connector at the end of bridge-chain */
-	connector = drm_bridge_connector_init(&tidss->ddev, enc);
+	connector = drm_bridge_connector_init(&tidss->ddev, enc, BIT(HDMI_COLORSPACE_RGB));
 	if (IS_ERR(connector)) {
 		dev_err(tidss->dev, "bridge_connector create failed\n");
 		return PTR_ERR(connector);
diff --git a/include/drm/drm_bridge_connector.h b/include/drm/drm_bridge_connector.h
index 69630815fb09..e1446d8af8b7 100644
--- a/include/drm/drm_bridge_connector.h
+++ b/include/drm/drm_bridge_connector.h
@@ -11,6 +11,7 @@ struct drm_device;
 struct drm_encoder;
 
 struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
-						struct drm_encoder *encoder);
+						struct drm_encoder *encoder,
+						unsigned int supported_colorformats);
 
 #endif /* __DRM_BRIDGE_CONNECTOR_H__ */
-- 
2.47.2

[PATCH 6/8] drm/i915: Implement the "color format" DRM property

[Marius Vlad]

From: Andri Yngvason <andri@yngvason.is>

This includes YUV420 as well YUV444 and Auto. Auto will fallback to RGB
to keep things sane and still working.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Co-Developed-by: Andri Yngvason <andri@yngvason.is>
Co-Developed-by: Marius Vlad <marius.vlad@collabora.com>
---
 .../gpu/drm/i915/display/intel_connector.c    | 21 +++++++
 .../gpu/drm/i915/display/intel_connector.h    |  2 +
 .../drm/i915/display/intel_display_types.h    |  1 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 60 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp.h       |  5 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 58 +++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 58 ++++++++++++++++--
 7 files changed, 192 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 42c923f416b3..4f76c91ed614 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -39,6 +39,13 @@
 #include "intel_hdcp.h"
 #include "intel_panel.h"
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444 |
+       DRM_COLOR_FORMAT_YCBCR420;
+
+
 static void intel_connector_modeset_retry_work_fn(struct work_struct *work)
 {
 	struct intel_connector *connector = container_of(work, typeof(*connector),
@@ -325,6 +332,13 @@ intel_attach_hdmi_colorspace_property(struct drm_connector *connector)
 		drm_connector_attach_colorspace_property(connector);
 }
 
+void
+intel_attach_hdmi_colorformat_property(struct drm_connector *connector)
+{
+	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
+		drm_connector_attach_color_format_property(connector);
+}
+
 void
 intel_attach_dp_colorspace_property(struct drm_connector *connector)
 {
@@ -332,6 +346,13 @@ intel_attach_dp_colorspace_property(struct drm_connector *connector)
 		drm_connector_attach_colorspace_property(connector);
 }
 
+void
+intel_attach_dp_colorformat_property(struct drm_connector *connector)
+{
+	if (!drm_mode_create_dp_color_format_property(connector, supported_colorformats))
+		drm_connector_attach_color_format_property(connector);
+}
+
 void
 intel_attach_scaling_mode_property(struct drm_connector *connector)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h
index aafb25a814fa..7191df21b48b 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.h
+++ b/drivers/gpu/drm/i915/display/intel_connector.h
@@ -31,7 +31,9 @@ void intel_attach_force_audio_property(struct drm_connector *connector);
 void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 void intel_attach_aspect_ratio_property(struct drm_connector *connector);
 void intel_attach_hdmi_colorspace_property(struct drm_connector *connector);
+void intel_attach_hdmi_colorformat_property(struct drm_connector *connector);
 void intel_attach_dp_colorspace_property(struct drm_connector *connector);
+void intel_attach_dp_colorformat_property(struct drm_connector *connector);
 void intel_attach_scaling_mode_property(struct drm_connector *connector);
 void intel_connector_queue_modeset_retry_work(struct intel_connector *connector);
 void intel_connector_cancel_modeset_retry_work(struct intel_connector *connector);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index ce45261c4a8f..4128547c3396 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -924,6 +924,7 @@ enum intel_output_format {
 	INTEL_OUTPUT_FORMAT_RGB,
 	INTEL_OUTPUT_FORMAT_YCBCR420,
 	INTEL_OUTPUT_FORMAT_YCBCR444,
+	INTEL_OUTPUT_FORMAT_AUTO,
 };
 
 /* Used by dp and fdi links */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7976fec88606..1df0ec28dde2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1090,6 +1090,7 @@ static bool source_can_output(struct intel_dp *intel_dp,
 	struct intel_display *display = to_intel_display(intel_dp);
 
 	switch (format) {
+	case INTEL_OUTPUT_FORMAT_AUTO:
 	case INTEL_OUTPUT_FORMAT_RGB:
 		return true;
 
@@ -1159,7 +1160,7 @@ dfp_can_convert(struct intel_dp *intel_dp,
 	return false;
 }
 
-static enum intel_output_format
+enum intel_output_format
 intel_dp_output_format(struct intel_connector *connector,
 		       enum intel_output_format sink_format)
 {
@@ -1194,6 +1195,7 @@ intel_dp_output_format(struct intel_connector *connector,
 	return output_format;
 }
 
+
 int intel_dp_min_bpp(enum intel_output_format output_format)
 {
 	if (output_format == INTEL_OUTPUT_FORMAT_RGB)
@@ -3030,6 +3032,22 @@ static bool intel_dp_has_audio(struct intel_encoder *encoder,
 		return intel_conn_state->force_audio == HDMI_AUDIO_ON;
 }
 
+static u32
+intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	case INTEL_OUTPUT_FORMAT_AUTO:
+		return DRM_COLOR_FORMAT_AUTO;
+	case INTEL_OUTPUT_FORMAT_RGB:
+		return DRM_COLOR_FORMAT_RGB444;
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return DRM_COLOR_FORMAT_YCBCR444;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return DRM_COLOR_FORMAT_YCBCR420;
+	}
+	return 0;
+}
+
 static int
 intel_dp_compute_output_format(struct intel_encoder *encoder,
 			       struct intel_crtc_state *crtc_state,
@@ -3041,17 +3059,32 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 	struct intel_connector *connector = intel_dp->attached_connector;
 	const struct drm_display_info *info = &connector->base.display_info;
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
-	bool ycbcr_420_only;
+	bool ycbcr_420_output = false;
+	bool ycbcr_444_output = false;
 	int ret;
+	u32 intel_convert_color_fmt = 0;
+
+	if ((conn_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+	    drm_mode_is_420_also(&connector->base.display_info, adjusted_mode)) ||
+	    drm_mode_is_420_only(info, adjusted_mode)) {
+		ycbcr_420_output = true;
+	} else if (conn_state->color_format == DRM_COLOR_FORMAT_YCBCR444) {
+		ycbcr_444_output = true;
+	}
+
+
+	if (ycbcr_420_output)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+	else if (ycbcr_444_output)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+	else
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
 
-	ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
 
-	if (ycbcr_420_only && !connector->base.ycbcr_420_allowed) {
+	if (ycbcr_420_output && !connector->base.ycbcr_420_allowed) {
 		drm_dbg_kms(display->drm,
 			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
 		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
-	} else {
-		crtc_state->sink_format = intel_dp_sink_format(connector, adjusted_mode);
 	}
 
 	crtc_state->output_format = intel_dp_output_format(connector, crtc_state->sink_format);
@@ -3071,6 +3104,18 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 						   respect_downstream_limits);
 	}
 
+	intel_convert_color_fmt =
+		intel_output_format_to_drm_color_format(crtc_state->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != intel_convert_color_fmt &&
+	    conn_state->color_format != DRM_COLOR_FORMAT_AUTO) {
+		drm_dbg_kms(display->drm,
+			    "Unsupported color format %s (0x%x), sink has 0x%x\n",
+			    drm_get_color_format_name(conn_state->color_format),\
+			    conn_state->color_format, intel_convert_color_fmt);
+		ret = -EINVAL;
+	}
+
 	return ret;
 }
 
@@ -6442,12 +6487,15 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *_connec
 	else if (DISPLAY_VER(display) >= 5)
 		drm_connector_attach_max_bpc_property(&connector->base, 6, 12);
 
+
 	/* Register HDMI colorspace for case of lspcon */
 	if (intel_bios_encoder_is_lspcon(dp_to_dig_port(intel_dp)->base.devdata)) {
 		drm_connector_attach_content_type_property(&connector->base);
 		intel_attach_hdmi_colorspace_property(&connector->base);
+		intel_attach_hdmi_colorformat_property(&connector->base);
 	} else {
 		intel_attach_dp_colorspace_property(&connector->base);
+		intel_attach_dp_colorformat_property(&connector->base);
 	}
 
 	if (intel_dp_has_gamut_metadata_dip(&dp_to_dig_port(intel_dp)->base))
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 0657f5681196..0d6bdcc70b41 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -199,6 +199,11 @@ bool intel_dp_compute_config_limits(struct intel_dp *intel_dp,
 				    bool dsc,
 				    struct link_config_limits *limits);
 
+enum intel_output_format
+intel_dp_output_format(struct intel_connector *connector,
+		enum intel_output_format sink_format);
+
+
 void intel_dp_get_dsc_sink_cap(u8 dpcd_rev, struct intel_connector *connector);
 bool intel_dp_has_gamut_metadata_dip(struct intel_encoder *encoder);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 74497c9a0554..34db7ee00d63 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -92,6 +92,23 @@
  * registers.
  */
 
+static u32
+intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	case INTEL_OUTPUT_FORMAT_AUTO:
+		return DRM_COLOR_FORMAT_AUTO;
+	case INTEL_OUTPUT_FORMAT_RGB:
+		return DRM_COLOR_FORMAT_RGB444;
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return DRM_COLOR_FORMAT_YCBCR444;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return DRM_COLOR_FORMAT_YCBCR420;
+	}
+
+	return 0;
+}
+
 /* From fake MST stream encoder to primary encoder */
 static struct intel_encoder *to_primary_encoder(struct intel_encoder *encoder)
 {
@@ -640,10 +657,14 @@ static int mst_stream_compute_config(struct intel_encoder *encoder,
 		to_intel_connector(conn_state->connector);
 	const struct drm_display_mode *adjusted_mode =
 		&pipe_config->hw.adjusted_mode;
+	const struct drm_display_info *info =
+		&connector->base.display_info;
 	struct link_config_limits limits;
 	bool dsc_needed, joiner_needs_dsc;
 	int num_joined_pipes;
 	int ret = 0;
+	bool ycbcr_420_output = false;
+	u32 intel_convert_color_fmt = 0;
 
 	if (pipe_config->fec_enable &&
 	    !intel_dp_supports_fec(intel_dp, connector, pipe_config))
@@ -658,10 +679,38 @@ static int mst_stream_compute_config(struct intel_encoder *encoder,
 	if (num_joined_pipes > 1)
 		pipe_config->joiner_pipes = GENMASK(crtc->pipe + num_joined_pipes - 1, crtc->pipe);
 
-	pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
-	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
+	if ((conn_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+	    drm_mode_is_420_also(&connector->base.display_info, adjusted_mode)) ||
+	    drm_mode_is_420_only(info, adjusted_mode)) {
+		ycbcr_420_output = true;
+	}
+
+	if (ycbcr_420_output && !connector->base.ycbcr_420_allowed) {
+		drm_dbg_kms(display->drm,
+			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
+	}
+
+	if (ycbcr_420_output)
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+	else
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
+
+	pipe_config->output_format = intel_dp_output_format(connector, pipe_config->sink_format);
 	pipe_config->has_pch_encoder = false;
 
+	intel_convert_color_fmt =
+		intel_output_format_to_drm_color_format(pipe_config->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != intel_convert_color_fmt &&
+	    conn_state->color_format != DRM_COLOR_FORMAT_AUTO) {
+		drm_dbg_kms(display->drm,
+			    "Unsupported color format %s (0x%x), sink has 0x%x\n",
+			    drm_get_color_format_name(conn_state->color_format),\
+			    conn_state->color_format, intel_convert_color_fmt);
+		ret = -EINVAL;
+	}
+
 	joiner_needs_dsc = intel_dp_joiner_needs_dsc(display, num_joined_pipes);
 
 	dsc_needed = joiner_needs_dsc || intel_dp->force_dsc_en ||
@@ -1647,6 +1696,11 @@ static int mst_topology_add_connector_properties(struct intel_dp *intel_dp,
 	if (connector->base.max_bpc_property)
 		drm_connector_attach_max_bpc_property(&connector->base, 6, 12);
 
+	connector->base.color_format_property =
+		intel_dp->attached_connector->base.color_format_property;
+	if (connector->base.color_format_property)
+		intel_attach_dp_colorformat_property(&connector->base);
+
 	return drm_connector_set_path_property(&connector->base, pathprop);
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 9961ff259298..016edb1ab248 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2243,6 +2243,23 @@ intel_hdmi_output_format(const struct intel_crtc_state *crtc_state)
 	return crtc_state->sink_format;
 }
 
+static u32
+intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	case INTEL_OUTPUT_FORMAT_AUTO:
+		return DRM_COLOR_FORMAT_AUTO;
+	case INTEL_OUTPUT_FORMAT_RGB:
+		return DRM_COLOR_FORMAT_RGB444;
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return DRM_COLOR_FORMAT_YCBCR444;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return DRM_COLOR_FORMAT_YCBCR420;
+	}
+
+	return 0;
+}
+
 static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 					    struct intel_crtc_state *crtc_state,
 					    const struct drm_connector_state *conn_state,
@@ -2252,13 +2269,29 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	const struct drm_display_info *info = &connector->base.display_info;
-	bool ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
+	bool ycbcr_420_output = false;
+	bool ycbcr_444_output = false;
+	u32 intel_convert_color_fmt = 0;
 	int ret;
 
-	crtc_state->sink_format =
-		intel_hdmi_sink_format(crtc_state, connector, ycbcr_420_only);
+	if ((conn_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+	     drm_mode_is_420_also(&connector->base.display_info, adjusted_mode)) ||
+	     drm_mode_is_420_only(info, adjusted_mode)) {
+		ycbcr_420_output = true;
+	} else if (conn_state->color_format == DRM_COLOR_FORMAT_YCBCR444) {
+		ycbcr_444_output = true;
+	}
+
+	if (ycbcr_420_output)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+	else if (ycbcr_444_output)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+	else
+		crtc_state->sink_format = intel_hdmi_sink_format(crtc_state,
+								 connector,
+								 ycbcr_420_output);
 
-	if (ycbcr_420_only && crtc_state->sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) {
+	if (ycbcr_420_output && crtc_state->sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) {
 		drm_dbg_kms(display->drm,
 			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
 		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
@@ -2278,6 +2311,19 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 		ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
 	}
 
+	intel_convert_color_fmt =
+		intel_output_format_to_drm_color_format(crtc_state->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != intel_convert_color_fmt &&
+	    conn_state->color_format != DRM_COLOR_FORMAT_AUTO) {
+		drm_dbg_kms(display->drm,
+			    "Unsupported color format %s (0x%x), current sink color format 0x%x\n",
+			    drm_get_color_format_name(conn_state->color_format),
+			    conn_state->color_format,
+			    intel_convert_color_fmt);
+		ret = -EINVAL;
+	}
+
 	return ret;
 }
 
@@ -2668,8 +2714,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *_
 	if (DISPLAY_VER(display) >= 10)
 		drm_connector_attach_hdr_output_metadata_property(&connector->base);
 
-	if (!HAS_GMCH(display))
+	if (!HAS_GMCH(display)) {
 		drm_connector_attach_max_bpc_property(&connector->base, 8, 12);
+		intel_attach_hdmi_colorformat_property(&connector->base);
+	}
 }
 
 /*
-- 
2.47.2

[PATCH 7/8] drm/amdgpu: Implement "color format" DRM property

[Marius Vlad]

From: Andri Yngvason <andri@yngvason.is>

This includes YUV420 as well YUV444 and Auto. Auto will fallback to RGB
to keep things sane and still working, similar to i915.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Co-Developed-by: Andri Yngvason <andri@yngvason.is>
Co-Developed-by: Marius Vlad <marius.vlad@collabora.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 74 +++++++++++++++++--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 13 ++++
 2 files changed, 81 insertions(+), 6 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 cc19c4bfd623..beb127471a61 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6426,15 +6426,32 @@ static void fill_stream_properties_from_drm_display_mode(
 	timing_out->h_border_right = 0;
 	timing_out->v_border_top = 0;
 	timing_out->v_border_bottom = 0;
-	/* TODO: un-hardcode */
-	if (drm_mode_is_420_only(info, mode_in) || (drm_mode_is_420_also(info, mode_in) &&
-	    aconnector && aconnector->force_yuv420_output))
+
+	if (drm_mode_is_420_also(info, mode_in) && aconnector &&
+	    (connector_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+	     aconnector->force_yuv420_output)) {
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
-			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+	} else if ((connector_state && connector_state->color_format == DRM_COLOR_FORMAT_YCBCR444 &&
+		  connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
+			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A) {
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
-	else
+	} else if (connector_state && connector_state->color_format == DRM_COLOR_FORMAT_RGB444 &&
+		 !drm_mode_is_420_only(info, mode_in)) {
 		timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+	} else {
+		/*
+		 * connector_state->force_color_format not possible
+		 * || connector_state->force_color_format == 0 (auto)
+		 * || connector_state->force_color_format == DRM_COLOR_FORMAT_YCBCR422
+		 */
+		if (drm_mode_is_420_only(info, mode_in))
+			timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+		else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
+			 && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+			timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+		else
+			timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+	}
 
 	timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
 	timing_out->display_color_depth = convert_color_depth_from_display_info(
@@ -7664,6 +7681,37 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 	return dc_result;
 }
 
+static enum dc_status
+dm_validate_stream_color_format(const struct drm_connector_state *drm_state,
+				const struct dc_stream_state *stream)
+{
+	if (!drm_state->color_format ||
+	    drm_state->color_format == DRM_COLOR_FORMAT_NONE)
+		return DC_OK;
+
+	enum dc_pixel_encoding encoding = PIXEL_ENCODING_UNDEFINED;
+	switch (drm_state->color_format) {
+	case DRM_COLOR_FORMAT_AUTO:
+	case DRM_COLOR_FORMAT_RGB444:
+		encoding = PIXEL_ENCODING_RGB;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR444:
+		encoding = PIXEL_ENCODING_YCBCR444;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR422:
+		encoding = PIXEL_ENCODING_YCBCR422;
+		break;
+	case DRM_COLOR_FORMAT_YCBCR420:
+		encoding = PIXEL_ENCODING_YCBCR420;
+		break;
+	default:
+		break;
+	}
+
+	return encoding == stream->timing.pixel_encoding ?
+		DC_OK : DC_UNSUPPORTED_VALUE;
+}
+
 struct dc_stream_state *
 create_validate_stream_for_sink(struct drm_connector *connector,
 				const struct drm_display_mode *drm_mode,
@@ -7709,6 +7757,9 @@ create_validate_stream_for_sink(struct drm_connector *connector,
 		if (dc_result == DC_OK)
 			dc_result = dm_validate_stream_and_context(adev->dm.dc, stream);
 
+		if (dc_result == DC_OK)
+			dc_result = dm_validate_stream_color_format(drm_state, stream);
+
 		if (dc_result != DC_OK) {
 			DRM_DEBUG_KMS("Pruned mode %d x %d (clk %d) %s %s -- %s\n",
 				      drm_mode->hdisplay,
@@ -8477,6 +8528,13 @@ static const u32 supported_colorspaces =
 	BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
 	BIT(DRM_MODE_COLORIMETRY_BT2020_YCC);
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444 |
+       DRM_COLOR_FORMAT_YCBCR422 |
+       DRM_COLOR_FORMAT_YCBCR420;
+
 void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				     struct amdgpu_dm_connector *aconnector,
 				     int connector_type,
@@ -8564,10 +8622,14 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	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);
+		if (!drm_mode_create_hdmi_color_format_property(&aconnector->base, supported_colorformats))
+			drm_connector_attach_color_format_property(&aconnector->base);
 	} else if ((connector_type == DRM_MODE_CONNECTOR_DisplayPort && !aconnector->mst_root) ||
 		   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 (!drm_mode_create_dp_color_format_property(&aconnector->base, supported_colorformats))
+			drm_connector_attach_color_format_property(&aconnector->base);
 	}
 
 	if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 6a817508c826..01f8f26fde0a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -52,6 +52,13 @@
 
 #define PEAK_FACTOR_X1000 1006
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444 |
+       DRM_COLOR_FORMAT_YCBCR422 |
+       DRM_COLOR_FORMAT_YCBCR420;
+
 /*
  * This function handles both native AUX and I2C-Over-AUX transactions.
  */
@@ -679,6 +686,12 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	if (connector->max_bpc_property)
 		drm_connector_attach_max_bpc_property(connector, 8, 16);
 
+	connector->color_format_property = master->base.color_format_property;
+	if (connector->color_format_property) {
+		if (!drm_mode_create_dp_color_format_property(&aconnector->base, supported_colorformats))
+			drm_connector_attach_color_format_property(&aconnector->base);
+	}
+
 	connector->vrr_capable_property = master->base.vrr_capable_property;
 	if (connector->vrr_capable_property)
 		drm_connector_attach_vrr_capable_property(connector);
-- 
2.47.2

[PATCH 8/8] drm/rockchip: Implement "color format" DRM property

[Marius Vlad]

From: Derek Foreman <derek.foreman@collabora.com>

This adds YUV444 and Auto, which will fallback to RGB as per
commit "drm: Pass supported color formats straight onto drm_bridge".

Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
---
 .../gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 10 +++-
 drivers/gpu/drm/rockchip/inno_hdmi.c          |  8 ++++
 drivers/gpu/drm/rockchip/rk3066_hdmi.c        |  8 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 46 +++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h  |  2 +
 5 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 58e24669ef34..794b8de9c9c5 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -427,6 +427,11 @@ static const struct of_device_id dw_hdmi_qp_rockchip_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, dw_hdmi_qp_rockchip_dt_ids);
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444;
+
 static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 				    void *data)
 {
@@ -563,13 +568,16 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	connector = drm_bridge_connector_init(drm, encoder, BIT(HDMI_COLORSPACE_RGB));
+	connector = drm_bridge_connector_init(drm, encoder, supported_colorformats);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
 		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 
+	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
+		drm_connector_attach_color_format_property(connector);
+
 	return drm_connector_attach_encoder(connector, encoder);
 }
 
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 1ab3ad4bde9e..59f0f055cdf1 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -810,6 +810,11 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
 	return 0;
 }
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444;
+
 static int inno_hdmi_setup(struct inno_hdmi *hdmi,
 			   struct drm_atomic_state *state)
 {
@@ -826,6 +831,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
 	if (WARN_ON(!new_crtc_state))
 		return -EINVAL;
 
+	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
+		drm_connector_attach_color_format_property(connector);
+
 	/* Mute video and audio output */
 	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
 		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
index ae4a5ac2299a..4794ab334cb2 100644
--- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
+++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
@@ -302,6 +302,11 @@ static void rk3066_hdmi_config_phy(struct rk3066_hdmi *hdmi)
 	}
 }
 
+static const u32 supported_colorformats =
+       DRM_COLOR_FORMAT_AUTO |
+       DRM_COLOR_FORMAT_RGB444 |
+       DRM_COLOR_FORMAT_YCBCR444;
+
 static int rk3066_hdmi_setup(struct rk3066_hdmi *hdmi,
 			     struct drm_atomic_state *state)
 {
@@ -322,6 +327,9 @@ static int rk3066_hdmi_setup(struct rk3066_hdmi *hdmi,
 	if (WARN_ON(!new_crtc_state))
 		return -EINVAL;
 
+	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
+		drm_connector_attach_color_format_property(connector);
+
 	display = &connector->display_info;
 	mode = &new_crtc_state->adjusted_mode;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 186f6452a7d3..5634e59a6412 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1543,6 +1543,50 @@ static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
 				DITHER_DOWN_ALLEGRO);
 }
 
+static void vop2_bcsh_config(struct drm_crtc *crtc, struct vop2_video_port *vp)
+{
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+	u32 format = 0;
+	enum drm_colorspace colorspace = 0;
+	u32 val = 0;
+
+	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (!(crtc->state->connector_mask & drm_connector_mask(connector)))
+			continue;
+
+		format = connector->state->color_format;
+		colorspace = connector->state->colorspace;
+		break;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	if (format == DRM_COLOR_FORMAT_YCBCR420 ||
+	    format == DRM_COLOR_FORMAT_YCBCR444 ||
+	    format == DRM_COLOR_FORMAT_YCBCR422) {
+		val = RK3568_VP_BCSH_CTRL__BCSH_R2Y_EN | BIT(7);
+
+		switch (colorspace) {
+		case DRM_MODE_COLORIMETRY_BT2020_RGB:
+		case DRM_MODE_COLORIMETRY_BT2020_YCC:
+			val |= BIT(7) | BIT(6);
+			break;
+		case DRM_MODE_COLORIMETRY_BT709_YCC:
+			val |= BIT(6);
+			break;
+		default:
+			break;
+		}
+		if (colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB ||
+		    colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC)
+			val |= BIT(6);
+	}
+
+	vop2_vp_write(vp, RK3568_VP_BCSH_CTRL, val);
+	vop2_vp_write(vp, RK3568_VP_BCSH_COLOR_BAR, 0);
+}
+
 static void vop2_post_config(struct drm_crtc *crtc)
 {
 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
@@ -1594,6 +1638,8 @@ static void vop2_post_config(struct drm_crtc *crtc)
 	}
 
 	vop2_vp_write(vp, RK3568_VP_DSP_BG, 0);
+
+	vop2_bcsh_config(crtc, vp);
 }
 
 static int us_to_vertical_line(struct drm_display_mode *mode, int us)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
index fa5c56f16047..0b589f326093 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -638,6 +638,8 @@ enum dst_factor_mode {
 
 #define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN		BIT(15)
 
+#define RK3568_VP_BCSH_CTRL__BCSH_R2Y_EN		BIT(4)
+
 #define RK3568_VP_DSP_CTRL__STANDBY			BIT(31)
 #define RK3568_VP_DSP_CTRL__DSP_LUT_EN			BIT(28)
 #define RK3568_VP_DSP_CTRL__DITHER_DOWN_MODE		BIT(20)
-- 
2.47.2

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Thu, Sep 11, 2025 at 04:07:34PM +0300, Marius Vlad wrote:
> From: Andri Yngvason <andri@yngvason.is>
> 
> Adds a new general DRM property named "color format" which can be used by
> userspace to force the display driver output a particular color format.
> 
> Possible options are:
>     - auto (setup by default, driver internally picks the color format)
>     - rgb
>     - ycbcr444
>     - ycbcr422
>     - ycbcr420
> 
> Drivers should advertise from this list the formats which they support.
> Together with this list and EDID data from the sink we should be able
> to relay a list of usable color formats to users to pick from.

It's unclear, who should be combining this data: should it be the kernel
or the userspace.

From my POV deferring this to the userspace doesn't make sense: there
will be different quirks for monitors / panels, e.g. the EDID wrongly
advertising YUV or not advertising a knowlingly-working capability.

I think that the property should reflect the kernel view on the possible
formats, which should be updated during get_modes() (or every time EDID
is being read).

And that's not to mention that there are enough use-cases where the
connector doesn't have EDID at all.

> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |   5 +
>  drivers/gpu/drm/drm_atomic_uapi.c   |   4 +
>  drivers/gpu/drm/drm_connector.c     | 177 ++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         |  54 ++++++++-
>  4 files changed, 235 insertions(+), 5 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Dmitry Baryshkov]

On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> Initialize drm_brige with advertised colors formats straight on.
> 
> Drivers that make use of DRM helpers would check the
> drm_brige::supported_formats bit-field list and refuse to use the color
> format passed. Drivers that make use of drm_bridge can pass the
> supported color formats in the bridge as well as supported color format
> for the DRM color format property.

Your commit message doesn't match patch contents. You are pushing format
selection to the instance creating drm_bridge_connector, which
frequently has no idea about the other end of the chain - the bridges
which actually send pixel data to the monitor.

We have drm_bridge::ycbcr_420_allowed with clearly defined meaning. I
think it would be wise to start from that and to describe why such a
field doesn't fulfill your needs.

> 
> This includes a fallback to RGB when Auto has been selected.
> 
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c      | 2 +-
>  drivers/gpu/drm/bridge/ite-it6263.c               | 2 +-
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c             | 3 ++-
>  drivers/gpu/drm/display/drm_bridge_connector.c    | 4 ++--
>  drivers/gpu/drm/imx/dcss/dcss-kms.c               | 2 +-
>  drivers/gpu/drm/mediatek/mtk_dpi.c                | 2 +-
>  drivers/gpu/drm/mediatek/mtk_dsi.c                | 2 +-
>  drivers/gpu/drm/meson/meson_encoder_cvbs.c        | 3 ++-
>  drivers/gpu/drm/meson/meson_encoder_hdmi.c        | 4 ++--
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c          | 2 +-
>  drivers/gpu/drm/msm/dp/dp_drm.c                   | 3 ++-
>  drivers/gpu/drm/msm/dsi/dsi_manager.c             | 2 +-
>  drivers/gpu/drm/msm/hdmi/hdmi.c                   | 2 +-
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c | 2 +-
>  drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  | 2 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c            | 2 +-
>  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 2 +-
>  drivers/gpu/drm/rockchip/rockchip_lvds.c          | 2 +-
>  drivers/gpu/drm/tegra/hdmi.c                      | 2 +-
>  drivers/gpu/drm/tegra/rgb.c                       | 2 +-
>  drivers/gpu/drm/tidss/tidss_encoder.c             | 2 +-
>  include/drm/drm_bridge_connector.h                | 3 ++-
>  22 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 9a461ab2f32f..8d5299849be6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -368,7 +368,8 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
>  {
>  	struct drm_connector *connector = NULL;
>  
> -	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
> +	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder,
> +					      BIT(HDMI_COLORSPACE_RGB));

Just to point out: this is wrong.

>  	if (IS_ERR(connector))
>  		return connector;
>  
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index ca400924d4ee..4b87f4f78d38 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
>  	if (ret)
>  		return ret;
>  
> -	connector = drm_bridge_connector_init(dev, encoder);
> +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));

And this totally depends on the bridge chain. If we have a DSI-to-HDMI
bridge somewhere in the middle, we are able to output YUV data to the
HDMI connector.

>  	if (IS_ERR(connector)) {
>  		DRM_ERROR("Unable to create bridge connector\n");
>  		return PTR_ERR(connector);

-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Marius Vlad]

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

On Thu, Sep 11, 2025 at 04:50:59PM +0300, Dmitry Baryshkov wrote:
> On Thu, Sep 11, 2025 at 04:07:34PM +0300, Marius Vlad wrote:
> > From: Andri Yngvason <andri@yngvason.is>
> > 
> > Adds a new general DRM property named "color format" which can be used by
> > userspace to force the display driver output a particular color format.
> > 
> > Possible options are:
> >     - auto (setup by default, driver internally picks the color format)
> >     - rgb
> >     - ycbcr444
> >     - ycbcr422
> >     - ycbcr420
> > 
> > Drivers should advertise from this list the formats which they support.
> > Together with this list and EDID data from the sink we should be able
> > to relay a list of usable color formats to users to pick from.
> 
> It's unclear, who should be combining this data: should it be the kernel
> or the userspace.
Userspace. I've went a bit into why that is in the cover letter. That
was a pain point in the previous versions raised by other folks. Drivers
are free to advertise whatever color formats their HW supports. To
filter what panel/display supports userspace would look at the EDID and do an
intersection with the ones with the driver. This not uncommon, userspace
already looks today at the EDID for color management / HDR purposes. There's
just too much for the kernel to handle and rather than offloading that
to the kernel, people suggested previously to let userspace handle that.
> 
> From my POV deferring this to the userspace doesn't make sense: there
> will be different quirks for monitors / panels, e.g. the EDID wrongly
> advertising YUV or not advertising a knowlingly-working capability.
Yeah, for sure. There have been some folks also raising that and discussing 
that a bit further in previous thread on similar topic:
https://lore.kernel.org/dri-devel/TY4PR01MB14432B688209B2AA416A95228983EA@TY4PR01MB14432.jpnprd01.prod.outlook.com/

Note that people have added quirks into libdisplay-info library to
overcome these limitations. It is far more easier to that into a library
than in the kernel.

I think to some extent 'Auto' can fill up this task. Drivers can perform
these checks on their own, including the ability to look at the EDID data.
Most likely some of them do that today and performs actions based on
that (all the Y420 checks for instance).

> 
> I think that the property should reflect the kernel view on the possible
> formats, which should be updated during get_modes() (or every time EDID
> is being read).
The property advertises the supported color formats by display driver.
Userspace just filters these out based on what the sink supports. This
could just a policy in the compositor / UI.  There's nothing preventing
you to force push from those advertised formats.
> 
> And that's not to mention that there are enough use-cases where the
> connector doesn't have EDID at all.
Totally. But what would be the expectation in that case? Drivers can
still advertise 'Auto' if they'd like.
> 
> > 
> > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > Signed-off-by: Andri Yngvason <andri@yngvason.is>
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c |   5 +
> >  drivers/gpu/drm/drm_atomic_uapi.c   |   4 +
> >  drivers/gpu/drm/drm_connector.c     | 177 ++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h         |  54 ++++++++-
> >  4 files changed, 235 insertions(+), 5 deletions(-)
> > 
> 
> -- 
> With best wishes
> Dmitry

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

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Marius Vlad]

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

On Thu, Sep 11, 2025 at 04:55:29PM +0300, Dmitry Baryshkov wrote:
> On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> > Initialize drm_brige with advertised colors formats straight on.
> > 
> > Drivers that make use of DRM helpers would check the
> > drm_brige::supported_formats bit-field list and refuse to use the color
> > format passed. Drivers that make use of drm_bridge can pass the
> > supported color formats in the bridge as well as supported color format
> > for the DRM color format property.
> 
> Your commit message doesn't match patch contents. You are pushing format
> selection to the instance creating drm_bridge_connector, which
> frequently has no idea about the other end of the chain - the bridges
> which actually send pixel data to the monitor.

None of these changes in this patch actually perform a functional
change, it will just explicitly expose the fact that BIT(HDMI_COLORSPACE_RGB)
was embedded in drm_bridge_connector_init(). 

Commit description is a bit forthcoming explaining the rationale behind
the change. This actually allows the ability to pass a list of supported
formats the bridge itself, similar how do we do it with the connector.

If any of these are wrong, they were prior to me touching them.

> 
> We have drm_bridge::ycbcr_420_allowed with clearly defined meaning. I
> think it would be wise to start from that and to describe why such a
> field doesn't fulfill your needs.
Alright, I'll be looking into this. 
> 
> > 
> > This includes a fallback to RGB when Auto has been selected.
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c      | 2 +-
> >  drivers/gpu/drm/bridge/ite-it6263.c               | 2 +-
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c             | 3 ++-
> >  drivers/gpu/drm/display/drm_bridge_connector.c    | 4 ++--
> >  drivers/gpu/drm/imx/dcss/dcss-kms.c               | 2 +-
> >  drivers/gpu/drm/mediatek/mtk_dpi.c                | 2 +-
> >  drivers/gpu/drm/mediatek/mtk_dsi.c                | 2 +-
> >  drivers/gpu/drm/meson/meson_encoder_cvbs.c        | 3 ++-
> >  drivers/gpu/drm/meson/meson_encoder_hdmi.c        | 4 ++--
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c          | 2 +-
> >  drivers/gpu/drm/msm/dp/dp_drm.c                   | 3 ++-
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c             | 2 +-
> >  drivers/gpu/drm/msm/hdmi/hdmi.c                   | 2 +-
> >  drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c | 2 +-
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  | 2 +-
> >  drivers/gpu/drm/rockchip/cdn-dp-core.c            | 2 +-
> >  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 2 +-
> >  drivers/gpu/drm/rockchip/rockchip_lvds.c          | 2 +-
> >  drivers/gpu/drm/tegra/hdmi.c                      | 2 +-
> >  drivers/gpu/drm/tegra/rgb.c                       | 2 +-
> >  drivers/gpu/drm/tidss/tidss_encoder.c             | 2 +-
> >  include/drm/drm_bridge_connector.h                | 3 ++-
> >  22 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 9a461ab2f32f..8d5299849be6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -368,7 +368,8 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
> >  {
> >  	struct drm_connector *connector = NULL;
> >  
> > -	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
> > +	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder,
> > +					      BIT(HDMI_COLORSPACE_RGB));
> 
> Just to point out: this is wrong.
Yeah, I understand why you're saying that just that I haven't really
modified these.
> 
> >  	if (IS_ERR(connector))
> >  		return connector;
> >  
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index ca400924d4ee..4b87f4f78d38 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
> >  	if (ret)
> >  		return ret;
> >  
> > -	connector = drm_bridge_connector_init(dev, encoder);
> > +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
> 
> And this totally depends on the bridge chain. If we have a DSI-to-HDMI
> bridge somewhere in the middle, we are able to output YUV data to the
> HDMI connector.
That's actually the usecase for this patch: to allow passing other color
formats, but this patch is a transitory patch to further expose the fact
drm_bridge_connector_init was embedding BIT(HDMI_COLORSPACE_RGB) for the
format. See rockchip implementation for this bit, the last patch in this
series.
> 
> >  	if (IS_ERR(connector)) {
> >  		DRM_ERROR("Unable to create bridge connector\n");
> >  		return PTR_ERR(connector);
> 
> -- 
> With best wishes
> Dmitry

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

Re: [PATCH 2/8] hdmi: Add HDMI_COLORSPACE_AUTO enum option

[Maxime Ripard]

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

Hi,

On Thu, Sep 11, 2025 at 04:07:33PM +0300, Marius Vlad wrote:
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 45b42f14a750..74fe925c69a2 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1031,8 +1031,8 @@ static const char *hdmi_colorspace_get_name(enum hdmi_colorspace colorspace)
>  		return "YCbCr 4:4:4";
>  	case HDMI_COLORSPACE_YUV420:
>  		return "YCbCr 4:2:0";
> -	case HDMI_COLORSPACE_RESERVED4:
> -		return "Reserved (4)";
> +	case HDMI_COLORSPACE_AUTO:
> +		return "Auto";
>  	case HDMI_COLORSPACE_RESERVED5:
>  		return "Reserved (5)";
>  	case HDMI_COLORSPACE_RESERVED6:
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 96bda41d9148..045402033763 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -82,7 +82,7 @@ enum hdmi_colorspace {
>  	HDMI_COLORSPACE_YUV422,
>  	HDMI_COLORSPACE_YUV444,
>  	HDMI_COLORSPACE_YUV420,
> -	HDMI_COLORSPACE_RESERVED4,
> +	HDMI_COLORSPACE_AUTO,
>  	HDMI_COLORSPACE_RESERVED5,
>  	HDMI_COLORSPACE_RESERVED6,
>  	HDMI_COLORSPACE_IDO_DEFINED,

I'm not sure we can use hdmi_colorspace as is. This is the enum that
represents the colorspace encoded in the AVI infoframe, so we can't
change it, really.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 4/8] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Maxime Ripard]

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

On Thu, Sep 11, 2025 at 04:07:35PM +0300, Marius Vlad wrote:
> This would please the compiler to have a enum transformation from one to
> another even though the values are the same. It should also make things
> obvious that we use different enums.
> 
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> ---
>  .../gpu/drm/display/drm_hdmi_state_helper.c    |  4 +++-
>  drivers/gpu/drm/drm_connector.c                | 18 ++++++++++++++++++
>  include/drm/drm_connector.h                    |  3 +++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index f9aa922d25d3..dc2f32651cb9 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -653,9 +653,11 @@ hdmi_compute_config(const struct drm_connector *connector,
>  				       conn_state->max_bpc,
>  				       8, connector->max_bpc);
>  	int ret;
> +	enum hdmi_colorspace hdmi_colorspace =
> +		color_format_to_hdmi_colorspace(conn_state->color_format);
>  
>  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> -				      HDMI_COLORSPACE_RGB);
> +				      hdmi_colorspace);
>  	if (ret) {
>  		if (connector->ycbcr_420_allowed) {
>  			ret = hdmi_compute_format_bpc(connector, conn_state,

Does it really make sense to have the fallback to YUV420 here? I would
expect to get rid of it, or only use that for "auto".

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Maxime Ripard]

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

Hi,

On Thu, Sep 11, 2025 at 08:34:48PM +0300, Marius Vlad wrote:
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > index ca400924d4ee..4b87f4f78d38 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	connector = drm_bridge_connector_init(dev, encoder);
> > > +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
> > 
> > And this totally depends on the bridge chain. If we have a DSI-to-HDMI
> > bridge somewhere in the middle, we are able to output YUV data to the
> > HDMI connector.
> That's actually the usecase for this patch: to allow passing other color
> formats, but this patch is a transitory patch to further expose the fact
> drm_bridge_connector_init was embedding BIT(HDMI_COLORSPACE_RGB) for the
> format. See rockchip implementation for this bit, the last patch in this
> series.

I think Dmitry's point is that it needs to be integrated with the
atomic_get_input_bus_fmt / atomic_get_output_bus_fmt, because not only
we need to make sure the monitor supports it, and the userspace demands
it, we also need to make sure every bridge in the chain (and possibly
the encoder) can implement it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Maxime Ripard]

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

On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index 61cab32e213a..15820e6ba057 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -1057,7 +1057,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		goto err_cleanup;
>  
> -	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder);
> +	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder, BIT(HDMI_COLORSPACE_RGB));
>  	if (IS_ERR(dpi->connector)) {
>  		dev_err(dev, "Unable to create bridge connector\n");
>  		ret = PTR_ERR(dpi->connector);
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index d7726091819c..91afdbf676f0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -930,7 +930,7 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
>  	if (ret)
>  		goto err_cleanup_encoder;
>  
> -	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder);
> +	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder, BIT(HDMI_COLORSPACE_RGB));
>  	if (IS_ERR(dsi->connector)) {
>  		DRM_ERROR("Unable to create bridge connector\n");
>  		ret = PTR_ERR(dsi->connector);
> diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> index dc374bfc5951..a475fc34ca23 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> @@ -275,7 +275,8 @@ int meson_encoder_cvbs_probe(struct meson_drm *priv)
>  	}
>  
>  	/* Initialize & attach Bridge Connector */
> -	connector = drm_bridge_connector_init(priv->drm, &meson_encoder_cvbs->encoder);
> +	connector = drm_bridge_connector_init(priv->drm,
> +			&meson_encoder_cvbs->encoder, BIT(HDMI_COLORSPACE_RGB));
>  	if (IS_ERR(connector))
>  		return dev_err_probe(priv->dev, PTR_ERR(connector),
>  				     "Unable to create CVBS bridge connector\n");

Why do we need to pass an HDMI color format for a DSI, DPI or Analog TV
driver?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/8] hdmi: Add HDMI_COLORSPACE_AUTO enum option

[Marius Vlad]

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

On Fri, Sep 12, 2025 at 05:17:56PM +0200, Maxime Ripard wrote:
> Hi,
Hi Maxime,
> 
> On Thu, Sep 11, 2025 at 04:07:33PM +0300, Marius Vlad wrote:
> > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> > index 45b42f14a750..74fe925c69a2 100644
> > --- a/drivers/video/hdmi.c
> > +++ b/drivers/video/hdmi.c
> > @@ -1031,8 +1031,8 @@ static const char *hdmi_colorspace_get_name(enum hdmi_colorspace colorspace)
> >  		return "YCbCr 4:4:4";
> >  	case HDMI_COLORSPACE_YUV420:
> >  		return "YCbCr 4:2:0";
> > -	case HDMI_COLORSPACE_RESERVED4:
> > -		return "Reserved (4)";
> > +	case HDMI_COLORSPACE_AUTO:
> > +		return "Auto";
> >  	case HDMI_COLORSPACE_RESERVED5:
> >  		return "Reserved (5)";
> >  	case HDMI_COLORSPACE_RESERVED6:
> > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> > index 96bda41d9148..045402033763 100644
> > --- a/include/linux/hdmi.h
> > +++ b/include/linux/hdmi.h
> > @@ -82,7 +82,7 @@ enum hdmi_colorspace {
> >  	HDMI_COLORSPACE_YUV422,
> >  	HDMI_COLORSPACE_YUV444,
> >  	HDMI_COLORSPACE_YUV420,
> > -	HDMI_COLORSPACE_RESERVED4,
> > +	HDMI_COLORSPACE_AUTO,
> >  	HDMI_COLORSPACE_RESERVED5,
> >  	HDMI_COLORSPACE_RESERVED6,
> >  	HDMI_COLORSPACE_IDO_DEFINED,
> 
> I'm not sure we can use hdmi_colorspace as is. This is the enum that
> represents the colorspace encoded in the AVI infoframe, so we can't
> change it, really.
I see. Was hoping I can re-use these as is when defining the color
format enum.

Should I just de-couple the color format entirely from the enum hdmi
colorspace? With the enum color format in it feels they sort of overlap,
with some drivers explicitly using the hdmi colorspace enum others the
color format enum. Feels a bit inconsistent but maybe that's just me
and folks do not see this as an issue.

Perhaps just handle 'Auto' distinctly but still re-use the hdmi
colorspace enum? 

Any (strong) preference? 
> 
> Maxime



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

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Marius Vlad]

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

On Fri, Sep 12, 2025 at 05:31:17PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Sep 11, 2025 at 08:34:48PM +0300, Marius Vlad wrote:
> > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > index ca400924d4ee..4b87f4f78d38 100644
> > > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	connector = drm_bridge_connector_init(dev, encoder);
> > > > +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
> > > 
> > > And this totally depends on the bridge chain. If we have a DSI-to-HDMI
> > > bridge somewhere in the middle, we are able to output YUV data to the
> > > HDMI connector.
> > That's actually the usecase for this patch: to allow passing other color
> > formats, but this patch is a transitory patch to further expose the fact
> > drm_bridge_connector_init was embedding BIT(HDMI_COLORSPACE_RGB) for the
> > format. See rockchip implementation for this bit, the last patch in this
> > series.
> 
> I think Dmitry's point is that it needs to be integrated with the
> atomic_get_input_bus_fmt / atomic_get_output_bus_fmt, because not only
> we need to make sure the monitor supports it, and the userspace demands
> it, we also need to make sure every bridge in the chain (and possibly
> the encoder) can implement it.
Oookay. Will be looking at those bits then. Thanks for pointing where
I need to look at.

tbh it isn't super clear to me why is it now that an issue. If you don't
mind replying back, it this patch actually exposing that, or that was a
thing that had to addressed at one point? 
> 
> Maxime



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

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Marius Vlad]

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

On Fri, Sep 12, 2025 at 05:33:42PM +0200, Maxime Ripard wrote:
> On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index 61cab32e213a..15820e6ba057 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > @@ -1057,7 +1057,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
> >  	if (ret)
> >  		goto err_cleanup;
> >  
> > -	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder);
> > +	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder, BIT(HDMI_COLORSPACE_RGB));
> >  	if (IS_ERR(dpi->connector)) {
> >  		dev_err(dev, "Unable to create bridge connector\n");
> >  		ret = PTR_ERR(dpi->connector);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index d7726091819c..91afdbf676f0 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -930,7 +930,7 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
> >  	if (ret)
> >  		goto err_cleanup_encoder;
> >  
> > -	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder);
> > +	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder, BIT(HDMI_COLORSPACE_RGB));
> >  	if (IS_ERR(dsi->connector)) {
> >  		DRM_ERROR("Unable to create bridge connector\n");
> >  		ret = PTR_ERR(dsi->connector);
> > diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > index dc374bfc5951..a475fc34ca23 100644
> > --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > @@ -275,7 +275,8 @@ int meson_encoder_cvbs_probe(struct meson_drm *priv)
> >  	}
> >  
> >  	/* Initialize & attach Bridge Connector */
> > -	connector = drm_bridge_connector_init(priv->drm, &meson_encoder_cvbs->encoder);
> > +	connector = drm_bridge_connector_init(priv->drm,
> > +			&meson_encoder_cvbs->encoder, BIT(HDMI_COLORSPACE_RGB));
> >  	if (IS_ERR(connector))
> >  		return dev_err_probe(priv->dev, PTR_ERR(connector),
> >  				     "Unable to create CVBS bridge connector\n");
> 
> Why do we need to pass an HDMI color format for a DSI, DPI or Analog TV
> driver?
That's what drm_bridge_connector_init() had initially set as supported
color format. I just pulled that out for every other driver that made
use of drm_bridge_connector_init. So I guess the answer is we don't
actually need to do that.
> 
> Maxime



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

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Dmitry Baryshkov]

On Fri, Sep 12, 2025 at 09:57:47PM +0300, Marius Vlad wrote:
> On Fri, Sep 12, 2025 at 05:31:17PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, Sep 11, 2025 at 08:34:48PM +0300, Marius Vlad wrote:
> > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > index ca400924d4ee..4b87f4f78d38 100644
> > > > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > > > @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >  
> > > > > -	connector = drm_bridge_connector_init(dev, encoder);
> > > > > +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
> > > > 
> > > > And this totally depends on the bridge chain. If we have a DSI-to-HDMI
> > > > bridge somewhere in the middle, we are able to output YUV data to the
> > > > HDMI connector.
> > > That's actually the usecase for this patch: to allow passing other color
> > > formats, but this patch is a transitory patch to further expose the fact
> > > drm_bridge_connector_init was embedding BIT(HDMI_COLORSPACE_RGB) for the
> > > format. See rockchip implementation for this bit, the last patch in this
> > > series.
> > 
> > I think Dmitry's point is that it needs to be integrated with the
> > atomic_get_input_bus_fmt / atomic_get_output_bus_fmt, because not only
> > we need to make sure the monitor supports it, and the userspace demands
> > it, we also need to make sure every bridge in the chain (and possibly
> > the encoder) can implement it.
> Oookay. Will be looking at those bits then. Thanks for pointing where
> I need to look at.
> 
> tbh it isn't super clear to me why is it now that an issue. If you don't
> mind replying back, it this patch actually exposing that, or that was a
> thing that had to addressed at one point? 

As I wrote, the encoder doesn't know which colorformats are supported at
the end of the bridge chain. I think that at some point we will have to
redo ycbcr420_supported too. Basically, consider the following chain:
encoder -> DSI host -> DSI-to-HDMI bridge -> HDMI connector bridge

The DSI host can send RGB only, but then the DSI-to-HDMI bridge might be
able to do all the conversion and send YUV 444 or 420 over the wire. The
HDMI connector (yes, it's a bridge too) doesn't really care, it just
passes the data through.

By adding supported_formats to drm_bridge_connector_init() you are
insisting on the encoder / root driver knowing what formats are
supported. It's not the case. The negotiation should start from the end
of the chain and follow back until the bridge which says 'I can do
this, no matter what comes in'.


-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Thu, Sep 11, 2025 at 08:15:48PM +0300, Marius Vlad wrote:
> On Thu, Sep 11, 2025 at 04:50:59PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Sep 11, 2025 at 04:07:34PM +0300, Marius Vlad wrote:
> > > From: Andri Yngvason <andri@yngvason.is>
> > > 
> > > Adds a new general DRM property named "color format" which can be used by
> > > userspace to force the display driver output a particular color format.
> > > 
> > > Possible options are:
> > >     - auto (setup by default, driver internally picks the color format)
> > >     - rgb
> > >     - ycbcr444
> > >     - ycbcr422
> > >     - ycbcr420
> > > 
> > > Drivers should advertise from this list the formats which they support.
> > > Together with this list and EDID data from the sink we should be able
> > > to relay a list of usable color formats to users to pick from.
> > 
> > It's unclear, who should be combining this data: should it be the kernel
> > or the userspace.
> Userspace. I've went a bit into why that is in the cover letter. That
> was a pain point in the previous versions raised by other folks. Drivers
> are free to advertise whatever color formats their HW supports. To
> filter what panel/display supports userspace would look at the EDID and do an
> intersection with the ones with the driver. This not uncommon, userspace
> already looks today at the EDID for color management / HDR purposes. There's
> just too much for the kernel to handle and rather than offloading that
> to the kernel, people suggested previously to let userspace handle that.
> > 
> > From my POV deferring this to the userspace doesn't make sense: there
> > will be different quirks for monitors / panels, e.g. the EDID wrongly
> > advertising YUV or not advertising a knowlingly-working capability.
> Yeah, for sure. There have been some folks also raising that and discussing 
> that a bit further in previous thread on similar topic:
> https://lore.kernel.org/dri-devel/TY4PR01MB14432B688209B2AA416A95228983EA@TY4PR01MB14432.jpnprd01.prod.outlook.com/
> 
> Note that people have added quirks into libdisplay-info library to
> overcome these limitations. It is far more easier to that into a library
> than in the kernel.

This forces everybody who wishes to use this property to use that
library (or to duplicate the code, making it spread over different
projects).

> 
> I think to some extent 'Auto' can fill up this task. Drivers can perform
> these checks on their own, including the ability to look at the EDID data.
> Most likely some of them do that today and performs actions based on
> that (all the Y420 checks for instance).
> 
> > 
> > I think that the property should reflect the kernel view on the possible
> > formats, which should be updated during get_modes() (or every time EDID
> > is being read).
> The property advertises the supported color formats by display driver.
> Userspace just filters these out based on what the sink supports. This
> could just a policy in the compositor / UI.  There's nothing preventing
> you to force push from those advertised formats.

What is the expected behaviour if userspace asks for the colorspace
which is supported by the driver but not by the display?

> > 
> > And that's not to mention that there are enough use-cases where the
> > connector doesn't have EDID at all.
> Totally. But what would be the expectation in that case? Drivers can
> still advertise 'Auto' if they'd like.

I'm trying to point out that this complicates userspace: it is now
required to handle EDID and non-EDID cases for no practical reason. For
all other usecases it is enough to query available modes from the
kernel.

> > 
> > > 
> > > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > > Signed-off-by: Andri Yngvason <andri@yngvason.is>
> > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c |   5 +
> > >  drivers/gpu/drm/drm_atomic_uapi.c   |   4 +
> > >  drivers/gpu/drm/drm_connector.c     | 177 ++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h         |  54 ++++++++-
> > >  4 files changed, 235 insertions(+), 5 deletions(-)
> > > 
> > 
> > -- 
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Dmitry Baryshkov]

On Fri, Sep 12, 2025 at 10:09:54PM +0300, Marius Vlad wrote:
> On Fri, Sep 12, 2025 at 05:33:42PM +0200, Maxime Ripard wrote:
> > On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > index 61cab32e213a..15820e6ba057 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > @@ -1057,7 +1057,7 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
> > >  	if (ret)
> > >  		goto err_cleanup;
> > >  
> > > -	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder);
> > > +	dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder, BIT(HDMI_COLORSPACE_RGB));
> > >  	if (IS_ERR(dpi->connector)) {
> > >  		dev_err(dev, "Unable to create bridge connector\n");
> > >  		ret = PTR_ERR(dpi->connector);
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index d7726091819c..91afdbf676f0 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -930,7 +930,7 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
> > >  	if (ret)
> > >  		goto err_cleanup_encoder;
> > >  
> > > -	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder);
> > > +	dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder, BIT(HDMI_COLORSPACE_RGB));
> > >  	if (IS_ERR(dsi->connector)) {
> > >  		DRM_ERROR("Unable to create bridge connector\n");
> > >  		ret = PTR_ERR(dsi->connector);
> > > diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > > index dc374bfc5951..a475fc34ca23 100644
> > > --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > > +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> > > @@ -275,7 +275,8 @@ int meson_encoder_cvbs_probe(struct meson_drm *priv)
> > >  	}
> > >  
> > >  	/* Initialize & attach Bridge Connector */
> > > -	connector = drm_bridge_connector_init(priv->drm, &meson_encoder_cvbs->encoder);
> > > +	connector = drm_bridge_connector_init(priv->drm,
> > > +			&meson_encoder_cvbs->encoder, BIT(HDMI_COLORSPACE_RGB));
> > >  	if (IS_ERR(connector))
> > >  		return dev_err_probe(priv->dev, PTR_ERR(connector),
> > >  				     "Unable to create CVBS bridge connector\n");
> > 
> > Why do we need to pass an HDMI color format for a DSI, DPI or Analog TV
> > driver?
> That's what drm_bridge_connector_init() had initially set as supported
> color format. I just pulled that out for every other driver that made
> use of drm_bridge_connector_init. So I guess the answer is we don't
> actually need to do that.

The problem is that the host controller (which calls
drm_bridge_connector_init() ) doesn't know _at_ _all_ if this is going
to be HDMI connector or not.

-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

Hi Dmitry,

On Mon, 15 Sept 2025 at 02:57, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Thu, Sep 11, 2025 at 08:15:48PM +0300, Marius Vlad wrote:
> > On Thu, Sep 11, 2025 at 04:50:59PM +0300, Dmitry Baryshkov wrote:
> > > It's unclear, who should be combining this data: should it be the kernel
> > > or the userspace.
> >
> > Userspace. I've went a bit into why that is in the cover letter. That
> > was a pain point in the previous versions raised by other folks. Drivers
> > are free to advertise whatever color formats their HW supports. To
> > filter what panel/display supports userspace would look at the EDID and do an
> > intersection with the ones with the driver. This not uncommon, userspace
> > already looks today at the EDID for color management / HDR purposes. There's
> > just too much for the kernel to handle and rather than offloading that
> > to the kernel, people suggested previously to let userspace handle that.
> >
> > > From my POV deferring this to the userspace doesn't make sense: there
> > > will be different quirks for monitors / panels, e.g. the EDID wrongly
> > > advertising YUV or not advertising a knowlingly-working capability.
> >
> > Yeah, for sure. There have been some folks also raising that and discussing
> > that a bit further in previous thread on similar topic:
> > https://lore.kernel.org/dri-devel/TY4PR01MB14432B688209B2AA416A95228983EA@TY4PR01MB14432.jpnprd01.prod.outlook.com/
> >
> > Note that people have added quirks into libdisplay-info library to
> > overcome these limitations. It is far more easier to that into a library
> > than in the kernel.
>
> This forces everybody who wishes to use this property to use that
> library (or to duplicate the code, making it spread over different
> projects).

This really is already the case though. There is far more than the
kernel can parse and handle in connector modes - and far more than it
makes sense for the kernel to do.

The kernel absolutely should have enough to support simple usecases,
e.g. console and splash screen, to make sure that they work out of the
box no matter what. But once you get into HDR/YUV/VRR/stereo/etc
usecases, it doesn't make sense for the kernel to abstract the EDID
parsing so much that userspace never needs to touch it - it makes the
kernel just a lossy middle barrier. So if you look just at
compositors, all the big four of KWin, Mutter, Weston, and wlroots,
all use libdisplay-info to parse the EDID. And that's fine - they also
use libevdev and libinput to handle raw input data, and libxkbcommon
to deal with the pain that is keyboards.

> > > I think that the property should reflect the kernel view on the possible
> > > formats, which should be updated during get_modes() (or every time EDID
> > > is being read).
> >
> > The property advertises the supported color formats by display driver.
> > Userspace just filters these out based on what the sink supports. This
> > could just a policy in the compositor / UI.  There's nothing preventing
> > you to force push from those advertised formats.
>
> What is the expected behaviour if userspace asks for the colorspace
> which is supported by the driver but not by the display?

Quite possibly just a failure to display. Same as if the driver
guesses it wrong - including for reasons it can never statically
detect (e.g. buying a 10m-long uncertified HDMI cable which drops
signal, or having the cable pulled around a 90° bend making it very
marginal for transmission).

> > > And that's not to mention that there are enough use-cases where the
> > > connector doesn't have EDID at all.
> > Totally. But what would be the expectation in that case? Drivers can
> > still advertise 'Auto' if they'd like.
>
> I'm trying to point out that this complicates userspace: it is now
> required to handle EDID and non-EDID cases for no practical reason. For
> all other usecases it is enough to query available modes from the
> kernel.

But not 'now', because that's been happening for years. And not 'no
practical reason', because in order to support features the kernel has
no involvement in (colour management and HDR as a large part), you
need to see the full EDID.

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Mon, Sep 15, 2025 at 12:33:08PM +0200, Daniel Stone wrote:
> Hi Dmitry,
> 
> On Mon, 15 Sept 2025 at 02:57, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > On Thu, Sep 11, 2025 at 08:15:48PM +0300, Marius Vlad wrote:
> > > On Thu, Sep 11, 2025 at 04:50:59PM +0300, Dmitry Baryshkov wrote:
> > > > It's unclear, who should be combining this data: should it be the kernel
> > > > or the userspace.
> > >
> > > Userspace. I've went a bit into why that is in the cover letter. That
> > > was a pain point in the previous versions raised by other folks. Drivers
> > > are free to advertise whatever color formats their HW supports. To
> > > filter what panel/display supports userspace would look at the EDID and do an
> > > intersection with the ones with the driver. This not uncommon, userspace
> > > already looks today at the EDID for color management / HDR purposes. There's
> > > just too much for the kernel to handle and rather than offloading that
> > > to the kernel, people suggested previously to let userspace handle that.
> > >
> > > > From my POV deferring this to the userspace doesn't make sense: there
> > > > will be different quirks for monitors / panels, e.g. the EDID wrongly
> > > > advertising YUV or not advertising a knowlingly-working capability.
> > >
> > > Yeah, for sure. There have been some folks also raising that and discussing
> > > that a bit further in previous thread on similar topic:
> > > https://lore.kernel.org/dri-devel/TY4PR01MB14432B688209B2AA416A95228983EA@TY4PR01MB14432.jpnprd01.prod.outlook.com/
> > >
> > > Note that people have added quirks into libdisplay-info library to
> > > overcome these limitations. It is far more easier to that into a library
> > > than in the kernel.
> >
> > This forces everybody who wishes to use this property to use that
> > library (or to duplicate the code, making it spread over different
> > projects).
> 
> This really is already the case though. There is far more than the
> kernel can parse and handle in connector modes - and far more than it
> makes sense for the kernel to do.
> 
> The kernel absolutely should have enough to support simple usecases,
> e.g. console and splash screen, to make sure that they work out of the
> box no matter what. But once you get into HDR/YUV/VRR/stereo/etc
> usecases, it doesn't make sense for the kernel to abstract the EDID
> parsing so much that userspace never needs to touch it - it makes the
> kernel just a lossy middle barrier. So if you look just at
> compositors, all the big four of KWin, Mutter, Weston, and wlroots,
> all use libdisplay-info to parse the EDID. And that's fine - they also
> use libevdev and libinput to handle raw input data, and libxkbcommon
> to deal with the pain that is keyboards.

I understand the HDR part, but what is so special regarding YUV? We
already need to parse VSDB information inside the kernel.

> > > > I think that the property should reflect the kernel view on the possible
> > > > formats, which should be updated during get_modes() (or every time EDID
> > > > is being read).
> > >
> > > The property advertises the supported color formats by display driver.
> > > Userspace just filters these out based on what the sink supports. This
> > > could just a policy in the compositor / UI.  There's nothing preventing
> > > you to force push from those advertised formats.
> >
> > What is the expected behaviour if userspace asks for the colorspace
> > which is supported by the driver but not by the display?
> 
> Quite possibly just a failure to display. Same as if the driver
> guesses it wrong - including for reasons it can never statically
> detect (e.g. buying a 10m-long uncertified HDMI cable which drops
> signal, or having the cable pulled around a 90° bend making it very
> marginal for transmission).

Ack

> 
> > > > And that's not to mention that there are enough use-cases where the
> > > > connector doesn't have EDID at all.
> > > Totally. But what would be the expectation in that case? Drivers can
> > > still advertise 'Auto' if they'd like.
> >
> > I'm trying to point out that this complicates userspace: it is now
> > required to handle EDID and non-EDID cases for no practical reason. For
> > all other usecases it is enough to query available modes from the
> > kernel.
> 
> But not 'now', because that's been happening for years. And not 'no
> practical reason', because in order to support features the kernel has
> no involvement in (colour management and HDR as a large part), you
> need to see the full EDID.

As I wrote, I completely agree regarding CM and HDR. From my POV the YUV
part isn't that complicated. I might be wrong though.

-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Maxime Ripard]

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

On Mon, Sep 15, 2025 at 12:33:08PM +0200, Daniel Stone wrote:
> > > > I think that the property should reflect the kernel view on the possible
> > > > formats, which should be updated during get_modes() (or every time EDID
> > > > is being read).
> > >
> > > The property advertises the supported color formats by display driver.
> > > Userspace just filters these out based on what the sink supports. This
> > > could just a policy in the compositor / UI.  There's nothing preventing
> > > you to force push from those advertised formats.
> >
> > What is the expected behaviour if userspace asks for the colorspace
> > which is supported by the driver but not by the display?
> 
> Quite possibly just a failure to display. Same as if the driver
> guesses it wrong - including for reasons it can never statically
> detect (e.g. buying a 10m-long uncertified HDMI cable which drops
> signal, or having the cable pulled around a 90° bend making it very
> marginal for transmission).

I guess there's two cases for "not supported by the display"?

If the display reports that it supports a format but is broken, yeah,
there's not much we can do except maybe add a quirk.

But if it's that the monitor doesn't support that format in the first
place, we should just reject that commit.

Just like we don't let any mode go through if we know it's obviously
wrong (like if it exceeds max_tmds_clock) but can't indeed account for a
long / broken cable.

> > > > And that's not to mention that there are enough use-cases where the
> > > > connector doesn't have EDID at all.
> > > Totally. But what would be the expectation in that case? Drivers can
> > > still advertise 'Auto' if they'd like.
> >
> > I'm trying to point out that this complicates userspace: it is now
> > required to handle EDID and non-EDID cases for no practical reason. For
> > all other usecases it is enough to query available modes from the
> > kernel.
> 
> But not 'now', because that's been happening for years. And not 'no
> practical reason', because in order to support features the kernel has
> no involvement in (colour management and HDR as a large part), you
> need to see the full EDID.

I guess it's still slightly different when we're talking about opt-in
features like VRR or HDR, and "just get something on my screen".

Introducing a dependency on libdisplay-info for the latter is still
something new, but I guess you can always YOLO it, try a format and see
if it works.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

Hi,

On Tue, 16 Sept 2025 at 09:29, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 15, 2025 at 12:33:08PM +0200, Daniel Stone wrote:
> > Quite possibly just a failure to display. Same as if the driver
> > guesses it wrong - including for reasons it can never statically
> > detect (e.g. buying a 10m-long uncertified HDMI cable which drops
> > signal, or having the cable pulled around a 90° bend making it very
> > marginal for transmission).
>
> I guess there's two cases for "not supported by the display"?
>
> If the display reports that it supports a format but is broken, yeah,
> there's not much we can do except maybe add a quirk.
>
> But if it's that the monitor doesn't support that format in the first
> place, we should just reject that commit.
>
> Just like we don't let any mode go through if we know it's obviously
> wrong (like if it exceeds max_tmds_clock) but can't indeed account for a
> long / broken cable.
>
> > > I'm trying to point out that this complicates userspace: it is now
> > > required to handle EDID and non-EDID cases for no practical reason. For
> > > all other usecases it is enough to query available modes from the
> > > kernel.
> >
> > But not 'now', because that's been happening for years. And not 'no
> > practical reason', because in order to support features the kernel has
> > no involvement in (colour management and HDR as a large part), you
> > need to see the full EDID.
>
> I guess it's still slightly different when we're talking about opt-in
> features like VRR or HDR, and "just get something on my screen".

Yeah, I absolutely agree (as with the quoted parts as well) - the
kernel should absolutely just get stuff on the screen.

> Introducing a dependency on libdisplay-info for the latter is still
> something new, but I guess you can always YOLO it, try a format and see
> if it works.

Again though, it's not something new. I promise you that Weston (for
over a year), Mutter (for about a year), KWin (for over two years),
and wlroots (for two and a half years) already have hard deps on
libdisplay-info. Even outside of 'serious' compositors, Mesa requires
it to support HDR in VK_KHR_display (when it was added a couple of
months ago),

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

Typing with one hand (and not the useful one): not good.

On Tue, 16 Sept 2025 at 11:46, Daniel Stone <daniel@fooishbar.org> wrote:
> Again though, it's not something new. I promise you that Weston (for
> over a year), Mutter (for about a year), KWin (for over two years),
> and wlroots (for two and a half years) already have hard deps on
> libdisplay-info. Even outside of 'serious' compositors, Mesa requires
> it to support HDR in VK_KHR_display (when it was added a couple of
> months ago),

... and mpv has also required it for any DRM backend support (same as
all the compositors) for the past year.

So yeah, I see it as the same as the input situation: you _can_ do the
basics with raw evdev, but unless you're very special, you should use
libinput. Equally for output, when you go past what e.g. Plymouth
would require, use libdisplay-info to parse the EDID, rather than
trying to make the kernel try to turn the unhinged madness of EDID
into something userspace can reason about.

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Tue, Sep 16, 2025 at 11:48:39AM +0100, Daniel Stone wrote:
> Typing with one hand (and not the useful one): not good.
> 
> On Tue, 16 Sept 2025 at 11:46, Daniel Stone <daniel@fooishbar.org> wrote:
> > Again though, it's not something new. I promise you that Weston (for
> > over a year), Mutter (for about a year), KWin (for over two years),
> > and wlroots (for two and a half years) already have hard deps on
> > libdisplay-info. Even outside of 'serious' compositors, Mesa requires
> > it to support HDR in VK_KHR_display (when it was added a couple of
> > months ago),
> 
> ... and mpv has also required it for any DRM backend support (same as
> all the compositors) for the past year.
> 
> So yeah, I see it as the same as the input situation: you _can_ do the
> basics with raw evdev, but unless you're very special, you should use
> libinput. Equally for output, when you go past what e.g. Plymouth
> would require, use libdisplay-info to parse the EDID, rather than
> trying to make the kernel try to turn the unhinged madness of EDID
> into something userspace can reason about.

We do a lot of EDID parsing in the kernel, including HDMI VSDB and
Y420CMDB parsing. Do we need anything else for this feature?

-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

On Mon, 15 Sept 2025 at 11:46, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Mon, Sep 15, 2025 at 12:33:08PM +0200, Daniel Stone wrote:
> > But not 'now', because that's been happening for years. And not 'no
> > practical reason', because in order to support features the kernel has
> > no involvement in (colour management and HDR as a large part), you
> > need to see the full EDID.
>
> As I wrote, I completely agree regarding CM and HDR. From my POV the YUV
> part isn't that complicated. I might be wrong though.

It's not super complicated if you just want to get a splash screen up,
and you're willing to be conservative about the way in which you do it
to get _something_ up on screen. Or if you're on a laptop so you'd
rather not have your HDMI clock smashed up to max all the time. But
yeah, as soon as you get to CM and HDR usecases, userspace really
wants to have both control and visibility here.

The cable usecase is a very real one - you want to use the most
conservative setting possible to make sure you get _something_ on
screen. But then the CM usecase is a very real one too - you want to
get the best image possible on screen rather than destroying accuracy,
even at the cost of the dreaded 'Can you see this now you've changed
your display settings? [Y] [N, timeout 20sec]' dialog box.

So yeah, i'm not really seeing any way around giving userspace
explicit visibility and control here - the kernel can't 'just do the
right thing' in all cases, and creating new uAPI to abstract EDID
seems the wrong direction as a) there's already a perfectly good uAPI
for EDID, and b) it means the kernel has to do a lot of complex
interpretation and transformation-of-representation of information it
won't even do anything with.

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

On Tue, 16 Sept 2025 at 11:57, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Tue, Sep 16, 2025 at 11:48:39AM +0100, Daniel Stone wrote:
> > So yeah, I see it as the same as the input situation: you _can_ do the
> > basics with raw evdev, but unless you're very special, you should use
> > libinput. Equally for output, when you go past what e.g. Plymouth
> > would require, use libdisplay-info to parse the EDID, rather than
> > trying to make the kernel try to turn the unhinged madness of EDID
> > into something userspace can reason about.
>
> We do a lot of EDID parsing in the kernel, including HDMI VSDB and
> Y420CMDB parsing. Do we need anything else for this feature?

I'm slightly confused as to what you're saying here. Are you saying
that it's OK for the kernel to expose connector properties for
userspace to see which colour properties
(model/range/depth/subsampling) are OK and control what is actually
used, but your hard line is that the kernel must do an intersection
between the sink EDID and the encoder/connector capabilities to filter
the list to what it believes to be achievable?

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Tue, 16 Sept 2025 at 14:11, Daniel Stone <daniel@fooishbar.org> wrote:
>
> On Tue, 16 Sept 2025 at 11:57, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > On Tue, Sep 16, 2025 at 11:48:39AM +0100, Daniel Stone wrote:
> > > So yeah, I see it as the same as the input situation: you _can_ do the
> > > basics with raw evdev, but unless you're very special, you should use
> > > libinput. Equally for output, when you go past what e.g. Plymouth
> > > would require, use libdisplay-info to parse the EDID, rather than
> > > trying to make the kernel try to turn the unhinged madness of EDID
> > > into something userspace can reason about.
> >
> > We do a lot of EDID parsing in the kernel, including HDMI VSDB and
> > Y420CMDB parsing. Do we need anything else for this feature?
>
> I'm slightly confused as to what you're saying here. Are you saying
> that it's OK for the kernel to expose connector properties for
> userspace to see which colour properties
> (model/range/depth/subsampling) are OK and control what is actually
> used, but your hard line is that the kernel must do an intersection
> between the sink EDID and the encoder/connector capabilities to filter
> the list to what it believes to be achievable?

Yes. I'm sorry if I was not explicit enough. I'm fine with the idea of
the color format property (not just for HDMI, DP will need it too).
But I think the kernel should not be exposing 4:2:0 there if it
detects that the monitor can't support 4:2:0 at all. Likewise we
should be failing to enable 4:2:0 for a particular mode if the display
didn't list it in Y420CMDB (and we don't have e.g. a quirk of 'the
display lies, 420 is supported for this mode).


-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Daniel Stone]

Hi,

On Tue, 16 Sept 2025 at 12:15, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
> On Tue, 16 Sept 2025 at 14:11, Daniel Stone <daniel@fooishbar.org> wrote:
> > I'm slightly confused as to what you're saying here. Are you saying
> > that it's OK for the kernel to expose connector properties for
> > userspace to see which colour properties
> > (model/range/depth/subsampling) are OK and control what is actually
> > used, but your hard line is that the kernel must do an intersection
> > between the sink EDID and the encoder/connector capabilities to filter
> > the list to what it believes to be achievable?
>
> Yes. I'm sorry if I was not explicit enough. I'm fine with the idea of
> the color format property (not just for HDMI, DP will need it too).
> But I think the kernel should not be exposing 4:2:0 there if it
> detects that the monitor can't support 4:2:0 at all. Likewise we
> should be failing to enable 4:2:0 for a particular mode if the display
> didn't list it in Y420CMDB (and we don't have e.g. a quirk of 'the
> display lies, 420 is supported for this mode).

No problem at all, I think I was just being dense and not quite
parsing it properly. :)

I can see where you're going. There's definitely quite a bit of sense
in it - I'm just not sure about the value tradeoff, since I would
expect any userspace which is sophisticated enough to want
fine-grained control (as opposed to 'just get the splash screen up so
the user can enter their LUKS passphrase'), to be sophisticated enough
to also be parsing the EDID.

So I would still lean towards it not being worth the complexity in the
kernel to implement validation for userspace which really should know
better - and which already needs to have fallback handling for silent
as well as explicit failure, given part of the usecase is to be able
to deal with marginal signal propagation. But I'm not really against
it, so if you really want to see it then that should be fine.

Cheers,
Daniel

Re: [PATCH 3/8] drm: Add new general DRM property "color format"

[Dmitry Baryshkov]

On Tue, Sep 16, 2025 at 12:35:50PM +0100, Daniel Stone wrote:
> Hi,
> 
> On Tue, 16 Sept 2025 at 12:15, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > On Tue, 16 Sept 2025 at 14:11, Daniel Stone <daniel@fooishbar.org> wrote:
> > > I'm slightly confused as to what you're saying here. Are you saying
> > > that it's OK for the kernel to expose connector properties for
> > > userspace to see which colour properties
> > > (model/range/depth/subsampling) are OK and control what is actually
> > > used, but your hard line is that the kernel must do an intersection
> > > between the sink EDID and the encoder/connector capabilities to filter
> > > the list to what it believes to be achievable?
> >
> > Yes. I'm sorry if I was not explicit enough. I'm fine with the idea of
> > the color format property (not just for HDMI, DP will need it too).
> > But I think the kernel should not be exposing 4:2:0 there if it
> > detects that the monitor can't support 4:2:0 at all. Likewise we
> > should be failing to enable 4:2:0 for a particular mode if the display
> > didn't list it in Y420CMDB (and we don't have e.g. a quirk of 'the
> > display lies, 420 is supported for this mode).
> 
> No problem at all, I think I was just being dense and not quite
> parsing it properly. :)
> 
> I can see where you're going. There's definitely quite a bit of sense
> in it - I'm just not sure about the value tradeoff, since I would
> expect any userspace which is sophisticated enough to want
> fine-grained control (as opposed to 'just get the splash screen up so
> the user can enter their LUKS passphrase'), to be sophisticated enough
> to also be parsing the EDID.

That's fine.

> So I would still lean towards it not being worth the complexity in the
> kernel to implement validation for userspace which really should know
> better - and which already needs to have fallback handling for silent
> as well as explicit failure, given part of the usecase is to be able
> to deal with marginal signal propagation. But I'm not really against
> it, so if you really want to see it then that should be fine.

Well... I really think that we should not be exporting the values that
are going to fail. In the end it's as easy as checking what was parsed
from the HDMI VSDB and Y420CMDB.

Moreover, those checks would also fit nicely into the
hdmi_compute_config() / hdmi_compute_format() (a part of
drm_atomic_helper_connector_hdmi_check()).

-- 
With best wishes
Dmitry

Re: [PATCH 2/8] hdmi: Add HDMI_COLORSPACE_AUTO enum option

[Maxime Ripard]

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

On Fri, Sep 12, 2025 at 09:39:09PM +0300, Marius Vlad wrote:
> On Fri, Sep 12, 2025 at 05:17:56PM +0200, Maxime Ripard wrote:
> > Hi,
> Hi Maxime,
> > 
> > On Thu, Sep 11, 2025 at 04:07:33PM +0300, Marius Vlad wrote:
> > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> > > index 45b42f14a750..74fe925c69a2 100644
> > > --- a/drivers/video/hdmi.c
> > > +++ b/drivers/video/hdmi.c
> > > @@ -1031,8 +1031,8 @@ static const char *hdmi_colorspace_get_name(enum hdmi_colorspace colorspace)
> > >  		return "YCbCr 4:4:4";
> > >  	case HDMI_COLORSPACE_YUV420:
> > >  		return "YCbCr 4:2:0";
> > > -	case HDMI_COLORSPACE_RESERVED4:
> > > -		return "Reserved (4)";
> > > +	case HDMI_COLORSPACE_AUTO:
> > > +		return "Auto";
> > >  	case HDMI_COLORSPACE_RESERVED5:
> > >  		return "Reserved (5)";
> > >  	case HDMI_COLORSPACE_RESERVED6:
> > > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> > > index 96bda41d9148..045402033763 100644
> > > --- a/include/linux/hdmi.h
> > > +++ b/include/linux/hdmi.h
> > > @@ -82,7 +82,7 @@ enum hdmi_colorspace {
> > >  	HDMI_COLORSPACE_YUV422,
> > >  	HDMI_COLORSPACE_YUV444,
> > >  	HDMI_COLORSPACE_YUV420,
> > > -	HDMI_COLORSPACE_RESERVED4,
> > > +	HDMI_COLORSPACE_AUTO,
> > >  	HDMI_COLORSPACE_RESERVED5,
> > >  	HDMI_COLORSPACE_RESERVED6,
> > >  	HDMI_COLORSPACE_IDO_DEFINED,
> > 
> > I'm not sure we can use hdmi_colorspace as is. This is the enum that
> > represents the colorspace encoded in the AVI infoframe, so we can't
> > change it, really.
>
> I see. Was hoping I can re-use these as is when defining the color
> format enum.

You probably shouldn't, but it's not a big deal either. The values of
the property don't have to match anything.

> Should I just de-couple the color format entirely from the enum hdmi
> colorspace? With the enum color format in it feels they sort of overlap,
> with some drivers explicitly using the hdmi colorspace enum others the
> color format enum. Feels a bit inconsistent but maybe that's just me
> and folks do not see this as an issue.
> 
> Perhaps just handle 'Auto' distinctly but still re-use the hdmi
> colorspace enum? 
> 
> Any (strong) preference?

I think you can just create an enum for that property. We will use it to
fill drm_connector_hdmi_state.output_format anyway, and it will always
be something !auto in there so it would work just fine.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 4/8] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Cristian Ciocaltea]

On 2025/09/12, Maxime Ripard wrote:
> On Thu, Sep 11, 2025 at 04:07:35PM +0300, Marius Vlad wrote:
> > This would please the compiler to have a enum transformation from one to
> > another even though the values are the same. It should also make things
> > obvious that we use different enums.
> >
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  .../gpu/drm/display/drm_hdmi_state_helper.c    |  4 +++-
> >  drivers/gpu/drm/drm_connector.c                | 18 ++++++++++++++++++
> >  include/drm/drm_connector.h                    |  3 +++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > index f9aa922d25d3..dc2f32651cb9 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > @@ -653,9 +653,11 @@ hdmi_compute_config(const struct drm_connector *connector,
> >  				       conn_state->max_bpc,
> >  				       8, connector->max_bpc);
> >  	int ret;
> > +	enum hdmi_colorspace hdmi_colorspace =
> > +		color_format_to_hdmi_colorspace(conn_state->color_format);
> >
> >  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> > -				      HDMI_COLORSPACE_RGB);
> > +				      hdmi_colorspace);
> >  	if (ret) {
> >  		if (connector->ycbcr_420_allowed) {
> >  			ret = hdmi_compute_format_bpc(connector, conn_state,
>
> Does it really make sense to have the fallback to YUV420 here? I would
> expect to get rid of it, or only use that for "auto".

I think we should keep both RGB and YUV420 fallbacks, obviously making
them conditional on the requested color format to avoid redundant
checks.  E.g. if user-space has a preference on YUV444 which ends up not
being supported, switch to RGB, unless we're on a YUV420-only mode.

This would also require a reverse conversion from the resulting HDMI
colorspace stored in conn_state->hdmi.output_format and update
conn_state->color_format accordingly, so that all involved parties will
employ the actual color format rather than the user-preferred one.

Re: [PATCH 8/8] drm/rockchip: Implement "color format" DRM property

[Cristian Ciocaltea]

On 2025/09/11, Marius Vlad wrote:
> From: Derek Foreman <derek.foreman@collabora.com>
>
> This adds YUV444 and Auto, which will fallback to RGB as per
> commit "drm: Pass supported color formats straight onto drm_bridge".
>
> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> ---
>  .../gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 10 +++-
>  drivers/gpu/drm/rockchip/inno_hdmi.c          |  8 ++++
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c        |  8 ++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 46 +++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.h  |  2 +
>  5 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 58e24669ef34..794b8de9c9c5 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -427,6 +427,11 @@ static const struct of_device_id dw_hdmi_qp_rockchip_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dw_hdmi_qp_rockchip_dt_ids);
>
> +static const u32 supported_colorformats =
> +       DRM_COLOR_FORMAT_AUTO |
> +       DRM_COLOR_FORMAT_RGB444 |
> +       DRM_COLOR_FORMAT_YCBCR444;
> +
>  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  				    void *data)
>  {
> @@ -563,13 +568,16 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>
> -	connector = drm_bridge_connector_init(drm, encoder, BIT(HDMI_COLORSPACE_RGB));
> +	connector = drm_bridge_connector_init(drm, encoder, supported_colorformats);

The supported formats set to BIT(HDMI_COLORSPACE_RGB) initially as part
of drm_bridge_connector_init() and further moved out and passed as an
argument via "[PATCH 5/8] drm: Pass supported color formats straight
onto drm_bridge" is nothing but a default value to be used later on
drmm_connector_hdmi_init() invocation if the HDMI bridge in the display
pipeline (currently the framework allows only one bridge in a chain to
set DRM_BRIDGE_OP_HDMI) does not provide it's own supported_formats
before it gets attached to the encoder's chain.  Otherwise it will be
simply overridden:

    if (bridge->supported_formats)
        supported_formats = bridge->supported_formats;

This is precisely the case with [1] handling platform supported formats
and color depth in DW HDMI QP library.

>  	if (IS_ERR(connector)) {
>  		ret = PTR_ERR(connector);
>  		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>  		return ret;
>  	}
>
> +	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
> +		drm_connector_attach_color_format_property(connector);
> +
>  	return drm_connector_attach_encoder(connector, encoder);
>  }

[...]

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 186f6452a7d3..5634e59a6412 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1543,6 +1543,50 @@ static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
>  				DITHER_DOWN_ALLEGRO);
>  }
>
> +static void vop2_bcsh_config(struct drm_crtc *crtc, struct vop2_video_port *vp)
> +{
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +	u32 format = 0;
> +	enum drm_colorspace colorspace = 0;
> +	u32 val = 0;
> +
> +	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (!(crtc->state->connector_mask & drm_connector_mask(connector)))
> +			continue;
> +
> +		format = connector->state->color_format;
> +		colorspace = connector->state->colorspace;
> +		break;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	if (format == DRM_COLOR_FORMAT_YCBCR420 ||
> +	    format == DRM_COLOR_FORMAT_YCBCR444 ||
> +	    format == DRM_COLOR_FORMAT_YCBCR422) {
> +		val = RK3568_VP_BCSH_CTRL__BCSH_R2Y_EN | BIT(7);
> +
> +		switch (colorspace) {
> +		case DRM_MODE_COLORIMETRY_BT2020_RGB:
> +		case DRM_MODE_COLORIMETRY_BT2020_YCC:
> +			val |= BIT(7) | BIT(6);
> +			break;
> +		case DRM_MODE_COLORIMETRY_BT709_YCC:
> +			val |= BIT(6);
> +			break;
> +		default:
> +			break;
> +		}
> +		if (colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB ||
> +		    colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC)

While working on YUV420 output format support [2] I noticed VOP2 relies
on v4l2_colorspace instead of drm_colorspace, presumably that's just a
downstream inheritance.  Moreover, it also defines a custom
vop_csc_format and provides the vop2_convert_csc_mode() helper to translate
the V4L2_COLORSPACE_* entries into the internal CSC_* representation, as
required for the actual HW programming.

I implemented the rockchip_drm_colorimetry_to_v4l_colorspace() utility
in [3] to convert drm_colorspace to v4l2_colorspace as a temporary
workaround to pass conn_state->colorspace via
dw_hdmi_qp_rockchip_encoder_atomic_check() in [2].

I think we should try first to clean this up before adding even more
colorspace related mess, i.e. if possible get rid of v4l2_colorspace and
rely exclusively on drm_colorspace while providing a conversion helper
to the internal vop_csc_format.

Alternatively, we could keep the rockchip changes in this series to the
bare minimum (i.e. RGB-only) and handle YUV separately, to avoid adding
here unnecessary and/or unrelated complexity.  It's worth noting the 
indicated YUV420 related work has been done on top of the high color 
depth support series [1] for the required platform support, while in 
turn it conflicts with the HDMI CEC series [4], hence added as a 
dependency.

[1] https://lore.kernel.org/all/20250825-rk3588-10bpc-v2-2-955622d16985@collabora.com/
[2] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commit/6d78f42a5b88cf83cff771f7cccc44c8d67b9695
[3] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commit/9ce7c0c95ce024ee5799a3e5f776c98609683090
[4] https://lore.kernel.org/all/20250903-rk3588-hdmi-cec-v4-0-fa25163c4b08@collabora.com/

Re: [PATCH 4/8] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Maxime Ripard]

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

On Fri, Sep 26, 2025 at 05:50:43PM +0300, Cristian Ciocaltea wrote:
> On 2025/09/12, Maxime Ripard wrote:
> > On Thu, Sep 11, 2025 at 04:07:35PM +0300, Marius Vlad wrote:
> > > This would please the compiler to have a enum transformation from one to
> > > another even though the values are the same. It should also make things
> > > obvious that we use different enums.
> > >
> > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > ---
> > >  .../gpu/drm/display/drm_hdmi_state_helper.c    |  4 +++-
> > >  drivers/gpu/drm/drm_connector.c                | 18 ++++++++++++++++++
> > >  include/drm/drm_connector.h                    |  3 +++
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > index f9aa922d25d3..dc2f32651cb9 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > @@ -653,9 +653,11 @@ hdmi_compute_config(const struct drm_connector *connector,
> > >  				       conn_state->max_bpc,
> > >  				       8, connector->max_bpc);
> > >  	int ret;
> > > +	enum hdmi_colorspace hdmi_colorspace =
> > > +		color_format_to_hdmi_colorspace(conn_state->color_format);
> > >
> > >  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> > > -				      HDMI_COLORSPACE_RGB);
> > > +				      hdmi_colorspace);
> > >  	if (ret) {
> > >  		if (connector->ycbcr_420_allowed) {
> > >  			ret = hdmi_compute_format_bpc(connector, conn_state,
> >
> > Does it really make sense to have the fallback to YUV420 here? I would
> > expect to get rid of it, or only use that for "auto".
> 
> I think we should keep both RGB and YUV420 fallbacks, obviously making
> them conditional on the requested color format to avoid redundant
> checks.  E.g. if user-space has a preference on YUV444 which ends up not
> being supported, switch to RGB, unless we're on a YUV420-only mode.

I think some other part of that thread settled on if userspace
explicitly asks for YUV444 and it can't work on the
encoder/bridge/connector/monitor chain, the commit should be rejected,
and it's up to userspace to pick a reasonable fallback.

We've been asked something the kernel can't do, we should report it as
such and not fallback to something transparent to userspace.

For auto though, we should definitely keep the fallback from RGB to
YUV420.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]