[PATCH v7 02/22] drm: Add new general DRM property "color format"

Nicolas Frattaroli posted 22 patches 2 weeks, 2 days ago
[PATCH v7 02/22] drm: Add new general DRM property "color format"
Posted by Nicolas Frattaroli 2 weeks, 2 days ago
From: Andri Yngvason <andri@yngvason.is>

Add a new general DRM property named "color format" which can be used by
userspace to request the display driver to 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 which formats 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>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |   5 ++
 drivers/gpu/drm/drm_atomic_uapi.c   |  11 +++
 drivers/gpu/drm/drm_connector.c     | 142 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h         |  35 +++++++--
 4 files changed, 188 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index cc1f0c102414..1c2fa2e5f9b6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -737,6 +737,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 dc013a22bf26..6f87b7bf567d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -931,6 +931,15 @@ 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) {
+		if (val >= DRM_COLOR_FORMAT_COUNT) {
+			drm_dbg_atomic(connector->dev,
+				       "[CONNECTOR:%d:%s] unknown color format %llu\n",
+				       connector->base.id, connector->name, val);
+			return -EINVAL;
+		}
+
+		state->color_format = val ? BIT(val - 1) : DRM_COLOR_FORMAT_AUTO;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -1016,6 +1025,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 = ffs(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 4f5b27fab475..507787619b80 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1354,6 +1354,32 @@ static const char * const colorspace_names[] = {
 	[DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
 };
 
+/**
+ * drm_get_color_format_name - return a string for color format
+ * @color_fmt: color format to return the name of
+ *
+ * Returns a string constant matching the format's name, or NULL if no match
+ * is found.
+ */
+const char *drm_get_color_format_name(enum drm_color_format color_fmt)
+{
+	switch (color_fmt) {
+	case DRM_COLOR_FORMAT_AUTO:
+		return "AUTO";
+	case DRM_COLOR_FORMAT_RGB444:
+		return "RGB";
+	case DRM_COLOR_FORMAT_YCBCR444:
+		return "YUV 4:4:4";
+	case DRM_COLOR_FORMAT_YCBCR422:
+		return "YUV 4:2:2";
+	case DRM_COLOR_FORMAT_YCBCR420:
+		return "YUV 4:2:0";
+	default:
+		return NULL;
+	}
+}
+EXPORT_SYMBOL(drm_get_color_format_name);
+
 /**
  * drm_get_colorspace_name - return a string for color encoding
  * @colorspace: color space to compute name of
@@ -1383,6 +1409,20 @@ 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_RGB444 |
+	DRM_COLOR_FORMAT_YCBCR444 |
+	DRM_COLOR_FORMAT_YCBCR422 |
+	DRM_COLOR_FORMAT_YCBCR420;
+
+/* already bit-shifted */
+static const u32 dp_colorformats =
+	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
@@ -2634,6 +2674,89 @@ int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_create_hdmi_colorspace_property);
 
+/**
+ * drm_mode_create_color_format_property - create color format property
+ * @connector: connector to create the color format property on
+ * @supported_color_formats: bitmask of &enum drm_color_format values the
+ *                           connector supports
+ *
+ * Called by a driver to create a color format property. Must be attached to
+ * the desired connector afterwards.
+ *
+ * @supported_color_formats should only include color formats the connector
+ * type can actually support.
+ *
+ * Returns:
+ * 0 on success, negative errno on error
+ */
+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_COLOR_FORMAT_COUNT];
+	unsigned int len = 1;
+	unsigned int i = 1;
+	u32 fmt;
+
+	if (connector->color_format_property)
+		return 0;
+
+	if (!supported_color_formats) {
+		drm_err(dev, "No supported color formats provided on [CONNECTOR:%d:%s]\n",
+			connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	if (supported_color_formats & ~GENMASK_U32(DRM_COLOR_FORMAT_COUNT - 1, 0)) {
+		drm_err(dev, "Unknown color formats provided on [CONNECTOR:%d:%s]\n",
+			connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	switch (connector->connector_type) {
+	case DRM_MODE_CONNECTOR_HDMIA:
+	case DRM_MODE_CONNECTOR_HDMIB:
+		if (supported_color_formats & ~hdmi_colorformats) {
+			drm_err(dev, "Color formats not allowed for HDMI on [CONNECTOR:%d:%s]\n",
+				connector->base.id, connector->name);
+			return -EINVAL;
+		}
+		break;
+	case DRM_MODE_CONNECTOR_DisplayPort:
+	case DRM_MODE_CONNECTOR_eDP:
+		if (supported_color_formats & ~dp_colorformats) {
+			drm_err(dev, "Color formats not allowed for DP on [CONNECTOR:%d:%s]\n",
+				connector->base.id, connector->name);
+			return -EINVAL;
+		}
+		break;
+	}
+
+	enum_list[0].name = drm_get_color_format_name(DRM_COLOR_FORMAT_AUTO);
+	enum_list[0].type = 0;
+
+	while (supported_color_formats) {
+		fmt = BIT(i - 1);
+		if (supported_color_formats & fmt) {
+			supported_color_formats ^= fmt;
+			enum_list[len].name = drm_get_color_format_name(fmt);
+			enum_list[len].type = i;
+			len++;
+		}
+		i++;
+	}
+
+	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;
+}
+EXPORT_SYMBOL(drm_mode_create_color_format_property);
+
 /**
  * drm_mode_create_dp_colorspace_property - create dp colorspace property
  * @connector: connector to create the Colorspace property on.
@@ -2851,6 +2974,25 @@ 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 7eaec37ae1c7..b5604dca728a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -556,6 +556,16 @@ enum drm_colorspace {
 	DRM_MODE_COLORIMETRY_COUNT
 };
 
+enum drm_color_format {
+	DRM_COLOR_FORMAT_AUTO			= 0,
+	DRM_COLOR_FORMAT_RGB444			= BIT(0),
+	DRM_COLOR_FORMAT_YCBCR444		= BIT(1),
+	DRM_COLOR_FORMAT_YCBCR422		= BIT(2),
+	DRM_COLOR_FORMAT_YCBCR420		= BIT(3),
+};
+
+#define DRM_COLOR_FORMAT_COUNT 5
+
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info
  *
@@ -699,11 +709,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 +1112,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
 	 *
@@ -2105,6 +2117,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:
 	 *
@@ -2506,6 +2524,9 @@ 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_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);
@@ -2587,6 +2608,10 @@ 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);
+
 /**
  * drm_for_each_connector_iter - connector_list iterator macro
  * @connector: &struct drm_connector pointer used as cursor

-- 
2.52.0
Re: [PATCH v7 02/22] drm: Add new general DRM property "color format"
Posted by Maxime Ripard 17 hours ago
Hi,

On Wed, Jan 21, 2026 at 03:45:09PM +0100, Nicolas Frattaroli wrote:
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7eaec37ae1c7..b5604dca728a 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -556,6 +556,16 @@ enum drm_colorspace {
>  	DRM_MODE_COLORIMETRY_COUNT
>  };
>  
> +enum drm_color_format {
> +	DRM_COLOR_FORMAT_AUTO			= 0,
> +	DRM_COLOR_FORMAT_RGB444			= BIT(0),
> +	DRM_COLOR_FORMAT_YCBCR444		= BIT(1),
> +	DRM_COLOR_FORMAT_YCBCR422		= BIT(2),
> +	DRM_COLOR_FORMAT_YCBCR420		= BIT(3),
> +};
> +
> +#define DRM_COLOR_FORMAT_COUNT 5
> +

I don't really see a reason to expose an enum, with a bunch of values
that are all mutually exclusive, as a bitmask. It's pretty inconsistent
with most (all?) the other similar properties we have.

I appreciate you did that to avoid fixing up every driver using those
values, but then maybe we don't have to? We could create a userspace
facing enum, and convert to DRM_COLOR_FORMAT internally.

Maxime
Re: [PATCH v7 02/22] drm: Add new general DRM property "color format"
Posted by Nicolas Frattaroli 16 hours ago
Hello,

On Friday, 6 February 2026 15:05:08 Central European Standard Time Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jan 21, 2026 at 03:45:09PM +0100, Nicolas Frattaroli wrote:
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 7eaec37ae1c7..b5604dca728a 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -556,6 +556,16 @@ enum drm_colorspace {
> >  	DRM_MODE_COLORIMETRY_COUNT
> >  };
> >  
> > +enum drm_color_format {
> > +	DRM_COLOR_FORMAT_AUTO			= 0,
> > +	DRM_COLOR_FORMAT_RGB444			= BIT(0),
> > +	DRM_COLOR_FORMAT_YCBCR444		= BIT(1),
> > +	DRM_COLOR_FORMAT_YCBCR422		= BIT(2),
> > +	DRM_COLOR_FORMAT_YCBCR420		= BIT(3),
> > +};
> > +
> > +#define DRM_COLOR_FORMAT_COUNT 5
> > +
> 
> I don't really see a reason to expose an enum, with a bunch of values
> that are all mutually exclusive, as a bitmask. It's pretty inconsistent
> with most (all?) the other similar properties we have.
> 
> I appreciate you did that to avoid fixing up every driver using those
> values, but then maybe we don't have to? We could create a userspace
> facing enum, and convert to DRM_COLOR_FORMAT internally.

This is what the series did at v5 and earlier. IMHO it was kind of
counter-productive, because we then had two different things for the
same purpose, and some conversion logic between them. I think it's more
error prone to do it that way (think: mixing up the two), and doesn't
have a clear benefit. Just to give a picture of how bad things get:

1. we have the HDMI color format (aka "HDMI_COLORSPACE")
2. we have driver specific output color formats, e.g. the intel ones
3. we have DRM_COLOR_FORMAT
4. we have the bus formats (multiple per color format)
5. we have the DRM plane formats (again, multiple per color format)

Adding a sixth into the mix feels a bit bad because we'll then need to
justify why we should have another layer of switch-case statements.

Kind regards,
Nicolas Frattaroli

> 
> Maxime
>