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/amd/display: Add support for 'link bpc' property
Date: Sun, 22 Mar 2026 04:19:40 +1000
Message-ID: <review-patch3-20260319-link-bpc-v5-3-5306cd04a708@collabora.com>
In-Reply-To: <20260319-link-bpc-v5-3-5306cd04a708@collabora.com>
References: <20260319-link-bpc-v5-0-5306cd04a708@collabora.com> <20260319-link-bpc-v5-3-5306cd04a708@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

**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_m=
ax_bpc_property()` call on the line above is also not checked (and that fun=
ction also returns `int`), this is still a pattern to avoid. If property cr=
eation fails (e.g., memory allocation failure), it will silently continue w=
ith 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 silentl=
y 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_stat=
e,
				      enum dc_color_depth depth)
{
	/* 6 bpc is an experimental internal format only, use 8 as minimum */
	conn_state->link_bpc =3D 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_bp=
c_property()` (from patch 1) clamps again:

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

The double clamping is harmless but redundant. The core helper already hand=
les clamping, so the driver could just do a straight assignment (only filte=
ring out the 6bpc case with a `max()`). Either way it works, but it adds un=
necessary complexity.

**Same clamp() type concern** applies here: `convert_dc_color_depth_into_bp=
c()` returns `int`, `8` is `int`, and `conn_state->max_bpc` is `u8` =E2=80=
=94 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` i=
n 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 t=
he connector state, which is consistent with the HDMI helper approach. Good.

---
Generated by Claude Code Patch Reviewer