From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/connector: hdmi: Add a 'link bpc' property
Date: Thu, 12 Mar 2026 06:57:50 +1000
Message-ID: <review-patch1-20260311-link-bpc-v4-1-51775e964720@collabora.com>
In-Reply-To: <20260311-link-bpc-v4-1-51775e964720@collabora.com>
References: <20260311-link-bpc-v4-0-51775e964720@collabora.com> <20260311-link-bpc-v4-1-51775e964720@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Timing of property update in `commit_tail`** (medium severity):

```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 !=3D new_conn_state->link_bpc)
        drm_connector_update_link_bpc_property(connector,
                                               new_conn_state);
}
```

This fires the uevent before `funcs->atomic_commit_tail()` runs, meaning us=
erspace is notified of a change before hardware is actually programmed. For=
 drivers with custom `atomic_commit_tail` (like amdgpu), this is especially=
 problematic. The update should be placed *after* the hw commit, e.g. after=
 `drm_atomic_helper_commit_hw_done()`. However, the comment at line 2071 wa=
rns that `new_crtc_state` can't be accessed after `commit_hw_done()` =E2=80=
=94 but `connector_state` should still be safe. Consider moving this loop a=
fter the `atomic_commit_tail` call.

**Double clamping** (minor):

```c
void
drm_connector_update_link_bpc_property(struct drm_connector *connector,
                                       struct drm_connector_state *state)
{
    u8 bpc =3D clamp(state->link_bpc, 8, state->max_bpc);
```

The clamping is done both here and in the amdgpu helper (`amdgpu_dm_update_=
link_bpc`). For HDMI connectors, `output_bpc` is assigned directly. The def=
ensive clamping is fine but worth noting that drivers are expected to provi=
de sane values, and the double-clamp in patch 2 is redundant.

**Hardcoded minimum of 8** (minor):

```c
if (max_bpc < 8 || max_bpc > U8_MAX)
    return -EINVAL;

prop =3D drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
                                 "link bpc", 8, max_bpc);
```

The 8 bpc lower bound is hardcoded. Some display links (eDP panels, for ins=
tance) legitimately operate at 6 bpc. If the intent is to make this generic=
 for future non-HDMI connectors, consider either making the minimum a param=
eter or documenting why 8 is the universal floor.

**HDMI helper sets link_bpc only when output changes** (potential issue):

```c
if (old_conn_state->hdmi.broadcast_rgb !=3D new_conn_state->hdmi.broadcast_=
rgb ||
    old_conn_state->hdmi.output_bpc !=3D new_conn_state->hdmi.output_bpc ||
    old_conn_state->hdmi.output_format !=3D new_conn_state->hdmi.output_for=
mat) {
        ...
        new_conn_state->link_bpc =3D new_conn_state->hdmi.output_bpc;
```

`link_bpc` is only set when one of the HDMI output parameters changed. On t=
he very first modeset (when `old_conn_state->link_bpc` is 0 from `kzalloc` =
and `new_conn_state->link_bpc` is also 0 from `duplicate_state` if it's the=
 initial state), if `output_bpc` hasn't changed between old and new, `link_=
bpc` will remain 0 and the property won't be updated. This seems like it co=
uld lead to the property reporting `max_bpc` (the initial attached value) w=
hile the actual link_bpc state is 0 until the first mode change triggers th=
e conditional.

**Property documentation** looks good and is well-placed in the standard co=
nnector properties section.

---
Generated by Claude Code Patch Reviewer