[PATCH v4 00/10] Add new general DRM property "color format"

[PATCH v4 00/10] Add new general DRM property "color format"

[Nicolas Frattaroli]

Hello,

this is a follow-up to
https://lore.kernel.org/all/20250911130739.4936-1-marius.vlad@collabora.com/
which in of itself is a follow-up to
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.

That in turn was actually also a follow-up from Werner Sembach's posted at
https://lore.kernel.org/dri-devel/20210630151018.330354-1-wse@tuxedocomputers.com/

As the number of cooks have reached critical mass, I'm hoping I'll be
the last person to touch this particular series.

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 (HDMI, DP), amdgpu (HDMI, DP) and on rockchip
(HDMI).

You can also manually test this with modetest like so, but beware that
this is a non-atomic invocation, so testing YUV420 like this will result
in weird outcomes if only some of the modes support YUV420:

  $ 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.

General notes on the approach taken by me: instead of silently switching
to a different format than was explicitly requested, or even worse,
outputting something to the sink the sink doesn't support, bubble up an
error to userspace instead. "color format" is a "I want this" type
property, not a "force this" type property, i.e. the kernel will respect
the limits imposed by the hardware.

I'm not sure if my drm_bridge change actually achieves what I want in a
more complex bridge setup. I'd need to either come up with a virtual
bridge to test these scenarios, or spend some time making a flat flex
cable adapter for the DSI-HDMI bridge board I have here. Before I invest
too much time into either of those, I'd like to get some feedback on
this approach however.

Things I've tested:
- HDMI (YCbCr 4:4:4 + RGB + Auto) on RK3588
- HDMI + DP (YCbCr 4:4:4, YCbCr 4:2:0, RGB, Auto) on Intel N97 (i915)
  DP-MST is untested, but I expect it to work the same.
- HDMI (YCbCr 4:4:4, YCbCr 4:2:2, YCbCr 4:2:0, RGB, Auto) + DP (YCbCr
  4:4:4, RGB, Auto) on an AMD Radeon RX 550 (amdgpu). DP-MST is
  untested.

Changes in v4:
- Rebase onto next-20251117
- Get rid of HDMI_COLORSPACE_AUTO
- Split hdmi_compute_config change into separate patch
- Add missing symbol export for color_format_to_hdmi_colorspace to fix
  builds in certain configurations
- Drop "drm: Pass supported color formats straight onto drm_bridge"
- Make dw-hdmi-qp set the platform data's supported color formats as
  the bridge's supported HDMI color formats
- drm_hdmi_state_helper: pass requested color format to
  hdmi_compute_format_bpc if set.
- drm_bridge: limit the bus formats to those explicitly requested with
  the color format property during the atomic bridge check call,
  specifically in drm_atomic_bridge_chain_select_bus_fmts.
- i915: Remove INTEL_OUTPUT_FORMAT_AUTO, as automatic format selection
  does not need to involve the hardware state
- i915: Deduplicate ntel_output_format_to_drm_color_format code by
  moving it as a static inline __pure function into a shared header
- i915: rework logic in HDMI, DP and DP-MST output config functions to
  remove redundant locals, simplify execution flow, and return an error
  to userspace if an explicit color_format request can't be satisfied.
- i915: assign myself as the author and make the others Co-developers,
  so that they don't get the blame for any of my bugs.
- amdgpu: refactor fill_stream_properties_from_drm_display_mode to
  improve readability and ensure that impossible color format requests
  get bubbled up to userspace as errors
- amdgpu: don't pick YUV444 over RGB.
- amdgpu: assign authorship to myself, with others as Co-developers, as
  logic was modified and the blame should fall on me
- dw_hdmi_qp-rockchip: set the supported color formats platform data
  member
- rockchip: remove drm property registration for rk3066_hdmi and
  inno_hdmi. None of the platforms that use these use vop2 as the
  video output processor.
- Link to v3: https://lore.kernel.org/all/20250911130739.4936-1-marius.vlad@collabora.com/

Changes in v3 by mvlad compared to Andri's v2 series:
- 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
- Link to v2: https://lore.kernel.org/dri-devel/20240115160554.720247-1-andri@yngvason.is/

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Andri Yngvason (1):
      drm: Add new general DRM property "color format"

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

Marius Vlad (1):
      drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

Nicolas Frattaroli (6):
      drm/bridge: Act on the DRM color format property
      drm/bridge: dw-hdmi-qp: Set bridge supported_formats
      drm/rockchip: dw_hdmi_qp: Set supported_formats platdata
      drm/display: hdmi-state-helper: Act on color format DRM property
      drm/i915: Implement the "color format" DRM property
      drm/amdgpu: Implement "color format" DRM property

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

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |  93 ++++++++--
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    |  14 ++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       |   5 +
 drivers/gpu/drm/display/drm_hdmi_state_helper.c    |   8 +-
 drivers/gpu/drm/drm_atomic_helper.c                |   5 +
 drivers/gpu/drm/drm_atomic_uapi.c                  |   4 +
 drivers/gpu/drm/drm_bridge.c                       |  57 ++++++
 drivers/gpu/drm/drm_connector.c                    | 198 +++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_connector.c     |  19 ++
 drivers/gpu/drm/i915/display/intel_connector.h     |   2 +
 drivers/gpu/drm/i915/display/intel_display_types.h |  15 ++
 drivers/gpu/drm/i915/display/intel_dp.c            |  42 ++++-
 drivers/gpu/drm/i915/display/intel_dp.h            |   4 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c        |  37 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c          |  40 ++++-
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c     |  27 +++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c       |  46 +++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h       |   2 +
 include/drm/drm_connector.h                        |  57 +++++-
 19 files changed, 637 insertions(+), 38 deletions(-)
---
base-commit: d1d18879e01e4c9efcb85a96d188a8e4326136dd
change-id: 20251028-color-format-49fd202b7183

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v4 01/10] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check

[Nicolas Frattaroli]

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 e6728fd12eeb..598dc3ce993c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6522,12 +6522,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_YCBCR422)
 			&& aconnector

-- 
2.51.2

[PATCH v4 02/10] drm: Add new general DRM property "color format"

[Nicolas Frattaroli]

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     | 180 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h         |  54 ++++++++++-
 4 files changed, 238 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e641fcf8c568..25f354b2cc0d 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 b2cb5ae5a139..2f093b0d1628 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -784,6 +784,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);
@@ -869,6 +871,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 272d6254ea47..0ad7be0a2d09 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
@@ -2628,6 +2693,101 @@ 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;
+		if (i != DRM_MODE_COLOR_FORMAT_AUTO)
+			enum_list[len].name = output_format_str[i];
+		else
+			enum_list[len].name = "AUTO";
+		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.
@@ -2845,6 +3005,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..a071079fd3ad 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,
+	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.51.2

[PATCH v4 03/10] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Nicolas Frattaroli]

From: Marius Vlad <marius.vlad@collabora.com>

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>
---
 drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++++
 include/drm/drm_connector.h     |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 0ad7be0a2d09..61c077b67ac3 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:
+	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;
+	}
+}
+EXPORT_SYMBOL(color_format_to_hdmi_colorspace);
+
 /**
  * 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 a071079fd3ad..e044976c8d76 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.51.2

[PATCH v4 04/10] drm/bridge: Act on the DRM color format property

[Nicolas Frattaroli]

The new DRM color format property allows userspace to request a specific
color format on a connector. In turn, this fills the connector state's
color_format member to switch color formats.

Make drm_bridges consider the color_format set in the connector state
during the atomic bridge check. Specifically, reject any output bus
formats that do not correspond to the requested color format.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/drm_bridge.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 8f355df883d8..b7df5cbad832 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1052,6 +1052,59 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
 	return ret;
 }
 
+static bool __pure bus_format_is_color_fmt(u32 bus_fmt, enum drm_color_format fmt)
+{
+	if (bus_fmt == MEDIA_BUS_FMT_FIXED)
+		return true;
+
+	switch (fmt) {
+	case DRM_COLOR_FORMAT_NONE:
+	case DRM_COLOR_FORMAT_AUTO:
+		return true;
+	case DRM_COLOR_FORMAT_RGB444:
+		switch (bus_fmt) {
+		case MEDIA_BUS_FMT_RGB888_1X24:
+		case MEDIA_BUS_FMT_RGB101010_1X30:
+		case MEDIA_BUS_FMT_RGB121212_1X36:
+		case MEDIA_BUS_FMT_RGB161616_1X48:
+			return true;
+		default:
+			return false;
+		}
+	case DRM_COLOR_FORMAT_YCBCR444:
+		switch (bus_fmt) {
+		case MEDIA_BUS_FMT_YUV8_1X24:
+		case MEDIA_BUS_FMT_YUV10_1X30:
+		case MEDIA_BUS_FMT_YUV12_1X36:
+		case MEDIA_BUS_FMT_YUV16_1X48:
+			return true;
+		default:
+			return false;
+		}
+	case DRM_COLOR_FORMAT_YCBCR422:
+		switch (bus_fmt) {
+		case MEDIA_BUS_FMT_UYVY8_1X16:
+		case MEDIA_BUS_FMT_UYVY10_1X20:
+		case MEDIA_BUS_FMT_UYVY12_1X24:
+			return true;
+		default:
+			return false;
+		}
+	case DRM_COLOR_FORMAT_YCBCR420:
+		switch (bus_fmt) {
+		case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
+		case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
+		case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
+		case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
+			return true;
+		default:
+			return false;
+		}
+	}
+
+	return false;
+}
+
 /*
  * This function is called by &drm_atomic_bridge_chain_check() just before
  * calling &drm_bridge_funcs.atomic_check() on all elements of the chain.
@@ -1137,6 +1190,10 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
 	}
 
 	for (i = 0; i < num_out_bus_fmts; i++) {
+		if (!bus_format_is_color_fmt(out_bus_fmts[i], conn_state->color_format)) {
+			ret = -ENOTSUPP;
+			continue;
+		}
 		ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
 					       conn_state, out_bus_fmts[i]);
 		if (ret != -ENOTSUPP)

-- 
2.51.2

[PATCH v4 05/10] drm/bridge: dw-hdmi-qp: Set bridge supported_formats

[Nicolas Frattaroli]

The drm_bridge "supported_formats" member stores a bitmask of supported
HDMI output formats if the bridge is in fact an HDMI bridge.

However, until now, the synopsys dw-hdmi-qp driver did not set this
member in the bridge it creates.

Set it based on the platform data's supported_formats member, and
default to BIT(HDMI_COLORSPACE_RGB) if it's absent, which preserves the
previous behaviour.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index fe4c026280f0..cf888236bd65 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -1269,6 +1269,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 		dev_warn(dev, "Set ref_clk_rate to vendor default\n");
 	}
 
+	if (plat_data->supported_formats)
+		hdmi->bridge.supported_formats = plat_data->supported_formats;
+	else
+		hdmi->bridge.supported_formats = BIT(HDMI_COLORSPACE_RGB);
+
 	dw_hdmi_qp_init_hw(hdmi);
 
 	ret = devm_request_threaded_irq(dev, plat_data->main_irq,

-- 
2.51.2

[PATCH v4 06/10] drm/rockchip: dw_hdmi_qp: Set supported_formats platdata

[Nicolas Frattaroli]

With the introduction of the supported_formats member in the
dw-hdmi-qp platform data struct, drivers that have access to this
information should now set it.

Set it in the rockchip dw_hdmi_qp glue driver, where such a bitmask of
supported color formats already exists. It just needs to be converted to
the appropriate HDMI_COLORSPACE_ mask.

This allows this information to be passed down to the dw-hdmi-qp core,
which sets it in the bridge it creates, and consequently will allow the
common HDMI bridge code to act on it.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index c9fe6aa3e3e3..7c294751de19 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -468,6 +468,28 @@ 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 unsigned int __pure drm_to_hdmi_fmts(const u32 fmt)
+{
+	unsigned int res = 0;
+
+	if (fmt & DRM_COLOR_FORMAT_AUTO)
+		res |= BIT(HDMI_COLORSPACE_RGB);
+	if (fmt & DRM_COLOR_FORMAT_RGB444)
+		res |= BIT(HDMI_COLORSPACE_RGB);
+	if (fmt & DRM_COLOR_FORMAT_YCBCR444)
+		res |= BIT(HDMI_COLORSPACE_YUV444);
+	if (fmt & DRM_COLOR_FORMAT_YCBCR422)
+		res |= BIT(HDMI_COLORSPACE_YUV422);
+	if (fmt & DRM_COLOR_FORMAT_YCBCR420)
+		res |= BIT(HDMI_COLORSPACE_YUV420);
+
+	return res;
+}
+
 static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 				    void *data)
 {
@@ -521,6 +543,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	plat_data.phy_data = hdmi;
 	plat_data.max_bpc = 10;
 
+	plat_data.supported_formats = drm_to_hdmi_fmts(supported_colorformats);
+
 	encoder = &hdmi->encoder.encoder;
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 

-- 
2.51.2

[PATCH v4 07/10] drm/display: hdmi-state-helper: Act on color format DRM property

[Nicolas Frattaroli]

With the introduction of the "color format" DRM property, which allows
userspace to request a specific color format, the HDMI state helper
should implement this.

Implement it by checking whether the property is set and set to
something other than auto. If so, pass the requested color format, and
otherwise set RGB.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 8 +++++++-
 1 file changed, 7 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 a561f124be99..add0d51fce33 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -650,9 +650,15 @@ hdmi_compute_config(const struct drm_connector *connector,
 				       conn_state->max_bpc,
 				       8, connector->max_bpc);
 	int ret;
+	enum hdmi_colorspace hdmi_colorspace;
+
+	if (conn_state->color_format && conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
+		hdmi_colorspace = color_format_to_hdmi_colorspace(conn_state->color_format);
+	else
+		hdmi_colorspace = HDMI_COLORSPACE_RGB;
 
 	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,

-- 
2.51.2

[PATCH v4 08/10] drm/i915: Implement the "color format" DRM property

[Nicolas Frattaroli]

This includes RGB, YUV420, YUV444 and Auto. Auto will pick RGB, unless
the mode being asked for is YUV420-only, in which case it picks YUV420,
with a fallback for RGB in place just as a last-ditch attempt.

Should the explicitly requested color format not be supported by the
sink, then an error is returned to userspace, so that it can make a
better choice.

Co-developed-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Co-developed-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Co-developed-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/i915/display/intel_connector.c     | 19 ++++++++++
 drivers/gpu/drm/i915/display/intel_connector.h     |  2 ++
 drivers/gpu/drm/i915/display/intel_display_types.h | 15 ++++++++
 drivers/gpu/drm/i915/display/intel_dp.c            | 42 +++++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_dp.h            |  4 +++
 drivers/gpu/drm/i915/display/intel_dp_mst.c        | 37 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_hdmi.c          | 40 ++++++++++++++++-----
 7 files changed, 140 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 913d90a7a508..3d563e12d3aa 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -40,6 +40,11 @@
 #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),
@@ -326,6 +331,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)
 {
@@ -333,6 +345,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 0aa86626e646..38401bc705e3 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.h
+++ b/drivers/gpu/drm/i915/display/intel_connector.h
@@ -30,7 +30,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 38702a9e0f50..b06fbe56cbbe 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -2206,6 +2206,21 @@ to_intel_frontbuffer(struct drm_framebuffer *fb)
 	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
 }
 
+static inline __pure u32
+intel_output_format_to_drm_color_format(enum intel_output_format input)
+{
+	switch (input) {
+	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 DRM_COLOR_FORMAT_NONE;
+}
+
 /*
  * Conversion functions/macros from various pointer types to struct
  * intel_display pointer.
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0ec82fcbcf48..6fcd1f2dbefd 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1183,7 +1183,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)
 {
@@ -3141,17 +3141,23 @@ 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;
+	u32 sink_format_drm;
 	int ret;
 
-	ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
+	if ((conn_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+	     drm_mode_is_420_also(info, adjusted_mode)) ||
+	    drm_mode_is_420_only(info, adjusted_mode))
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+	else if (conn_state->color_format == DRM_COLOR_FORMAT_YCBCR444)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+	else
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
 
-	if (ycbcr_420_only && !connector->base.ycbcr_420_allowed) {
+	if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 &&
+	    !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");
+			"YCbCr 4:2:0 mode requested but unsupported by connector. Trying 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);
@@ -3159,6 +3165,11 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 	ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
 					   respect_downstream_limits);
 	if (ret) {
+		/*
+		 * If no valid link config can be found due to bandwidth constraints,
+		 * degrade from RGB/YCbCr 4:4:4 to YCbCr 4:2:0 if permitted by
+		 * the source and sink.
+		 */
 		if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
 		    !connector->base.ycbcr_420_allowed ||
 		    !drm_mode_is_420_also(info, adjusted_mode))
@@ -3169,9 +3180,22 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
 								   crtc_state->sink_format);
 		ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
 						   respect_downstream_limits);
+		if (ret)
+			return ret;
 	}
 
-	return ret;
+	sink_format_drm = intel_output_format_to_drm_color_format(crtc_state->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != sink_format_drm &&
+	    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, sink_format_drm);
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 void
@@ -6626,8 +6650,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *_connec
 	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 200a8b267f64..a7492c97ac97 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -205,6 +205,10 @@ bool intel_dp_compute_config_limits(struct intel_dp *intel_dp,
 void intel_dp_get_dsc_sink_cap(u8 dpcd_rev,
 			       const struct drm_dp_desc *desc, bool is_branch,
 			       struct intel_connector *connector);
+enum intel_output_format
+intel_dp_output_format(struct intel_connector *connector,
+		       enum intel_output_format sink_format);
+
 bool intel_dp_has_gamut_metadata_dip(struct intel_encoder *encoder);
 
 bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 4c0b943fe86f..35f8db595e22 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -653,9 +653,11 @@ 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;
+	u32 sink_format_drm;
 	int ret = 0;
 
 	if (pipe_config->fec_enable &&
@@ -671,10 +673,36 @@ 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(info, adjusted_mode)) ||
+	    drm_mode_is_420_only(info, adjusted_mode))
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
+	else if (conn_state->color_format == DRM_COLOR_FORMAT_YCBCR444)
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+	else
+		pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
+
+	if (pipe_config->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 &&
+	    !connector->base.ycbcr_420_allowed) {
+		drm_dbg_kms(display->drm,
+			"YCbCr 4:2:0 mode requested but it's unsupported by connector. Trying RGB.\n");
+		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;
 
+	sink_format_drm = intel_output_format_to_drm_color_format(pipe_config->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != sink_format_drm &&
+	    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, sink_format_drm);
+		return -EINVAL;
+	}
+
 	joiner_needs_dsc = intel_dp_joiner_needs_dsc(display, num_joined_pipes);
 
 	dsc_needed = joiner_needs_dsc || intel_dp->force_dsc_en ||
@@ -1660,6 +1688,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 908faf17f93d..adecee10cb4c 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2271,21 +2271,28 @@ 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);
+	u32 sink_format_drm;
+	bool want_420;
 	int ret;
 
-	crtc_state->sink_format =
-		intel_hdmi_sink_format(crtc_state, connector, ycbcr_420_only);
-
-	if (ycbcr_420_only && 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;
+	if (conn_state->color_format == DRM_COLOR_FORMAT_YCBCR444)
+		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+	else {
+		want_420 = ((conn_state->color_format == DRM_COLOR_FORMAT_YCBCR420 &&
+			     drm_mode_is_420(info, adjusted_mode)) ||
+			     drm_mode_is_420_only(info, adjusted_mode));
+		crtc_state->sink_format = intel_hdmi_sink_format(crtc_state, connector,
+								 want_420);
 	}
 
 	crtc_state->output_format = intel_hdmi_output_format(crtc_state);
 	ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
 	if (ret) {
+		/*
+		 * If no valid link config can be found due to bandwidth constraints,
+		 * degrade from RGB/YCbCr 4:4:4 to YCbCr 4:2:0 if permitted by
+		 * the source and sink.
+		 */
 		if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
 		    !crtc_state->has_hdmi_sink ||
 		    !connector->base.ycbcr_420_allowed ||
@@ -2295,6 +2302,19 @@ static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
 		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
 		crtc_state->output_format = intel_hdmi_output_format(crtc_state);
 		ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
+		if (ret)
+			return ret;
+	}
+
+	sink_format_drm = intel_output_format_to_drm_color_format(crtc_state->sink_format);
+	if (conn_state->color_format &&
+	    conn_state->color_format != sink_format_drm &&
+	    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, sink_format_drm);
+		ret = -EINVAL;
 	}
 
 	return ret;
@@ -2690,8 +2710,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.51.2

[PATCH v4 09/10] drm/amdgpu: Implement "color format" DRM property

[Nicolas Frattaroli]

The "color format" DRM property allows userspace to explicitly pick a
color format to use. If an unsupported color format is requested,
userspace will be given an error instead of silently having its request
disobeyed.

The default case, in which the property is either unset or set to Auto,
picks YCbCr 4:2:0 if it's a 4:2:0-only mode, and RGB in all other cases.

Co-developed-by: Werner Sembach <wse@tuxedocomputers.com>
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Co-developed-by: Andri Yngvason <andri@yngvason.is>
Signed-off-by: Andri Yngvason <andri@yngvason.is>
Co-developed-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 89 +++++++++++++++++++---
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 14 ++++
 2 files changed, 94 insertions(+), 9 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 598dc3ce993c..956dd4934fa6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6504,11 +6504,14 @@ static void fill_stream_properties_from_drm_display_mode(
 	const struct dc_stream_state *old_stream,
 	int requested_bpc)
 {
+	bool is_dp_or_hdmi = dc_is_hdmi_signal(stream->signal) || dc_is_dp_signal(stream->signal);
 	struct dc_crtc_timing *timing_out = &stream->timing;
 	const struct drm_display_info *info = &connector->display_info;
 	struct amdgpu_dm_connector *aconnector = NULL;
 	struct hdmi_vendor_infoframe hv_frame;
 	struct hdmi_avi_infoframe avi_frame;
+	bool want_420;
+	bool want_422;
 	ssize_t err;
 
 	if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
@@ -6521,19 +6524,38 @@ 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))
+
+	want_420 = (aconnector && aconnector->force_yuv420_output) ||
+		   (connector_state->color_format == DRM_COLOR_FORMAT_YCBCR420);
+	want_422 = (aconnector && aconnector->force_yuv422_output) ||
+		   (connector_state->color_format == DRM_COLOR_FORMAT_YCBCR422);
+
+	if (drm_mode_is_420(info, mode_in) && want_420)
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
-	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR422)
-			&& aconnector
-			&& aconnector->force_yuv422_output)
+	else if ((info->color_formats & DRM_COLOR_FORMAT_YCBCR422) && want_422 && is_dp_or_hdmi)
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR422;
-	else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
-			&& stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+	else if (connector_state->color_format == DRM_COLOR_FORMAT_YCBCR444 &&
+		 (info->color_formats & DRM_COLOR_FORMAT_YCBCR444) && is_dp_or_hdmi)
 		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
-	else
+	else if ((connector_state->color_format == DRM_COLOR_FORMAT_RGB444 ||
+		  connector_state->color_format == DRM_COLOR_FORMAT_AUTO) &&
+		 !drm_mode_is_420_only(info, mode_in))
 		timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+	else {
+		/*
+		 * If a format was explicitly requested but the requested format
+		 * can't be satisfied, set it to an invalid value so that an
+		 * error bubbles up to userspace. This way, userspace knows it
+		 * needs to make a better choice.
+		 */
+		if (connector_state->color_format &&
+			connector_state->color_format != DRM_COLOR_FORMAT_AUTO)
+			timing_out->pixel_encoding = PIXEL_ENCODING_UNDEFINED;
+		else if (drm_mode_is_420_only(info, mode_in))
+			timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+		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(
@@ -7874,6 +7896,39 @@ 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)
+{
+	enum dc_pixel_encoding encoding;
+
+	if (!drm_state->color_format ||
+	    drm_state->color_format == DRM_COLOR_FORMAT_NONE)
+		return DC_OK;
+
+	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:
+		encoding = PIXEL_ENCODING_UNDEFINED;
+		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,
@@ -7920,6 +7975,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,
@@ -8735,6 +8793,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,
@@ -8827,10 +8892,16 @@ 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 5e92eaa67aa3..7d35779c99d5 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,13 @@ 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.51.2

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

[Nicolas Frattaroli]

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

Register the color format property in the dw_hdmi_qp-rockchip driver,
and act on requested format changes as part of the connector state in
the vop2 video output driver.

Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c |  3 ++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c   | 46 ++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h   |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 7c294751de19..7028166fdace 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -635,6 +635,9 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
+	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 498df0ce4680..2fc9b21c5522 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1549,6 +1549,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);
@@ -1600,6 +1644,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 9124191899ba..33fdc9d8d819 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -637,6 +637,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.51.2

Re: [PATCH v4 02/10] drm: Add new general DRM property "color format"

[kernel test robot]

Hi Nicolas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d1d18879e01e4c9efcb85a96d188a8e4326136dd]

url:    https://github.com/intel-lab-lkp/linux/commits/Nicolas-Frattaroli/drm-amd-display-Remove-unnecessary-SIGNAL_TYPE_HDMI_TYPE_A-check/20251118-031440
base:   d1d18879e01e4c9efcb85a96d188a8e4326136dd
patch link:    https://lore.kernel.org/r/20251117-color-format-v4-2-0ded72bd1b00%40collabora.com
patch subject: [PATCH v4 02/10] drm: Add new general DRM property "color format"
config: arm-randconfig-001-20251118 (https://download.01.org/0day-ci/archive/20251118/202511181958.eS8Ctiow-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251118/202511181958.eS8Ctiow-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511181958.eS8Ctiow-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/gpu/drm/drm_connector.c:1392 function parameter 'color_fmt' not described in 'drm_get_color_format_name'
>> Warning: drivers/gpu/drm/drm_connector.c:1392 function parameter 'color_fmt' not described in 'drm_get_color_format_name'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 05/10] drm/bridge: dw-hdmi-qp: Set bridge supported_formats

[Cristian Ciocaltea]

Hi Nicolas,

On 11/17/25 9:11 PM, Nicolas Frattaroli wrote:
> The drm_bridge "supported_formats" member stores a bitmask of supported
> HDMI output formats if the bridge is in fact an HDMI bridge.
> 
> However, until now, the synopsys dw-hdmi-qp driver did not set this
> member in the bridge it creates.
> 
> Set it based on the platform data's supported_formats member, and
> default to BIT(HDMI_COLORSPACE_RGB) if it's absent, which preserves the
> previous behaviour.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index fe4c026280f0..cf888236bd65 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> @@ -1269,6 +1269,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>  		dev_warn(dev, "Set ref_clk_rate to vendor default\n");
>  	}
>  
> +	if (plat_data->supported_formats)
> +		hdmi->bridge.supported_formats = plat_data->supported_formats;

This duplicates a change already introduced via commit 1ff27c5929ab
("drm/bridge: dw-hdmi-qp: Handle platform supported formats and color depth").

> +	else
> +		hdmi->bridge.supported_formats = BIT(HDMI_COLORSPACE_RGB);

And this one looks redundant as well, since RGB is supposed to be mandatory:
supported_formats defaults to RGB in drm_bridge_connector_init() if there's no
HDMI bridge in the pipeline overriding it, while drmm_connector_hdmi_init()
errors out if supported_formats is unset or doesn't advertise RGB.

Hence I think this patch can be dropped.

Regards,
Cristian

Re: [PATCH v4 06/10] drm/rockchip: dw_hdmi_qp: Set supported_formats platdata

[Cristian Ciocaltea]

On 11/17/25 9:11 PM, Nicolas Frattaroli wrote:
> With the introduction of the supported_formats member in the
> dw-hdmi-qp platform data struct, drivers that have access to this
> information should now set it.
> 
> Set it in the rockchip dw_hdmi_qp glue driver, where such a bitmask of
> supported color formats already exists. It just needs to be converted to
> the appropriate HDMI_COLORSPACE_ mask.
> 
> This allows this information to be passed down to the dw-hdmi-qp core,
> which sets it in the bridge it creates, and consequently will allow the
> common HDMI bridge code to act on it.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index c9fe6aa3e3e3..7c294751de19 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -468,6 +468,28 @@ 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 unsigned int __pure drm_to_hdmi_fmts(const u32 fmt)
> +{
> +	unsigned int res = 0;
> +
> +	if (fmt & DRM_COLOR_FORMAT_AUTO)
> +		res |= BIT(HDMI_COLORSPACE_RGB);
> +	if (fmt & DRM_COLOR_FORMAT_RGB444)
> +		res |= BIT(HDMI_COLORSPACE_RGB);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR444)
> +		res |= BIT(HDMI_COLORSPACE_YUV444);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR422)
> +		res |= BIT(HDMI_COLORSPACE_YUV422);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR420)
> +		res |= BIT(HDMI_COLORSPACE_YUV420);
> +
> +	return res;
> +}
> +
>  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  				    void *data)
>  {
> @@ -521,6 +543,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	plat_data.phy_data = hdmi;
>  	plat_data.max_bpc = 10;
>  
> +	plat_data.supported_formats = drm_to_hdmi_fmts(supported_colorformats);

Any reason why this cannot be simply set as

  BIT(HDMI_COLORSPACE_RGB) | BIT(HDMI_COLORSPACE_YUV444) | BIT(HDMI_COLORSPACE_YUV422)

and get rid of the unnecessary conversion?

> +
>  	encoder = &hdmi->encoder.encoder;
>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
>  
> 

Re: [PATCH v4 02/10] drm: Add new general DRM property "color format"

[Laurent Pinchart]

On Mon, Nov 17, 2025 at 08:11:46PM +0100, Nicolas Frattaroli wrote:
> From: Andri Yngvason <andri@yngvason.is>
> 
> Adds a new general DRM property named "color format" which can be used by

s/Adds/Add/

> 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     | 180 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         |  54 ++++++++++-
>  4 files changed, 238 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e641fcf8c568..25f354b2cc0d 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 b2cb5ae5a139..2f093b0d1628 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -784,6 +784,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);
> @@ -869,6 +871,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 272d6254ea47..0ad7be0a2d09 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;
> +	}

It seems this could be simplified to

	return BIT(fmt_enum);

The above function could also be simplified by using ffs().

> +}
> +
> +/**
> + * drm_get_color_format_name - return a string for color format
> + * @colorspace: color format to compute name of

s/colorspace/color_fmt/

Please make sure to compile the documentation and check for warnings in
the next iteration.

> + *
> + */
> +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);

Could we flip that and make drm_hdmi_connector_get_output_format_name()
a wrapper around 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
> @@ -2628,6 +2693,101 @@ 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;

Neither can be negative, you can use unsigned int.

> +
> +	if (connector->color_format_property)
> +		return 0;
> +
> +	if (!supported_color_formats) {
> +		drm_err(dev, "No supported color formats provded on [CONNECTOR:%d:%s]\n",

s/provded/provided/

> +			    connector->base.id, connector->name);

Incorrect alignment.

> +		return -EINVAL;
> +	}
> +
> +	if ((supported_color_formats & -BIT(DRM_MODE_COLOR_FORMAT_COUNT)) != 0) {

GENMASK(DRM_MODE_COLOR_FORMAT_COUNT - 1, 0)

would be clearer than

-BIT(DRM_MODE_COLOR_FORMAT_COUNT)

> +		drm_err(dev, "Unknown colorspace provded on [CONNECTOR:%d:%s]\n",

s/colorspace/color format/
s/provded/provided/

> +			    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;
> +		if (i != DRM_MODE_COLOR_FORMAT_AUTO)
> +			enum_list[len].name = output_format_str[i];
> +		else
> +			enum_list[len].name = "AUTO";
> +		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

You don't document that supported_color_formats can be 0. I think I'd
actually drop that possibility, it's not used by drivers in this series,
and it seems to be error-prone. If a later version of HDMI or DP adds
support for more color formats, those would get automatically advertised
while not supported by drivers.

> + *
> + * 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);

I think we could simplify the API here by picking the defaults based on
the connector type:

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];
	unsigned int i, len;

	if (connector->color_format_property)
		return 0;

	switch (connector->type) {
	case DRM_MODE_CONNECTOR_HDMIA:
	case DRM_MODE_CONNECTOR_HDMIB:
		supported_color_formats &= hdmi_colorformats;
		break;

	case DRM_MODE_CONNECTOR_DisplayPort:
	case DRM_MODE_CONNECTOR_eDP:
		supported_color_formats &= dp_colorformats;
		break;

	default:
		drm_err(dev, ...);
		return -EINVAL;
	}

	if (!supported_color_formats) {
		drm_err(dev, "No supported color formats provided on [CONNECTOR:%d:%s]\n",
			connector->base.id, connector->name);
		return -EINVAL;
	}

	...
}

> +
> +/**
> + * 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.
> @@ -2845,6 +3005,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..a071079fd3ad 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 {

Please document this.

> +	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,
> +	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),
> +};

It would be nicer to have a single enum, and use
BIT(DRM_MODE_COLOR_FORMAT_*) wherever a mask is used.

> +
>  /**
>   * 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);

This holds on a single line.

> +
> +u32
> +drm_color_format_enum_to_color_format(enum drm_color_format_enum fmt_enum);

I'd avoid the line break here too.

> +
>  /**
>   * drm_for_each_connector_iter - connector_list iterator macro
>   * @connector: &struct drm_connector pointer used as cursor

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 03/10] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Laurent Pinchart]

On Mon, Nov 17, 2025 at 08:11:47PM +0100, Nicolas Frattaroli wrote:
> From: Marius Vlad <marius.vlad@collabora.com>
> 
> 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

The hdmi_colorspace enumerators are defined as (with comments added to
make the values explicit)

enum hdmi_colorspace {
	HDMI_COLORSPACE_RGB,			/* 0 */
	HDMI_COLORSPACE_YUV422,			/* 1 */
	HDMI_COLORSPACE_YUV444,			/* 2 */
	HDMI_COLORSPACE_YUV420,			/* 3 */
	HDMI_COLORSPACE_RESERVED4,		/* 4 */
	HDMI_COLORSPACE_RESERVED5,		/* 5 */
	HDMI_COLORSPACE_RESERVED6,		/* 6 */
	HDMI_COLORSPACE_IDO_DEFINED,		/* 7 */
};

and the DRM color formats as (after patch 02/10)

enum drm_color_format {
	DRM_COLOR_FORMAT_NONE			= 0,		/* 0 */
	DRM_COLOR_FORMAT_RGB444			= (1 << 0),	/* 1 */
	DRM_COLOR_FORMAT_YCBCR422		= (1 << 1),	/* 2 */
	DRM_COLOR_FORMAT_YCBCR444		= (1 << 2),	/* 4 */
	DRM_COLOR_FORMAT_YCBCR420		= (1 << 3),	/* 8 */
	/* auto case, driver will set the color_format */
	DRM_COLOR_FORMAT_AUTO			= (1 << 4),	/* 16 */
};

Contrary to what is stated in the commit message, the values are not the
same. Maybe you confused DRM_COLOR_FORMAT_* with DRM_MODE_COLOR_FORMAT_*
?

> obvious that we use different enums.
> 
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++++
>  include/drm/drm_connector.h     |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 0ad7be0a2d09..61c077b67ac3 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:
> +	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;
> +	}
> +}
> +EXPORT_SYMBOL(color_format_to_hdmi_colorspace);
> +
>  /**
>   * 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 a071079fd3ad..e044976c8d76 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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 05/10] drm/bridge: dw-hdmi-qp: Set bridge supported_formats

[Laurent Pinchart]

On Mon, Nov 17, 2025 at 08:11:49PM +0100, Nicolas Frattaroli wrote:
> The drm_bridge "supported_formats" member stores a bitmask of supported
> HDMI output formats if the bridge is in fact an HDMI bridge.

It would be nice to convert the supported_formats field to a bitmask of
DRM_MODE_COLOR_FORMAT_* values.

> 
> However, until now, the synopsys dw-hdmi-qp driver did not set this
> member in the bridge it creates.
> 
> Set it based on the platform data's supported_formats member, and
> default to BIT(HDMI_COLORSPACE_RGB) if it's absent, which preserves the
> previous behaviour.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index fe4c026280f0..cf888236bd65 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> @@ -1269,6 +1269,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>  		dev_warn(dev, "Set ref_clk_rate to vendor default\n");
>  	}
>  
> +	if (plat_data->supported_formats)
> +		hdmi->bridge.supported_formats = plat_data->supported_formats;
> +	else
> +		hdmi->bridge.supported_formats = BIT(HDMI_COLORSPACE_RGB);
> +
>  	dw_hdmi_qp_init_hw(hdmi);
>  
>  	ret = devm_request_threaded_irq(dev, plat_data->main_irq,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/10] drm/rockchip: dw_hdmi_qp: Set supported_formats platdata

[Laurent Pinchart]

On Mon, Nov 17, 2025 at 08:11:50PM +0100, Nicolas Frattaroli wrote:
> With the introduction of the supported_formats member in the
> dw-hdmi-qp platform data struct, drivers that have access to this
> information should now set it.
> 
> Set it in the rockchip dw_hdmi_qp glue driver, where such a bitmask of
> supported color formats already exists. It just needs to be converted to
> the appropriate HDMI_COLORSPACE_ mask.
> 
> This allows this information to be passed down to the dw-hdmi-qp core,
> which sets it in the bridge it creates, and consequently will allow the
> common HDMI bridge code to act on it.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index c9fe6aa3e3e3..7c294751de19 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -468,6 +468,28 @@ 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 unsigned int __pure drm_to_hdmi_fmts(const u32 fmt)
> +{
> +	unsigned int res = 0;
> +
> +	if (fmt & DRM_COLOR_FORMAT_AUTO)
> +		res |= BIT(HDMI_COLORSPACE_RGB);
> +	if (fmt & DRM_COLOR_FORMAT_RGB444)
> +		res |= BIT(HDMI_COLORSPACE_RGB);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR444)
> +		res |= BIT(HDMI_COLORSPACE_YUV444);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR422)
> +		res |= BIT(HDMI_COLORSPACE_YUV422);
> +	if (fmt & DRM_COLOR_FORMAT_YCBCR420)
> +		res |= BIT(HDMI_COLORSPACE_YUV420);
> +
> +	return res;
> +}
> +

This would be greatly simplified by turning supported_formats into a
bitmask of DRM_MODE_COLOR_FORMAT_* values, as suggested in 05/10.

>  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  				    void *data)
>  {
> @@ -521,6 +543,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	plat_data.phy_data = hdmi;
>  	plat_data.max_bpc = 10;
>  
> +	plat_data.supported_formats = drm_to_hdmi_fmts(supported_colorformats);
> +
>  	encoder = &hdmi->encoder.encoder;
>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 04/10] drm/bridge: Act on the DRM color format property

[Laurent Pinchart]

On Mon, Nov 17, 2025 at 08:11:48PM +0100, Nicolas Frattaroli wrote:
> The new DRM color format property allows userspace to request a specific
> color format on a connector. In turn, this fills the connector state's
> color_format member to switch color formats.
> 
> Make drm_bridges consider the color_format set in the connector state
> during the atomic bridge check. Specifically, reject any output bus
> formats that do not correspond to the requested color format.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 8f355df883d8..b7df5cbad832 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1052,6 +1052,59 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
>  	return ret;
>  }
>  
> +static bool __pure bus_format_is_color_fmt(u32 bus_fmt, enum drm_color_format fmt)
> +{
> +	if (bus_fmt == MEDIA_BUS_FMT_FIXED)
> +		return true;
> +
> +	switch (fmt) {
> +	case DRM_COLOR_FORMAT_NONE:
> +	case DRM_COLOR_FORMAT_AUTO:
> +		return true;
> +	case DRM_COLOR_FORMAT_RGB444:
> +		switch (bus_fmt) {
> +		case MEDIA_BUS_FMT_RGB888_1X24:
> +		case MEDIA_BUS_FMT_RGB101010_1X30:
> +		case MEDIA_BUS_FMT_RGB121212_1X36:
> +		case MEDIA_BUS_FMT_RGB161616_1X48:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	case DRM_COLOR_FORMAT_YCBCR444:
> +		switch (bus_fmt) {
> +		case MEDIA_BUS_FMT_YUV8_1X24:
> +		case MEDIA_BUS_FMT_YUV10_1X30:
> +		case MEDIA_BUS_FMT_YUV12_1X36:
> +		case MEDIA_BUS_FMT_YUV16_1X48:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	case DRM_COLOR_FORMAT_YCBCR422:
> +		switch (bus_fmt) {
> +		case MEDIA_BUS_FMT_UYVY8_1X16:
> +		case MEDIA_BUS_FMT_UYVY10_1X20:
> +		case MEDIA_BUS_FMT_UYVY12_1X24:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	case DRM_COLOR_FORMAT_YCBCR420:
> +		switch (bus_fmt) {
> +		case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> +		case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> +		case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
> +		case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	}

I'd find this more readable:

	if (fmt == DRM_COLOR_FORMAT_NONE || fmt == DRM_COLOR_FORMAT_AUTO)
		return true;

	switch (bus_fmt) {
	case MEDIA_BUS_FMT_RGB888_1X24:
	case MEDIA_BUS_FMT_RGB101010_1X30:
	case MEDIA_BUS_FMT_RGB121212_1X36:
	case MEDIA_BUS_FMT_RGB161616_1X48:
		return fmt == DRM_COLOR_FORMAT_RGB444:

	case MEDIA_BUS_FMT_YUV8_1X24:
	case MEDIA_BUS_FMT_YUV10_1X30:
	case MEDIA_BUS_FMT_YUV12_1X36:
	case MEDIA_BUS_FMT_YUV16_1X48:
		return fmt == DRM_COLOR_FORMAT_YCBCR444;

	case MEDIA_BUS_FMT_UYVY8_1X16:
	case MEDIA_BUS_FMT_UYVY10_1X20:
	case MEDIA_BUS_FMT_UYVY12_1X24:
		return fmt == DRM_COLOR_FORMAT_YCBCR422;

	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
	case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
	case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
		return fmt == DRM_COLOR_FORMAT_YCBCR420;

	default:
		return false;
	}

but it could be a matter for personal preference ?

I'm also a bit concerned about the 

	if (fmt == DRM_COLOR_FORMAT_NONE || fmt == DRM_COLOR_FORMAT_AUTO)

test. What's the difference between NONE and AUTO ? Is it meaningful, or
should the two enumerators be merged into a single one ?

> +
> +	return false;
> +}
> +
>  /*
>   * This function is called by &drm_atomic_bridge_chain_check() just before
>   * calling &drm_bridge_funcs.atomic_check() on all elements of the chain.
> @@ -1137,6 +1190,10 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
>  	}
>  
>  	for (i = 0; i < num_out_bus_fmts; i++) {
> +		if (!bus_format_is_color_fmt(out_bus_fmts[i], conn_state->color_format)) {
> +			ret = -ENOTSUPP;
> +			continue;
> +		}
>  		ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
>  					       conn_state, out_bus_fmts[i]);
>  		if (ret != -ENOTSUPP)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/10] drm/display: hdmi-state-helper: Act on color format DRM property

[Maxime Ripard]

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

Hi,

On Mon, Nov 17, 2025 at 08:11:51PM +0100, Nicolas Frattaroli wrote:
> With the introduction of the "color format" DRM property, which allows
> userspace to request a specific color format, the HDMI state helper
> should implement this.
> 
> Implement it by checking whether the property is set and set to
> something other than auto. If so, pass the requested color format, and
> otherwise set RGB.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 8 +++++++-
>  1 file changed, 7 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 a561f124be99..add0d51fce33 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -650,9 +650,15 @@ hdmi_compute_config(const struct drm_connector *connector,
>  				       conn_state->max_bpc,
>  				       8, connector->max_bpc);
>  	int ret;
> +	enum hdmi_colorspace hdmi_colorspace;
> +
> +	if (conn_state->color_format && conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
> +		hdmi_colorspace = color_format_to_hdmi_colorspace(conn_state->color_format);
> +	else
> +		hdmi_colorspace = HDMI_COLORSPACE_RGB;
>  
>  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> -				      HDMI_COLORSPACE_RGB);
> +				      hdmi_colorspace);

I don't think we want the fallback to yuv420 for anything but auto, so
I'd rather have something like

if (conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
   return hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
                                  color_format_to_hdmi_colorspace(conn_state->color_format))

We'll also need unit tests.

Maxime

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

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

[Maxime Ripard]

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

On Mon, Nov 17, 2025 at 08:11:54PM +0100, Nicolas Frattaroli wrote:
> From: Derek Foreman <derek.foreman@collabora.com>
> 
> Register the color format property in the dw_hdmi_qp-rockchip driver,
> and act on requested format changes as part of the connector state in
> the vop2 video output driver.
> 
> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c |  3 ++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c   | 46 ++++++++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.h   |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 7c294751de19..7028166fdace 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -635,6 +635,9 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
>  				     "Failed to init bridge connector\n");
>  
> +	if (!drm_mode_create_hdmi_color_format_property(connector, supported_colorformats))
> +		drm_connector_attach_color_format_property(connector);
> +

Why shouldn't we register the property in drmm_connector_hdmi_init directly?

Maxime

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

Re: [PATCH v4 07/10] drm/display: hdmi-state-helper: Act on color format DRM property

[Nicolas Frattaroli]

On Wednesday, 19 November 2025 10:09:12 Central European Standard Time Maxime Ripard wrote:
> Hi,
> 
> On Mon, Nov 17, 2025 at 08:11:51PM +0100, Nicolas Frattaroli wrote:
> > With the introduction of the "color format" DRM property, which allows
> > userspace to request a specific color format, the HDMI state helper
> > should implement this.
> > 
> > Implement it by checking whether the property is set and set to
> > something other than auto. If so, pass the requested color format, and
> > otherwise set RGB.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 8 +++++++-
> >  1 file changed, 7 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 a561f124be99..add0d51fce33 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > @@ -650,9 +650,15 @@ hdmi_compute_config(const struct drm_connector *connector,
> >  				       conn_state->max_bpc,
> >  				       8, connector->max_bpc);
> >  	int ret;
> > +	enum hdmi_colorspace hdmi_colorspace;
> > +
> > +	if (conn_state->color_format && conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
> > +		hdmi_colorspace = color_format_to_hdmi_colorspace(conn_state->color_format);
> > +	else
> > +		hdmi_colorspace = HDMI_COLORSPACE_RGB;
> >  
> >  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> > -				      HDMI_COLORSPACE_RGB);
> > +				      hdmi_colorspace);
> 
> I don't think we want the fallback to yuv420 for anything but auto, so

Okay. Changing all the non-hdmi-state-helper drivers (amdgpu, i915)
to do this as well would require some more work however, especially
in the case of amdgpu where the code flow is not always obvious.

> I'd rather have something like
> 
> if (conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
>    return hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
>                                   color_format_to_hdmi_colorspace(conn_state->color_format))
> 
> We'll also need unit tests.

Sure, am I guessing correctly that they'd go in
drm_hdmi_state_helper_test.c?

> Maxime
> 

Re: [PATCH v4 03/10] drm: Add enum conversion from/to HDMI_COLORSPACE to DRM_COLOR_FORMAT

[Nicolas Frattaroli]

On Wednesday, 19 November 2025 05:16:48 Central European Standard Time Laurent Pinchart wrote:
> On Mon, Nov 17, 2025 at 08:11:47PM +0100, Nicolas Frattaroli wrote:
> > From: Marius Vlad <marius.vlad@collabora.com>
> > 
> > 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
> 
> The hdmi_colorspace enumerators are defined as (with comments added to
> make the values explicit)
> 
> enum hdmi_colorspace {
> 	HDMI_COLORSPACE_RGB,			/* 0 */
> 	HDMI_COLORSPACE_YUV422,			/* 1 */
> 	HDMI_COLORSPACE_YUV444,			/* 2 */
> 	HDMI_COLORSPACE_YUV420,			/* 3 */
> 	HDMI_COLORSPACE_RESERVED4,		/* 4 */
> 	HDMI_COLORSPACE_RESERVED5,		/* 5 */
> 	HDMI_COLORSPACE_RESERVED6,		/* 6 */
> 	HDMI_COLORSPACE_IDO_DEFINED,		/* 7 */
> };
> 
> and the DRM color formats as (after patch 02/10)
> 
> enum drm_color_format {
> 	DRM_COLOR_FORMAT_NONE			= 0,		/* 0 */
> 	DRM_COLOR_FORMAT_RGB444			= (1 << 0),	/* 1 */
> 	DRM_COLOR_FORMAT_YCBCR422		= (1 << 1),	/* 2 */
> 	DRM_COLOR_FORMAT_YCBCR444		= (1 << 2),	/* 4 */
> 	DRM_COLOR_FORMAT_YCBCR420		= (1 << 3),	/* 8 */
> 	/* auto case, driver will set the color_format */
> 	DRM_COLOR_FORMAT_AUTO			= (1 << 4),	/* 16 */
> };
> 
> Contrary to what is stated in the commit message, the values are not the
> same. Maybe you confused DRM_COLOR_FORMAT_* with DRM_MODE_COLOR_FORMAT_*
> ?

It's possible the equality got lost during my refactor, but I think
the needless enum conversions between like 4 different things should
be minimised anyway, so I'll look into getting rid of as much of this
as possible.

> 
> > obvious that we use different enums.
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 18 ++++++++++++++++++
> >  include/drm/drm_connector.h     |  3 +++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 0ad7be0a2d09..61c077b67ac3 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:
> > +	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;
> > +	}
> > +}
> > +EXPORT_SYMBOL(color_format_to_hdmi_colorspace);
> > +
> >  /**
> >   * 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 a071079fd3ad..e044976c8d76 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
> 
> 

Re: [PATCH v4 04/10] drm/bridge: Act on the DRM color format property

[Nicolas Frattaroli]

On Wednesday, 19 November 2025 05:32:46 Central European Standard Time Laurent Pinchart wrote:
> On Mon, Nov 17, 2025 at 08:11:48PM +0100, Nicolas Frattaroli wrote:
> > The new DRM color format property allows userspace to request a specific
> > color format on a connector. In turn, this fills the connector state's
> > color_format member to switch color formats.
> > 
> > Make drm_bridges consider the color_format set in the connector state
> > during the atomic bridge check. Specifically, reject any output bus
> > formats that do not correspond to the requested color format.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 8f355df883d8..b7df5cbad832 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -1052,6 +1052,59 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> >  	return ret;
> >  }
> >  
> > +static bool __pure bus_format_is_color_fmt(u32 bus_fmt, enum drm_color_format fmt)
> > +{
> > +	if (bus_fmt == MEDIA_BUS_FMT_FIXED)
> > +		return true;
> > +
> > +	switch (fmt) {
> > +	case DRM_COLOR_FORMAT_NONE:
> > +	case DRM_COLOR_FORMAT_AUTO:
> > +		return true;
> > +	case DRM_COLOR_FORMAT_RGB444:
> > +		switch (bus_fmt) {
> > +		case MEDIA_BUS_FMT_RGB888_1X24:
> > +		case MEDIA_BUS_FMT_RGB101010_1X30:
> > +		case MEDIA_BUS_FMT_RGB121212_1X36:
> > +		case MEDIA_BUS_FMT_RGB161616_1X48:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	case DRM_COLOR_FORMAT_YCBCR444:
> > +		switch (bus_fmt) {
> > +		case MEDIA_BUS_FMT_YUV8_1X24:
> > +		case MEDIA_BUS_FMT_YUV10_1X30:
> > +		case MEDIA_BUS_FMT_YUV12_1X36:
> > +		case MEDIA_BUS_FMT_YUV16_1X48:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	case DRM_COLOR_FORMAT_YCBCR422:
> > +		switch (bus_fmt) {
> > +		case MEDIA_BUS_FMT_UYVY8_1X16:
> > +		case MEDIA_BUS_FMT_UYVY10_1X20:
> > +		case MEDIA_BUS_FMT_UYVY12_1X24:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	case DRM_COLOR_FORMAT_YCBCR420:
> > +		switch (bus_fmt) {
> > +		case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> > +		case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> > +		case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
> > +		case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	}
> 
> I'd find this more readable:
> 
> 	if (fmt == DRM_COLOR_FORMAT_NONE || fmt == DRM_COLOR_FORMAT_AUTO)
> 		return true;
> 
> 	switch (bus_fmt) {
> 	case MEDIA_BUS_FMT_RGB888_1X24:
> 	case MEDIA_BUS_FMT_RGB101010_1X30:
> 	case MEDIA_BUS_FMT_RGB121212_1X36:
> 	case MEDIA_BUS_FMT_RGB161616_1X48:
> 		return fmt == DRM_COLOR_FORMAT_RGB444:
> 
> 	case MEDIA_BUS_FMT_YUV8_1X24:
> 	case MEDIA_BUS_FMT_YUV10_1X30:
> 	case MEDIA_BUS_FMT_YUV12_1X36:
> 	case MEDIA_BUS_FMT_YUV16_1X48:
> 		return fmt == DRM_COLOR_FORMAT_YCBCR444;
> 
> 	case MEDIA_BUS_FMT_UYVY8_1X16:
> 	case MEDIA_BUS_FMT_UYVY10_1X20:
> 	case MEDIA_BUS_FMT_UYVY12_1X24:
> 		return fmt == DRM_COLOR_FORMAT_YCBCR422;
> 
> 	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> 	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> 	case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
> 	case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
> 		return fmt == DRM_COLOR_FORMAT_YCBCR420;
> 
> 	default:
> 		return false;
> 	}
> 
> but it could be a matter for personal preference ?

I agree, that is nicer.

> 
> I'm also a bit concerned about the 
> 
> 	if (fmt == DRM_COLOR_FORMAT_NONE || fmt == DRM_COLOR_FORMAT_AUTO)
> 
> test. What's the difference between NONE and AUTO ? Is it meaningful, or
> should the two enumerators be merged into a single one ?

I've noticed that as well but didn't act on it before sending out v4.
I think we should try to make them the same yes. I don't see a valid
reason why anything would ever set "NONE" when it means "AUTO". If
there is a non-AUTO "NONE" case to describe an invalid format, then
this code is wrong anyways. I'll do some digging.

> > +
> > +	return false;
> > +}
> > +
> >  /*
> >   * This function is called by &drm_atomic_bridge_chain_check() just before
> >   * calling &drm_bridge_funcs.atomic_check() on all elements of the chain.
> > @@ -1137,6 +1190,10 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
> >  	}
> >  
> >  	for (i = 0; i < num_out_bus_fmts; i++) {
> > +		if (!bus_format_is_color_fmt(out_bus_fmts[i], conn_state->color_format)) {
> > +			ret = -ENOTSUPP;
> > +			continue;
> > +		}
> >  		ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
> >  					       conn_state, out_bus_fmts[i]);
> >  		if (ret != -ENOTSUPP)
> 
> 

Re: [PATCH v4 05/10] drm/bridge: dw-hdmi-qp: Set bridge supported_formats

[Nicolas Frattaroli]

On Tuesday, 18 November 2025 21:00:08 Central European Standard Time Cristian Ciocaltea wrote:
> Hi Nicolas,
> 
> On 11/17/25 9:11 PM, Nicolas Frattaroli wrote:
> > The drm_bridge "supported_formats" member stores a bitmask of supported
> > HDMI output formats if the bridge is in fact an HDMI bridge.
> > 
> > However, until now, the synopsys dw-hdmi-qp driver did not set this
> > member in the bridge it creates.
> > 
> > Set it based on the platform data's supported_formats member, and
> > default to BIT(HDMI_COLORSPACE_RGB) if it's absent, which preserves the
> > previous behaviour.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > index fe4c026280f0..cf888236bd65 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > @@ -1269,6 +1269,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
> >  		dev_warn(dev, "Set ref_clk_rate to vendor default\n");
> >  	}
> >  
> > +	if (plat_data->supported_formats)
> > +		hdmi->bridge.supported_formats = plat_data->supported_formats;
> 
> This duplicates a change already introduced via commit 1ff27c5929ab
> ("drm/bridge: dw-hdmi-qp: Handle platform supported formats and color depth").

Hmm, looks like I didn't notice that when rebasing onto next-20251117.

(Aside note, that commit is once again lacking from next-20251119,
did it get dropped for some reason or did DRM as a whole not get
pulled into that next version due to a conflict?)

> 
> > +	else
> > +		hdmi->bridge.supported_formats = BIT(HDMI_COLORSPACE_RGB);
> 
> And this one looks redundant as well, since RGB is supposed to be mandatory:
> supported_formats defaults to RGB in drm_bridge_connector_init() if there's no
> HDMI bridge in the pipeline overriding it, while drmm_connector_hdmi_init()
> errors out if supported_formats is unset or doesn't advertise RGB.

Oops, yeah you're right

> 
> Hence I think this patch can be dropped.

Will do! Thanks for the quick review

> 
> Regards,
> Cristian
> 
> 

Re: [PATCH v4 06/10] drm/rockchip: dw_hdmi_qp: Set supported_formats platdata

[Nicolas Frattaroli]

On Tuesday, 18 November 2025 21:14:31 Central European Standard Time Cristian Ciocaltea wrote:
> On 11/17/25 9:11 PM, Nicolas Frattaroli wrote:
> > With the introduction of the supported_formats member in the
> > dw-hdmi-qp platform data struct, drivers that have access to this
> > information should now set it.
> > 
> > Set it in the rockchip dw_hdmi_qp glue driver, where such a bitmask of
> > supported color formats already exists. It just needs to be converted to
> > the appropriate HDMI_COLORSPACE_ mask.
> > 
> > This allows this information to be passed down to the dw-hdmi-qp core,
> > which sets it in the bridge it creates, and consequently will allow the
> > common HDMI bridge code to act on it.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > index c9fe6aa3e3e3..7c294751de19 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> > @@ -468,6 +468,28 @@ 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 unsigned int __pure drm_to_hdmi_fmts(const u32 fmt)
> > +{
> > +	unsigned int res = 0;
> > +
> > +	if (fmt & DRM_COLOR_FORMAT_AUTO)
> > +		res |= BIT(HDMI_COLORSPACE_RGB);
> > +	if (fmt & DRM_COLOR_FORMAT_RGB444)
> > +		res |= BIT(HDMI_COLORSPACE_RGB);
> > +	if (fmt & DRM_COLOR_FORMAT_YCBCR444)
> > +		res |= BIT(HDMI_COLORSPACE_YUV444);
> > +	if (fmt & DRM_COLOR_FORMAT_YCBCR422)
> > +		res |= BIT(HDMI_COLORSPACE_YUV422);
> > +	if (fmt & DRM_COLOR_FORMAT_YCBCR420)
> > +		res |= BIT(HDMI_COLORSPACE_YUV420);
> > +
> > +	return res;
> > +}
> > +
> >  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> >  				    void *data)
> >  {
> > @@ -521,6 +543,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> >  	plat_data.phy_data = hdmi;
> >  	plat_data.max_bpc = 10;
> >  
> > +	plat_data.supported_formats = drm_to_hdmi_fmts(supported_colorformats);
> 
> Any reason why this cannot be simply set as
> 
>   BIT(HDMI_COLORSPACE_RGB) | BIT(HDMI_COLORSPACE_YUV444) | BIT(HDMI_COLORSPACE_YUV422)
> 
> and get rid of the unnecessary conversion?

My gut feeling lead me towards trying to have a single source of
truth for the supported color formats, but upon further reflection
this is indeed way too verbose and lead me to move the
supported_colorformats definition into this patch rather than the
one where it's needed for registering the property.

So I agree with you here and will simplify this by just setting
these as you described.

Kind regards,
Nicolas Frattaroli

> 
> > +
> >  	encoder = &hdmi->encoder.encoder;
> >  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> >  
> > 
> 
> 

Re: [PATCH v4 07/10] drm/display: hdmi-state-helper: Act on color format DRM property

[Maxime Ripard]

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

On Wed, Nov 19, 2025 at 01:41:18PM +0100, Nicolas Frattaroli wrote:
> On Wednesday, 19 November 2025 10:09:12 Central European Standard Time Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Nov 17, 2025 at 08:11:51PM +0100, Nicolas Frattaroli wrote:
> > > With the introduction of the "color format" DRM property, which allows
> > > userspace to request a specific color format, the HDMI state helper
> > > should implement this.
> > > 
> > > Implement it by checking whether the property is set and set to
> > > something other than auto. If so, pass the requested color format, and
> > > otherwise set RGB.
> > > 
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 8 +++++++-
> > >  1 file changed, 7 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 a561f124be99..add0d51fce33 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > @@ -650,9 +650,15 @@ hdmi_compute_config(const struct drm_connector *connector,
> > >  				       conn_state->max_bpc,
> > >  				       8, connector->max_bpc);
> > >  	int ret;
> > > +	enum hdmi_colorspace hdmi_colorspace;
> > > +
> > > +	if (conn_state->color_format && conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
> > > +		hdmi_colorspace = color_format_to_hdmi_colorspace(conn_state->color_format);
> > > +	else
> > > +		hdmi_colorspace = HDMI_COLORSPACE_RGB;
> > >  
> > >  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> > > -				      HDMI_COLORSPACE_RGB);
> > > +				      hdmi_colorspace);
> > 
> > I don't think we want the fallback to yuv420 for anything but auto, so
> 
> Okay. Changing all the non-hdmi-state-helper drivers (amdgpu, i915)
> to do this as well would require some more work however, especially
> in the case of amdgpu where the code flow is not always obvious.

Yeah, I think we want to be consistent here, the whole point of the HDMI
state helpers was to be consistently consistent with Intel's behaviour
anyway :)

> > I'd rather have something like
> > 
> > if (conn_state->color_format != DRM_COLOR_FORMAT_AUTO)
> >    return hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> >                                   color_format_to_hdmi_colorspace(conn_state->color_format))
> > 
> > We'll also need unit tests.
> 
> Sure, am I guessing correctly that they'd go in
> drm_hdmi_state_helper_test.c?

Yes

Maxime

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