[PATCH v5 0/3] Add "link bpc" DRM property

[PATCH v5 0/3] Add "link bpc" DRM property

[Nicolas Frattaroli]

This series adds a new "link bpc" DRM property. It reflects the display
link's actual achieved output bits per component, considering any
degradation of the bit depth done by drivers for bandwidth or other
reasons. The property's value is updated during an atomic commit, which
is also when it fires an uevent if it changed to let userspace know.

There's a weston implementation at [1] which makes use of this new
property to warn when a user's requested bpc could not be reached.

[1]: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850

---
Changes in v5:
- Split "drm/connector: hdmi: Add a 'link bpc' property" into two
  patches; one adding the property, the other making use for it in the
  HDMI helpers (Maxime)
- Apply Maxime's Ack to the two split patches as per the review
- Link to v4: https://lore.kernel.org/r/20260311-link-bpc-v4-0-51775e964720@collabora.com

Changes in v4:
- Throw out the workqueue and delayed work item
- Remove the drm_connector_update_link_bpc_state function
- Reimplement it by updating the property and firing the uevent in
  commit_tail
- Check that the provided max_bpc value in attach_link_bpc_property is
  within the expected range
- Clamp the connector state's link_bpc value between 8 and max_bpc so
  that no value outside the declared range is ever written to the drm
  property
- Update and reword doc strings
- Add an amdgpu implementation
- Link to v3: https://lore.kernel.org/r/20251022162843.1759-1-marius.vlad@collabora.com/T/

Changes in v3:
- remove VRR mention from commit description (Ville)
- add DRM_MODE_PROP_IMMUTABLE to flags (Ville)
- provide helpers functions for drivers to use (can be used by other
  types of connectors, not just HDMI)
- send uevent informating userspace when 'link bpc' connector state
  changed (Daniel @ https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850)
- added missing doc entry
- Link to v2: https://lore.kernel.org/r/20251006083043.3115-1-marius.vlad@collabora.com/T/

Changes in v2:
- replace return with EBUSY if connector already exists (Dmitry)
- add i-g-t test and an implementation for Weston (Dmitry)
- re-wording patch description (Jani)
- Link to v1: https://lore.kernel.org/r/20250801101750.1726-1-marius.vlad@collabora.com/T/

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

---
Nicolas Frattaroli (3):
      drm/connector: Add a 'link bpc' property
      drm/connector: hdmi: Add support for 'link bpc' property
      drm/amd/display: Add support for 'link bpc' property

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++-
 drivers/gpu/drm/display/drm_hdmi_state_helper.c   |  2 +
 drivers/gpu/drm/drm_atomic_helper.c               |  9 +++
 drivers/gpu/drm/drm_atomic_uapi.c                 |  2 +
 drivers/gpu/drm/drm_connector.c                   | 86 +++++++++++++++++++++++
 include/drm/drm_connector.h                       | 16 +++++
 6 files changed, 129 insertions(+), 1 deletion(-)
---
base-commit: f4482de2c06e19b0c337b774e485755378990614
change-id: 20260309-link-bpc-d0afc475ac49

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

[PATCH v5 1/3] drm/connector: Add a 'link bpc' property

[Nicolas Frattaroli]

Display drivers may degrade from higher bit depths to lower bit depths
for reasons such as bandwidth constraints. Userspace applications, such
as compositors, may wish to know that this has occurred transparently,
instead of assuming that the "max bpc" they requested could be reached.

Introduce a new immutable DRM property called "link bpc" that reflects
the current display link's bits-per-component. An uevent is fired when
the link bpc value changes on an atomic commit.

Acked-by: Maxime Ripard <mripard@kernel.org>
Co-developed-by: Derek Foreman <derek.foreman@collabora.com>
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
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/drm_atomic_helper.c |  9 ++++
 drivers/gpu/drm/drm_atomic_uapi.c   |  2 +
 drivers/gpu/drm/drm_connector.c     | 82 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h         | 16 ++++++++
 4 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 26953ed6b53e..4a932f543ac7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2033,9 +2033,11 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
 
 static void commit_tail(struct drm_atomic_state *state)
 {
+	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_device *dev = state->dev;
 	const struct drm_mode_config_helper_funcs *funcs;
 	struct drm_crtc_state *new_crtc_state;
+	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	ktime_t start;
 	s64 commit_time_ms;
@@ -2059,6 +2061,13 @@ static void commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_wait_for_dependencies(state);
 
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
+					   new_conn_state, i) {
+		if (old_conn_state->link_bpc != new_conn_state->link_bpc)
+			drm_connector_update_link_bpc_property(connector,
+							       new_conn_state);
+	}
+
 	/*
 	 * We cannot safely access new_crtc_state after
 	 * drm_atomic_helper_commit_hw_done() so figure out which crtc's have
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 5bd5bf6661df..12e7a5ab2299 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1020,6 +1020,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->link_bpc_property) {
+		*val = state->link_bpc;
 	} 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 e70699c59c43..878e9db2c895 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -542,6 +542,75 @@ int drmm_connector_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drmm_connector_init);
 
+/**
+ * drm_connector_attach_link_bpc_property - create and attach 'link bpc' property
+ * @connector: drm connector
+ * @max_bpc: specify the upper limit, matching  that of 'max bpc' property
+ *
+ * Create and attach the 'link bpc' DRM property on @connector with an upper
+ * limit of @max_bpc.
+ *
+ * Returns:
+ * 0 on success, negative errno on failure.
+ */
+int
+drm_connector_attach_link_bpc_property(struct drm_connector *connector,
+				       unsigned int max_bpc)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (connector->link_bpc_property)
+		return -EBUSY;
+
+	if (max_bpc < 8 || max_bpc > U8_MAX)
+		return -EINVAL;
+
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
+					 "link bpc", 8, max_bpc);
+	if (!prop)
+		return -ENOMEM;
+
+	connector->link_bpc_property = prop;
+
+	drm_object_attach_property(&connector->base, prop, max_bpc);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_link_bpc_property);
+
+/**
+ * drm_connector_update_link_bpc_property - update the 'link bpc' property of a
+ *                                          connector and fire uevent
+ * @connector: pointer to the &struct drm_connector
+ * @state: pointer to the &struct drm_connector_state with the new value
+ *
+ * Update the 'link bpc' property of the given @connector to the
+ * &drm_connector_state.link_bpc member's value of @state and fire a uevent.
+ */
+void
+drm_connector_update_link_bpc_property(struct drm_connector *connector,
+				       struct drm_connector_state *state)
+{
+	u8 bpc = clamp(state->link_bpc, 8, state->max_bpc);
+
+	if (!connector->link_bpc_property)
+		return;
+
+	if (bpc != state->link_bpc)
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Clamping link bpc from %u to %u\n",
+			    connector->base.id, connector->name, state->link_bpc, bpc);
+
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Setting state link bpc %u\n",
+				     connector->base.id, connector->name, bpc);
+	drm_object_property_set_value(&connector->base, connector->link_bpc_property,
+				      bpc);
+
+	drm_sysfs_connector_property_event(connector,
+					   connector->link_bpc_property);
+}
+EXPORT_SYMBOL(drm_connector_update_link_bpc_property);
+
 /**
  * drmm_connector_hdmi_init - Init a preallocated HDMI connector
  * @dev: DRM device
@@ -1713,6 +1782,19 @@ EXPORT_SYMBOL(drm_hdmi_connector_get_output_format_name);
  *	drm_connector_attach_max_bpc_property() to create and attach the
  *	property to the connector during initialization.
  *
+ * link bpc:
+ *	This immutable range property can be used by userspace to determine the
+ *	current display link's bit depth. Drivers can use
+ *	drm_connector_attach_link_bpc_property() to create and attach the
+ *	property to the connector during initialization. They can then set the
+ *	&drm_connector_state.link_bpc member to the actual output bit depth
+ *	after any degradation. The drm property will be updated to this member's
+ *	value on the next atomic commit, and if it changed, a uevent will be
+ *	fired.
+ *	Userspace can listen to the uevent to be notified of link bpc changes,
+ *	and compare the property's value to what userspace requested to
+ *	determine whether colour depth has been degraded.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c18be8c19de0..3c339a9840c9 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1132,6 +1132,11 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @link_bpc: Current display link's bits-per-component.
+	 */
+	u8 link_bpc;
+
 	/**
 	 * @privacy_screen_sw_state: See :ref:`Standard Connector
 	 * Properties<standard_connector_properties>`
@@ -2124,6 +2129,12 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @link_bpc_property: Connector property that reflects the current
+	 * output bits per component.
+	 */
+	struct drm_property *link_bpc_property;
+
 	/** @privacy_screen: drm_privacy_screen for this connector, or NULL. */
 	struct drm_privacy_screen *privacy_screen;
 
@@ -2534,6 +2545,11 @@ void drm_connector_attach_privacy_screen_provider(
 	struct drm_connector *connector, struct drm_privacy_screen *priv);
 void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
 
+int drm_connector_attach_link_bpc_property(struct drm_connector *connector,
+					   unsigned int max_bpc);
+void drm_connector_update_link_bpc_property(struct drm_connector *connector,
+					    struct drm_connector_state *state);
+
 /**
  * struct drm_tile_group - Tile group metadata
  * @refcount: reference count

-- 
2.53.0

Claude review: drm/connector: Add a 'link bpc' property

[Claude Code Review Bot]

Patch Review

**Timing issue in commit_tail (major concern):**

The patch inserts the link_bpc property update and uevent between `drm_atomic_helper_wait_for_dependencies()` and the `atomic_commit_tail()` call:

```c
	drm_atomic_helper_wait_for_dependencies(state);

	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
					   new_conn_state, i) {
		if (old_conn_state->link_bpc != new_conn_state->link_bpc)
			drm_connector_update_link_bpc_property(connector,
							       new_conn_state);
	}

	/* ... then atomic_commit_tail() actually programs hardware */
```

This fires the uevent *before* the hardware has been programmed. Userspace reacting to the uevent could query the property and see the new value while the hardware is still at the old state. The property update and uevent should be moved to *after* `atomic_commit_tail()` returns (but before `drm_atomic_helper_commit_cleanup_done()`). That would match the pattern used by `drm_hdcp_update_content_protection` where the property is updated after the HW state is known.

**clamp() type mismatch:**

```c
	u8 bpc = clamp(state->link_bpc, 8, state->max_bpc);
```

`state->link_bpc` is `u8`, `state->max_bpc` is `u8`, but the literal `8` is `int`. The kernel's `clamp()` macro has strict type checking and this will likely generate a compiler warning or error. Should be `clamp_t(u8, state->link_bpc, 8, state->max_bpc)` or use a `(u8)8` cast.

**Unconditional uevent on every link_bpc change:**

`drm_connector_update_link_bpc_property()` always fires a uevent via `drm_sysfs_connector_property_event()`. The caller in `commit_tail` already checks `old_conn_state->link_bpc != new_conn_state->link_bpc`, so the uevent is conditional on an actual change — that's good. However, the function itself doesn't gate on the property having actually changed from the connector object's perspective (via `drm_object_property_set_value`). If `drm_connector_update_link_bpc_property` is ever called from another path where the value hasn't changed, it would fire a spurious uevent. Consider adding a check inside the function, or document that callers must check for changes first.

**Missing `int i` declaration:**

The patch adds `for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i)` in `commit_tail()` but the loop variable `i` is already declared on line 2042 as `unsigned int i` (shared with the self_refresh loop). This works, but only because the existing `i` is `unsigned int` which matches what the macro expects.

**Return value of `drm_connector_attach_link_bpc_property` returns int:**

This function returns `int` (error codes), while the existing `drm_connector_attach_max_bpc_property()` also returns `int`. This is fine, but callers need to check it — see patch 3 comments.

**Minor nit — double space in doc:**

```c
 * @max_bpc: specify the upper limit, matching  that of 'max bpc' property
```

Extra space before "that".

---
Generated by Claude Code Patch Reviewer

[PATCH v5 2/3] drm/connector: hdmi: Add support for 'link bpc' property

[Nicolas Frattaroli]

Create and attach the 'link bpc' DRM property for every HDMI connector
that's created through drmm_connector_hdmi_init.

Then, set the connector state's link_bpc member in the HDMI atomic check
state helper to the connector state's HDMI output bpc.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
 drivers/gpu/drm/drm_connector.c                 | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index a1d16762ac7a..40648574f5e5 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -865,6 +865,8 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 		struct drm_crtc *crtc = new_conn_state->crtc;
 		struct drm_crtc_state *crtc_state;
 
+		new_conn_state->link_bpc = new_conn_state->hdmi.output_bpc;
+
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 878e9db2c895..572894dad4bf 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -693,6 +693,10 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
 	drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
 	connector->max_bpc = max_bpc;
 
+	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
+	if (ret)
+		return ret;
+
 	if (max_bpc > 8)
 		drm_connector_attach_hdr_output_metadata_property(connector);
 

-- 
2.53.0

Claude review: drm/connector: hdmi: Add support for 'link bpc' property

[Claude Code Review Bot]

Patch Review

This patch is clean and straightforward.

**Setting link_bpc during atomic check:**

```c
		new_conn_state->link_bpc = new_conn_state->hdmi.output_bpc;
```

This sets `link_bpc` from `output_bpc` during `drm_atomic_helper_connector_hdmi_check()`. The `output_bpc` is computed during check and represents the negotiated bpc after bandwidth constraints. This is the correct place to set it for HDMI connectors using the helper framework.

**Automatic attachment in drmm_connector_hdmi_init:**

```c
	ret = drm_connector_attach_link_bpc_property(connector, max_bpc);
	if (ret)
		return ret;
```

Return value is properly checked here. Good.

**One consideration:** When the CRTC is disabled (no `new_conn_state->crtc`), `link_bpc` is not explicitly set to 0 or any sentinel value. The `else` branch of the check doesn't touch `link_bpc`, so it retains whatever value was in the old state (or the default 0). This means that on disconnect, the property won't be updated (the commit_tail check compares old vs new, and if they're the same, it won't fire). This seems acceptable but could be documented.

---
Generated by Claude Code Patch Reviewer

[PATCH v5 3/3] drm/amd/display: Add support for 'link bpc' property

[Nicolas Frattaroli]

The 'link bpc' DRM property exposes the connector's current display link
bits per component value. This allows userspace to discover whether a
link has degraded from a higher bit depth to a lower one.

Add support for it in amdgpu.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 65b256a7b6c4..467821cab009 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7186,6 +7186,14 @@ static void apply_dsc_policy_for_stream(struct amdgpu_dm_connector *aconnector,
 }
 #endif
 
+static void amdgpu_dm_update_link_bpc(struct drm_connector_state *conn_state,
+				      enum dc_color_depth depth)
+{
+	/* 6 bpc is an experimental internal format only, use 8 as minimum */
+	conn_state->link_bpc = clamp(convert_dc_color_depth_into_bpc(depth), 8,
+				     conn_state->max_bpc);
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct drm_connector *connector,
 		       const struct drm_display_mode *drm_mode,
@@ -8983,8 +8991,10 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 				adev->mode_info.underscan_vborder_property,
 				0);
 
-	if (!aconnector->mst_root)
+	if (!aconnector->mst_root) {
 		drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16);
+		drm_connector_attach_link_bpc_property(&aconnector->base, 16);
+	}
 
 	aconnector->base.state->max_bpc = 16;
 	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
@@ -11427,6 +11437,9 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 			goto fail;
 		}
 
+		amdgpu_dm_update_link_bpc(drm_new_conn_state,
+					  new_stream->timing.display_color_depth);
+
 		/*
 		 * TODO: Check VSDB bits to decide whether this should
 		 * be enabled or not.

-- 
2.53.0

Claude review: drm/amd/display: Add support for 'link bpc' property

[Claude Code Review Bot]

Patch Review

**Unchecked return value:**

```c
		drm_connector_attach_link_bpc_property(&aconnector->base, 16);
```

The return value is not checked. While the existing `drm_connector_attach_max_bpc_property()` call on the line above is also not checked (and that function also returns `int`), this is still a pattern to avoid. If property creation fails (e.g., memory allocation failure), it will silently continue with a NULL `link_bpc_property`. The `drm_connector_update_link_bpc_property` does have a NULL guard, so it won't crash, but the connector will silently lack the property. Consider checking the return value, or at least adding a `drm_warn` on failure.

**Double clamping:**

```c
static void amdgpu_dm_update_link_bpc(struct drm_connector_state *conn_state,
				      enum dc_color_depth depth)
{
	/* 6 bpc is an experimental internal format only, use 8 as minimum */
	conn_state->link_bpc = clamp(convert_dc_color_depth_into_bpc(depth), 8,
				     conn_state->max_bpc);
}
```

This clamps link_bpc at assignment time. Then `drm_connector_update_link_bpc_property()` (from patch 1) clamps again:

```c
	u8 bpc = clamp(state->link_bpc, 8, state->max_bpc);
```

The double clamping is harmless but redundant. The core helper already handles clamping, so the driver could just do a straight assignment (only filtering out the 6bpc case with a `max()`). Either way it works, but it adds unnecessary complexity.

**Same clamp() type concern** applies here: `convert_dc_color_depth_into_bpc()` returns `int`, `8` is `int`, and `conn_state->max_bpc` is `u8` — mixed types in `clamp()`.

**`convert_dc_color_depth_into_bpc` can return 0:** If `display_color_depth` hits the `default` case, the function returns 0. The `clamp` will push it to 8, which is a reasonable fallback, but it might be worth a `drm_warn` in that scenario rather than silently assuming 8 bpc.

**Placement of the update call:** The `amdgpu_dm_update_link_bpc()` call is in `dm_update_crtc_state()`, which is called from `amdgpu_dm_atomic_check()`. This means the value is computed during the check phase and stored in the connector state, which is consistent with the HDMI helper approach. Good.

---
Generated by Claude Code Patch Reviewer

Re: [PATCH v5 0/3] Add "link bpc" DRM property

[Michel Dänzer]

On 3/19/26 13:28, Nicolas Frattaroli wrote:
> This series adds a new "link bpc" DRM property. It reflects the display
> link's actual achieved output bits per component, considering any
> degradation of the bit depth done by drivers for bandwidth or other
> reasons. The property's value is updated during an atomic commit, which
> is also when it fires an uevent if it changed to let userspace know.
> 
> There's a weston implementation at [1] which makes use of this new
> property to warn when a user's requested bpc could not be reached.
> 
> [1]: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850

I see no description of a real-world use case, either in this series or in the weston MR, beyond logging a message when the "link bpc" & "max bpc" property values don't match. They are not expected to match in general, so I have a hard time seeing the usefulness of that.

Moreover, there's no description of what exactly the "link bpc" property value means, e.g. vs things like DSC or dithering, or how a compositor / user would determine which value they need / want under given circumstances.

In summary, I'm skeptical that this will be useful in practice in the current form. I do see potential for spurious bug reports based on the "link bpc" property having the "wrong" value though.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH v5 0/3] Add "link bpc" DRM property

[Nicolas Frattaroli]

On Friday, 20 March 2026 15:32:37 Central European Standard Time Michel Dänzer wrote:
> On 3/19/26 13:28, Nicolas Frattaroli wrote:
> > This series adds a new "link bpc" DRM property. It reflects the display
> > link's actual achieved output bits per component, considering any
> > degradation of the bit depth done by drivers for bandwidth or other
> > reasons. The property's value is updated during an atomic commit, which
> > is also when it fires an uevent if it changed to let userspace know.
> > 
> > There's a weston implementation at [1] which makes use of this new
> > property to warn when a user's requested bpc could not be reached.
> > 
> > [1]: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
> 
> I see no description of a real-world use case, either in this series
> or in the weston MR, beyond logging a message when the "link bpc" &
> "max bpc" property values don't match. They are not expected to match
> in general, so I have a hard time seeing the usefulness of that.

Hello,

these are valid concerns. The problem being addressed is related to
userspace being able to detect whether the link has degraded due to,
say, a sketchy cable.

This patch started out as a method of forcing the output link's BPC
value to a certain value, but this is not desirable. The max bpc
property is already used to restrict the link's bpc due to sketchy
hardware that advertises a higher max bpc than it can actually
achieve.

This adds the other side of the equation, where userspace isn't
necessarily keen on blindly accepting the combination of output
link parameters the kernel degraded to. This allows userspace to
detect that an explicitly chosen value it tried did not work, and
try again with a different color format/VRR/bpc/etc.

A particular real-world use case is for playback of video content.
When playing back YUV 4:2:0 10-bit video content in a full-screen
setting, having RGB 10-bit degrade to YUV 4:2:0 10-bit rather than
RGB 8-bit is more desirable. However, this is a tradeoff only
userspace knows to make; the kernel doesn't necessarily know that
the framebuffer it has been handed as RGB 10-bit is secretly just
a video player's playback of YUV 4:2:0 10-bit content. As for
the property that let's userspace actually set the output color
format, that's a separate series of mine.

I agree that the weston implementation isn't a great showcase,
but it's actually supposed to compare link bpc with an explicitly
set max bpc config value, not the property value. The config value
exists to request a certain bpc.

> Moreover, there's no description of what exactly the "link bpc" property
> value means, e.g. vs things like DSC or dithering, or how a compositor / 
> user would determine which value they need / want under given circumstances.

I agree that I should've expanded on this after splitting it out of the
HDMI patch. It's the output BPC as HDMI understands it. That means DSC is not
a factor. I don't know if any display protocols do dithering at the
protocol level, I only know some monitors dither internally, which isn't
something that can be detected.

> In summary, I'm skeptical that this will be useful in practice in the
> current form. I do see potential for spurious bug reports based on the
> "link bpc" property having the "wrong" value though.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v5 0/3] Add "link bpc" DRM property

[Mario Kleiner]

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

As somebody who writes software for neuroscience research, I would find
this new property very useful.

Even if the 'link bpc' is not the perfectly accurate answer in presence of
dithering or display stream compression, I think it could provide an idea
about the minimum precision available, so a DRM client can at least make
sure its minimum requirements are met. E.g., a 'link bpc' of 10 would at
least guarantee 10 bpc, and effectively a bit more if spatial dithering is
applied in addition. That said, I don't have practical experience with the
effects of DSC, I don't have any suitable hardware. Afaik it is not truly
lossless, but only (supposed to be, usually) perceptually lossless. It
would be great to have some property that informs clients if DSC is active
or not, or allow some control over that.

Also as somebody who has spent many hours of his life hunting down some
sysfs or debugfs files for reporting such numbers, the files usually being
differently named, at different paths, with different formats, or not
existing at all, depending on kernel version and gpu configuration. I'd
like to do less of that in the future.

In this field of application it is often very important to know about the
actual precision of "what goes out of the computer", e.g., visual test
stimuli. Scientists use different methods to verify their experimental
stimulation setups, of different levels of rigorosity, including
photometers, colorimeters etc. to measure the actual light emitted by a
display. But knowing if things work, or where in the pipeline from app to
photon they break or degrade, if they break, is useful, and the more the
software can help with this, or warn about problems, the better for us.

Even if Wayland compositors wouldn't pick this up quickly, if it is a drm
connector property, I think it would be accessible under a native X11
X-Server via RandR output properties -- and my kind of applications still
heavily relies on native X11, as the Wayland eco system currently is not
ready for the more demanding or non-trivial use cases in this field. Also,
those properties are read-accessible to non-root, non-drm masters, so
applications like mine could read the property even under leased drm
connectors (Vulkan/WSI/display, OpenGL/EGL/drm), or probably even under a
running Wayland desktop if Wayland protocol lacks the means to do so.

In the scenarios used by my app, the app often knows what an optimal
setting for 'max bpc' or reported value from such a 'link bpc' would be, so
it can be used to reconfigure things (under X11 RandR, or as a drm master),
adapt to the situation, or at least warn the user if they are about to ruin
their experimental data collection, possibly guide them a bit in
troubleshooting.

But I could imagine regular desktop use cases, where a Wayland compositor
can somewhat know what good minimum values for 'link bpc' would be, and
maybe adapt, or give the user a hint about potentially degraded quality,
and what to do about it ("Check your cables", "Reduce video resolution or
refresh rate", "Run with less displays",...).

Similar to Nicolas rgb 10 bpc vs. yuv 10 bpc example for video playback:
While all these are critical for apps like mine, or other pro apps
depending on color quality, I'd assume a Wayland compositor could use the
same constraints, even if the worst case desktop scenario may only be an
underwhelmed user, if their HDR videos don't look as spiffy as they hoped.

- A HDR-10 display mode on a true HDR sink implies one really wants a 'link
bpc' of at least 10 bpc, especially given the large nonlinearity of EOTF's
like Perceptual Quantizer, or things will look poor. In a scientific
research setting that would not just be a bummer, but degradation would be
an absolute show stopper. Something one wants to fix, be it by
checking/swapping cables, or maybe by selecting a video mode with lower
bandwidth requirements, etc.

- Same is true for wide color gamut WCG color spaces, where one wants more
than 8 bits to resolve the larger color volume fine enough for good results.

- I'd also assume or hope that a wayland client asking for a fullscreen
(=possibly direct scanout capable) RGB10 framebuffer or fp16 fb or even
RGBA16 fb would imply to the compositor that that client really wants to
get at 10 bpc or even 12+ bpc out of the display connector. So having a too
low link bpc would be a reason to possibly notify the user.

Excuse the verbose reply, but at least from my corner of applications this
would have a big thumbs up.

Thanks,
-mario

On Fri, Mar 20, 2026 at 7:09 PM Nicolas Frattaroli <
nicolas.frattaroli@collabora.com> wrote:

> On Friday, 20 March 2026 15:32:37 Central European Standard Time Michel
> Dänzer wrote:
> > On 3/19/26 13:28, Nicolas Frattaroli wrote:
> > > This series adds a new "link bpc" DRM property. It reflects the display
> > > link's actual achieved output bits per component, considering any
> > > degradation of the bit depth done by drivers for bandwidth or other
> > > reasons. The property's value is updated during an atomic commit, which
> > > is also when it fires an uevent if it changed to let userspace know.
> > >
> > > There's a weston implementation at [1] which makes use of this new
> > > property to warn when a user's requested bpc could not be reached.
> > >
> > > [1]:
> https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850
> >
> > I see no description of a real-world use case, either in this series
> > or in the weston MR, beyond logging a message when the "link bpc" &
> > "max bpc" property values don't match. They are not expected to match
> > in general, so I have a hard time seeing the usefulness of that.
>
> Hello,
>
> these are valid concerns. The problem being addressed is related to
> userspace being able to detect whether the link has degraded due to,
> say, a sketchy cable.
>
> This patch started out as a method of forcing the output link's BPC
> value to a certain value, but this is not desirable. The max bpc
> property is already used to restrict the link's bpc due to sketchy
> hardware that advertises a higher max bpc than it can actually
> achieve.
>
> This adds the other side of the equation, where userspace isn't
> necessarily keen on blindly accepting the combination of output
> link parameters the kernel degraded to. This allows userspace to
> detect that an explicitly chosen value it tried did not work, and
> try again with a different color format/VRR/bpc/etc.
>
> A particular real-world use case is for playback of video content.
> When playing back YUV 4:2:0 10-bit video content in a full-screen
> setting, having RGB 10-bit degrade to YUV 4:2:0 10-bit rather than
> RGB 8-bit is more desirable. However, this is a tradeoff only
> userspace knows to make; the kernel doesn't necessarily know that
> the framebuffer it has been handed as RGB 10-bit is secretly just
> a video player's playback of YUV 4:2:0 10-bit content. As for
> the property that let's userspace actually set the output color
> format, that's a separate series of mine.
>
> I agree that the weston implementation isn't a great showcase,
> but it's actually supposed to compare link bpc with an explicitly
> set max bpc config value, not the property value. The config value
> exists to request a certain bpc.
>
> > Moreover, there's no description of what exactly the "link bpc" property
> > value means, e.g. vs things like DSC or dithering, or how a compositor /
> > user would determine which value they need / want under given
> circumstances.
>
> I agree that I should've expanded on this after splitting it out of the
> HDMI patch. It's the output BPC as HDMI understands it. That means DSC is
> not
> a factor. I don't know if any display protocols do dithering at the
> protocol level, I only know some monitors dither internally, which isn't
> something that can be detected.
>
> > In summary, I'm skeptical that this will be useful in practice in the
> > current form. I do see potential for spurious bug reports based on the
> > "link bpc" property having the "wrong" value though.
>
> Kind regards,
> Nicolas Frattaroli
>
>
>

[-- Attachment #2: Type: text/html, Size: 9427 bytes --]

Claude review: Add "link bpc" DRM property

[Claude Code Review Bot]

Overall Series Review

Subject: Add "link bpc" DRM property
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Patches: 7
Reviewed: 2026-03-22T04:19:39.991011

---

This 3-patch series adds a new immutable "link bpc" DRM connector property that exposes the actual bits-per-component achieved on the display link after any driver-side degradation (e.g., for bandwidth). The design is reasonable and the split is clean: patch 1 adds the core infrastructure, patch 2 wires it into the HDMI state helpers, and patch 3 adds amdgpu support. 

The main concern is **timing of the property update and uevent**: the link_bpc property is updated and the uevent is fired in `commit_tail()` *before* `atomic_commit_tail()` programs the hardware. This means userspace is notified of the new link_bpc before the display has actually switched to it. There are also a couple of minor issues noted below.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v4 0/2] Add "link bpc" DRM property

[Nicolas Frattaroli]

I'm taking over this series from Marius Vlad.

The series adds a new "link bpc" DRM property. It reflects the display
link's actual achieved output bits per component, considering any
degradation of the bit depth done by drivers for bandwidth or other
reasons. The property's value is updated during an atomic commit, which
is also when it fires an uevent if it changed to let userspace know.

There's a weston implementation at [1] which makes use of this new
property to warn when a user's requested bpc could not be reached.

[1]: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850

---
Changes in v4:
- Throw out the workqueue and delayed work item
- Remove the drm_connector_update_link_bpc_state function
- Reimplement it by updating the property and firing the uevent in
  commit_tail
- Check that the provided max_bpc value in attach_link_bpc_property is
  within the expected range
- Clamp the connector state's link_bpc value between 8 and max_bpc so
  that no value outside the declared range is ever written to the drm
  property
- Update and reword doc strings
- Add an amdgpu implementation
- Link to v3: https://lore.kernel.org/r/20251022162843.1759-1-marius.vlad@collabora.com/T/

Changes in v3:
- remove VRR mention from commit description (Ville)
- add DRM_MODE_PROP_IMMUTABLE to flags (Ville)
- provide helpers functions for drivers to use (can be used by other
  types of connectors, not just HDMI)
- send uevent informating userspace when 'link bpc' connector state
  changed (Daniel @ https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1850)
- added missing doc entry
- Link to v2: https://lore.kernel.org/r/20251006083043.3115-1-marius.vlad@collabora.com/T/

Changes in v2:
- replace return with EBUSY if connector already exists (Dmitry)
- add i-g-t test and an implementation for Weston (Dmitry)
- re-wording patch description (Jani)
- Link to v1: https://lore.kernel.org/r/20250801101750.1726-1-marius.vlad@collabora.com/T/

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

---
Nicolas Frattaroli (2):
      drm/connector: hdmi: Add a 'link bpc' property
      drm/amd/display: Add support for 'link bpc' property

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++-
 drivers/gpu/drm/display/drm_hdmi_state_helper.c   |  2 +
 drivers/gpu/drm/drm_atomic_helper.c               |  9 +++
 drivers/gpu/drm/drm_atomic_uapi.c                 |  2 +
 drivers/gpu/drm/drm_connector.c                   | 86 +++++++++++++++++++++++
 include/drm/drm_connector.h                       | 16 +++++
 6 files changed, 129 insertions(+), 1 deletion(-)
---
base-commit: 9a6bac4a4a289d3ac043f885758d208ccf07f149
change-id: 20260309-link-bpc-d0afc475ac49

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

Claude review: Add "link bpc" DRM property

[Claude Code Review Bot]

Overall Series Review

Subject: Add "link bpc" DRM property
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Patches: 3
Reviewed: 2026-03-12T06:57:50.360536

---

This is a 2-patch series adding a new immutable "link bpc" DRM connector property that exposes the actual bits-per-component achieved on a display link, letting userspace detect when bit depth has been degraded (e.g. due to bandwidth constraints). Patch 1 adds the core DRM infrastructure and HDMI helper integration; patch 2 adds amdgpu support.

The design is clean and the approach is reasonable. There are a few issues worth discussing:

**Timing concern in `commit_tail`**: The link_bpc property update and uevent fire *before* `funcs->atomic_commit_tail()` runs. This means userspace is notified of a bpc change before the hardware has actually committed the new state. For amdgpu specifically (which has its own `atomic_commit_tail`), this means the uevent fires before the display hardware is programmed. Ideally this should happen *after* the hw commit completes.

**Clamping lower bound of 8**: The property range and clamping both use 8 as the minimum, but some displays legitimately run at 6 bpc (e.g. certain eDP panels). If 6 bpc is "experimental internal" for amdgpu, that's fine for patch 2's local helper, but hard-coding 8 as the universal lower bound in the core DRM property (patch 1) seems potentially limiting.

**Missing link_bpc reset on disconnect**: When a connector is disabled/disconnected, `link_bpc` is never explicitly reset. It will carry the stale value from the last active state via `memcpy` in `duplicate_state`. This means the property could report a stale bpc value on a disconnected connector.

---

---
Generated by Claude Code Patch Reviewer