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: Fri, 27 Feb 2026 14:40:52 +1000
Message-ID: <review-patch2-20260224212639.390768-3-sunpeng.li@amd.com>
In-Reply-To: <20260224212639.390768-3-sunpeng.li@amd.com>
References: <20260224212639.390768-1-sunpeng.li@amd.com> <20260224212639.390768-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 substance. The design =E2=80=94 wra=
pping `drm_vblank_enable()`/`vblank_disable_fn()` in workers with pre/post =
callbacks =E2=80=94 is reasonable. However, several issues need attention:

**Issue 1: Unconditional workqueue allocation**

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

This is allocated for **every** DRM device in `drm_vblank_init()`, even tho=
ugh the vast majority of drivers will never use deferred vblank. Only devic=
es implementing the `pre/post_enable/disable_vblank` callbacks need this wo=
rkqueue. The allocation (and init of `enable_work`, `disable_work`, `enable=
_done` per-CRTC) should be deferred or conditional. One approach: allocate =
it lazily on first use, or check at CRTC init time if any CRTC needs it.

**Issue 2: Missing cancellation of pending deferred work in drm_crtc_vblank=
_off()**

The patched `drm_crtc_vblank_off()` calls `pre_disable_vblank` and `post_di=
sable_vblank` directly, and calls `drm_vblank_disable_and_save()` under the=
 spinlock. But it does **not** cancel any pending deferred `enable_work` or=
 `disable_work`.

Consider this race:
1. `drm_vblank_get()` queues `enable_work` to the deferred workqueue
2. Before the worker runs, `drm_crtc_vblank_off()` is called (modeset)
3. `vblank_off` disables vblank and sets `inmodeset`
4. The queued `enable_work` now runs, takes `vbl_lock`, calls `drm_vblank_e=
nable()` =E2=80=94 which checks `!vblank->enabled`, sees it's false, and **=
re-enables vblank** after `vblank_off` has already shut everything down

This could lead to vblank interrupts firing on hardware that the driver exp=
ects to be quiesced. `drm_crtc_vblank_off()` should `cancel_work_sync(&vbla=
nk->enable_work)` and `cancel_delayed_work_sync(&vblank->disable_work)` (af=
ter dropping spinlocks, before or after the existing `drm_vblank_flush_work=
er()` call).

Similarly, `drm_crtc_vblank_on_config()` should cancel any pending deferred=
 `disable_work` to avoid a stale disable racing with the new enable.

**Issue 3: Cosmetic extra parentheses**

```c
if ((!needs_deferred_enable && !vblank->enabled)) {
```

The outer parentheses are unnecessary.

**Issue 4: Comment style**

```c
/* Initialize in the completed state, the first deferred vblank
 * enable will reinitialize this. */
```

Kernel multi-line comment style puts `*/` on its own line:
```c
/*
 * Initialize in the completed state, the first deferred vblank
 * enable will reinitialize this.
 */
```

---
Generated by Claude Code Patch Reviewer