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

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 v6 0/2] 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. It is not representative of the entire use case of the
property, but rather a proof-of-concept.

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

---
Changes in v6:
- Drop amdgpu patch to unblock series.
- Reword property documentation to make it more explicit as to what bits
  this counts and why someone would want this.
- Reword first patch message to do the same.
- Link to v5: https://lore.kernel.org/r/20260319-link-bpc-v5-0-5306cd04a708@collabora.com

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 (2):
      drm/connector: Add a 'link bpc' property
      drm/connector: hdmi: Add support for 'link bpc' property

 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                 | 88 +++++++++++++++++++++++++
 include/drm/drm_connector.h                     | 16 +++++
 5 files changed, 117 insertions(+)
---
base-commit: 3a48a999a2e3c3d1988d44370ed8f43ff54700b6
change-id: 20260309-link-bpc-d0afc475ac49

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

[PATCH v6 1/2] 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.

Introduce a new immutable DRM property called "link bpc" that reflects
the current display link's bits-per-component without considering a
lower "on the wire" bit depth caused by display protocol features such
as compression or dithering. 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     | 84 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h         | 16 +++++++
 4 files changed, 111 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 47dc53c4a738..85557106218b 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,21 @@ 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. This bit depth is not reduced by
+ *	display protocol features such as compression or dithering. 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 would've hoped for,
+ *	so that userspace gets a view into bit depth degradation of its
+ *	submitted framebuffers that may be happening on the output.
+ *
  * Connectors also have one standardized atomic property:
  *
  * CRTC_ID:
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index af8b92d2d5b7..6ca5ebf7a6d3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1154,6 +1154,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>`
@@ -2146,6 +2151,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;
 
@@ -2556,6 +2567,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

[PATCH v6 2/2] 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 9f3b696aceeb..b670bcdbd3c6 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -887,6 +887,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 85557106218b..2a2e56bf062d 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: 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-31T17:31:19.145159

---

This is a clean, well-structured 2-patch series adding a new immutable `link bpc` DRM connector property. The design is sound: an immutable range property reflecting actual link bit depth, updated during commit_tail with a uevent for userspace notification. The series is at v6 and has an Ack from Maxime Ripard.

The code is generally good, but there is one notable bug in patch 2 regarding where `link_bpc` is set, and a few minor issues worth flagging.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Overall:** Solid infrastructure patch. The property creation, update mechanism, uevent firing, and documentation are all reasonable.

**Issue 1 (minor): `clamp()` type mismatch in `drm_connector_update_link_bpc_property`**

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

`state->link_bpc` and `state->max_bpc` are both `u8`, but the literal `8` is an `int`. The kernel's `clamp()` macro with `__builtin_choose_expr` type checking may warn or misbehave with mixed types depending on the kernel version. Consider using `clamp_t(u8, state->link_bpc, 8, state->max_bpc)` for explicit type safety.

**Issue 2 (medium): link_bpc update fires uevent before hw commit**

In `commit_tail()`:
```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);
}
```

The property update and uevent fire **before** `funcs->atomic_commit_tail()` is called (which does the actual hw commit). This means userspace receives the notification that link bpc changed before the hardware has actually been programmed. Arguably this should happen after `atomic_commit_tail` returns (or at least after `commit_hw_done`). Is there a deliberate reason for this ordering? It would be more correct to place this after the tail function call, similar to how other post-commit work is done.

**Issue 3 (nit): Extra space in doc comment**

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

Double space before "that".

**Issue 4 (minor): unconditional uevent in update function**

`drm_connector_update_link_bpc_property()` always fires a uevent even though the caller in `commit_tail` already checked that the value changed. This is fine from a correctness standpoint since the caller does the filtering, but the function itself has no guard — if called outside `commit_tail` without a prior check it would fire spurious uevents. The function name suggests "update" which implies a change, so this is acceptable, but documenting the expectation that callers check for changes would be good.

**Design observation:** The initial property value is set to `max_bpc` in `drm_connector_attach_link_bpc_property()`:
```c
drm_object_attach_property(&connector->base, prop, max_bpc);
```

This means before the first commit, userspace sees `link_bpc == max_bpc`, which is optimistic but reasonable as a default. Worth noting in the documentation.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Issue 1 (bug): `link_bpc` only set when output_bpc changes, not on initial modeset**

```c
if (old_conn_state->hdmi.broadcast_rgb != new_conn_state->hdmi.broadcast_rgb ||
    old_conn_state->hdmi.output_bpc != new_conn_state->hdmi.output_bpc ||
    old_conn_state->hdmi.output_format != new_conn_state->hdmi.output_format) {
        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;
```

The `link_bpc` assignment is **inside** the `if` block that only triggers when broadcast_rgb, output_bpc, or output_format have changed from the old state. On the **first** modeset (or any modeset where these values happen to be the same as previous), `link_bpc` will remain at its default of 0 (zero-initialized connector state), which is below the range minimum of 8.

The `commit_tail` code will then see `old_conn_state->link_bpc (0) != new_conn_state->link_bpc (0)` — actually they'd be equal so it wouldn't fire, but the property would remain stale at its initial `max_bpc` value rather than reflecting the actual output_bpc.

The assignment should be moved **outside** the if block, to unconditionally set `link_bpc` to `hdmi.output_bpc`:

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

if (old_conn_state->hdmi.broadcast_rgb != new_conn_state->hdmi.broadcast_rgb ||
    ...
```

This was actually what the cover letter's diff stat area suggests (the `+` line before the `crtc_state` block), but in the actual diff the assignment ended up inside the conditional. Looking at the diff more carefully, the `+` lines show it placed right after the opening brace of the if block. This is a real bug — on initial enable the link_bpc won't be set to the actual value.

**Patch 2 otherwise looks correct:** The `drmm_connector_hdmi_init` integration properly attaches the property after `max_bpc` is set up, and the error return is properly wired.

---
Generated by Claude Code Patch Reviewer