Claude review: drm/vblank: Introduce deferred vblank enable/disable

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the core patch and has the most issues:

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

```c
dev->deferred_vblank_wq = 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 call `drm_vblank_enable()` → `crtc->funcs->enable_vblank()` on hardware that's being torn down. The worker does:

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

But `drm_crtc_vblank_off()` sets `vblank->inmodeset` under `vbl_lock`, which would make `drm_vblank_enable()` fail with `-EINVAL` (because the refcount 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 pending enable work in `drm_crtc_vblank_off()` before proceeding, or checking `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 `queue_work()` for enable is non-delayed, so it may execute first, but there's no explicit cancellation of the pending `disable_work`. After enable completes, the stale disable could fire. The regular (non-deferred) path handles this via the `disable_timer` being re-armed, but the deferred path should cancel `disable_work` when queueing `enable_work`.

**4. drm_wait_vblank_ioctl ordering bug**

```c
ret = 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 `ret`. If `drm_vblank_get()` returned an error, this waits on a potentially stale/completed completion. It should be inside the success path:

```c
ret = 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 paths**

In `drm_dev_init_release()`, only `flush_workqueue` + `destroy_workqueue` is 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 =
    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_deferred_enable`) to block even though enable already completed. Using `completion_done()` instead would be non-destructive.

**7. `WQ_UNBOUND` with `max_active=0`**

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

---
Generated by Claude Code Patch Reviewer