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/vblank: Introduce deferred vblank enable/disable
Date: Wed, 25 Mar 2026 07:22:01 +1000
Message-ID: <review-patch2-20260323202755.315929-3-sunpeng.li@amd.com>
In-Reply-To: <20260323202755.315929-3-sunpeng.li@amd.com>
References: <20260323202755.315929-1-sunpeng.li@amd.com> <20260323202755.315929-3-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

This is the core patch and has the most issues:

**1. Workqueue allocated unconditionally (wasteful)**

```c
dev->deferred_vblank_wq =3D alloc_workqueue("drm_vblank_deferred_wq",
                                          WQ_HIGHPRI | WQ_UNBOUND,
                                          0);
```

This allocates a workqueue for *every* DRM device regardless of whether any=
 CRTC uses deferred vblank. This wastes resources for the vast majority of =
drivers. Consider lazily allocating it (e.g., in `drm_crtc_init_with_planes=
` when the funcs have deferred callbacks) or at least guarding it behind a =
check.

**2. Race between deferred enable and drm_crtc_vblank_off()**

`drm_vblank_get()` queues the enable worker, then releases `vbl_lock`. If `=
drm_crtc_vblank_off()` runs before the worker executes, the worker will cal=
l `drm_vblank_enable()` =E2=86=92 `crtc->funcs->enable_vblank()` on hardwar=
e that's being torn down. The worker does:

```c
spin_lock_irqsave(&dev->vbl_lock, irqflags);
ret =3D drm_vblank_enable(dev, vblank->pipe);
```

But `drm_crtc_vblank_off()` sets `vblank->inmodeset` under `vbl_lock`, whic=
h would make `drm_vblank_enable()` fail with `-EINVAL` (because the refcoun=
t was already decremented by `vblank_off`). However, the `pre_enable_vblank=
()` callback runs *before* this lock is taken, so the driver's blocking pre=
-work could still execute against torn-down hardware. Consider cancelling p=
ending enable work in `drm_crtc_vblank_off()` before proceeding, or checkin=
g `vblank->inmodeset` inside the worker before calling the pre-hook.

**3. Race between deferred disable and deferred enable**

If a deferred disable is pending (delayed work) and a new `drm_vblank_get()=
` triggers a deferred enable, the pending disable isn't cancelled. The `que=
ue_work()` for enable is non-delayed, so it may execute first, but there's =
no explicit cancellation of the pending `disable_work`. After enable comple=
tes, the stale disable could fire. The regular (non-deferred) path handles =
this via the `disable_timer` being re-armed, but the deferred path should c=
ancel `disable_work` when queueing `enable_work`.

**4. drm_wait_vblank_ioctl ordering bug**

```c
ret =3D drm_vblank_get(dev, pipe);
drm_crtc_vblank_wait_deferred_enable(drm_crtc_from_index(dev, pipe));

if (ret) {
```

The `drm_crtc_vblank_wait_deferred_enable()` is called *before* checking `r=
et`. If `drm_vblank_get()` returned an error, this waits on a potentially s=
tale/completed completion. It should be inside the success path:

```c
ret =3D drm_vblank_get(dev, pipe);
if (ret) {
    ...error handling...
}
drm_crtc_vblank_wait_deferred_enable(drm_crtc_from_index(dev, pipe));
```

**5. Missing `cancel_work_sync` / `cancel_delayed_work_sync` in cleanup pat=
hs**

In `drm_dev_init_release()`, only `flush_workqueue` + `destroy_workqueue` i=
s done. But `drm_vblank_init_release()` (per-CRTC cleanup via `timer_delete=
_sync`) doesn't cancel the per-CRTC `enable_work` and `disable_work`. These=
 should be cancelled in `drm_vblank_init_release()`:

```c
cancel_work_sync(&vblank->enable_work);
cancel_delayed_work_sync(&vblank->disable_work);
```

**6. `try_wait_for_completion` used outside lock for racy check**

```c
bool deferred_enable_pending =3D
    needs_deferred_enable &&
    !try_wait_for_completion(&vblank->enable_done);
```

`try_wait_for_completion()` actually *consumes* the completion if it's done=
. Inside `drm_vblank_get()` under `vbl_lock`, this consumption could cause =
a subsequent `wait_for_completion_timeout` (from `drm_crtc_vblank_wait_defe=
rred_enable`) to block even though enable already completed. Using `complet=
ion_done()` instead would be non-destructive.

**7. `WQ_UNBOUND` with `max_active=3D0`**

The workqueue is created with `WQ_UNBOUND` and `max_active=3D0` (unlimited)=
. The commit message says "single-threaded worker" but the workqueue is not=
 single-threaded. Enable and disable workers for different CRTCs (or even t=
he same CRTC) could run concurrently. If single-threaded semantics are inte=
nded, use `alloc_ordered_workqueue()`.

---
Generated by Claude Code Patch Reviewer