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: Implement deferred vblanks on IPS platforms
Date: Wed, 25 Mar 2026 07:22:02 +1000
Message-ID: <review-patch4-20260323202755.315929-5-sunpeng.li@amd.com>
In-Reply-To: <20260323202755.315929-5-sunpeng.li@amd.com>
References: <20260323202755.315929-1-sunpeng.li@amd.com> <20260323202755.315929-5-sunpeng.li@amd.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

**1. `active_vblank_irq_count` is not protected**

```c
dm->active_vblank_irq_count++;
```

This counter is incremented/decremented in the deferred callbacks, which ru=
n in workqueue context. Without any lock protecting it, concurrent enable/d=
isable on different CRTCs could race. The deferred workqueue isn't ordered =
(see patch 2 review), so these can run in parallel.

**2. `dc_lock` held across pre=E2=86=92enable=E2=86=92post boundary is asym=
metric**

`pre_enable_vblank` acquires `dc_lock`, and `post_enable_vblank` releases i=
t. This means the DRM vblank core's `drm_vblank_enable()` (called between p=
re and post) runs with `dc_lock` held. This works for the IPS use case, but=
 it means the `enable_vblank` driver callback also runs with `dc_lock` held=
 =E2=80=94 which is the intent, but it couples the DRM core's locking guara=
ntees to driver-internal lock ordering. If any code path under `drm_vblank_=
enable()` ever tries to acquire a lock that `dc_lock` nests inside, it woul=
d deadlock.

**3. Accessing `crtc->state` without proper locking**

```c
struct dm_crtc_state *acrtc_state =3D to_dm_crtc_state(crtc->state);
```

In `post_enable_vblank` and `post_disable_vblank`, `crtc->state` is accesse=
d from workqueue context. The `crtc->state` pointer can change during atomi=
c commits. This needs either atomic state locking or the driver needs to en=
sure the state won't change while the worker runs (e.g., by holding a refer=
ence or ensuring commit serialization). This is a pre-existing pattern in t=
he AMDGPU driver's vblank control worker, but deferring it to the DRM core'=
s workqueue may change timing assumptions.

**4. `vblank_control_work` on the stack**

```c
struct vblank_control_work vblank_work =3D { 0 };
```

Using a stack-allocated `vblank_control_work` and calling `amdgpu_dm_crtc_s=
et_panel_sr_feature()` with it seems fine as long as that function doesn't =
store a pointer to it. Worth verifying.

---
Generated by Claude Code Patch Reviewer