Message-ID: <20251119041532.GB10711@pendragon.ideasonboard.com>
Date: Wed, 19 Nov 2025 13:15:32 +0900
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Harry Wentland <harry.wentland@....com>, Leo Li <sunpeng.li@....com>,
	Rodrigo Siqueira <siqueira@...lia.com>,
	Alex Deucher <alexander.deucher@....com>,
	Christian König <christian.koenig@....com>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Sandy Huang <hjc@...k-chips.com>,
	Heiko Stübner <heiko@...ech.de>,
	Andy Yan <andy.yan@...k-chips.com>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@...el.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>, kernel@...labora.com,
	amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, intel-gfx@...ts.freedesktop.org,
	intel-xe@...ts.freedesktop.org, Andri Yngvason <andri@...vason.is>,
	Werner Sembach <wse@...edocomputers.com>,
	Marius Vlad <marius.vlad@...labora.com>
Subject: Re: [PATCH v4 02/10] drm: Add new general DRM property "color format"

On Mon, Nov 17, 2025 at 08:11:46PM +0100, Nicolas Frattaroli wrote:
> From: Andri Yngvason <andri@...vason.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@...edocomputers.com>
> Signed-off-by: Andri Yngvason <andri@...vason.is>
> Signed-off-by: Marius Vlad <marius.vlad@...labora.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

Powered by blists - more mailing lists