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: Add DRM_VBLANK_FLAG_SIMULATED
Date: Thu, 28 May 2026 12:19:14 +1000
Message-ID: <review-patch2-20260527133917.207150-3-tzimmermann@suse.de>
In-Reply-To: <20260527133917.207150-3-tzimmermann@suse.de>
References: <20260527133917.207150-1-tzimmermann@suse.de> <20260527133917.207150-3-tzimmermann@suse.de>
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 behavioral patch. It defines `DRM_VBLANK_FLAG_SIMULATED` a=
s `BIT(1)` and adds early-return checks in three ioctl handlers.

**Observations:**

- **BIT(1) not BIT(0):** The flag starts at `BIT(1)` rather than `BIT(0)`. =
This is unusual but perhaps intentional to reserve BIT(0) for something els=
e, or a minor mistake. It works correctly either way since the flags field =
starts at 0, but it's worth asking whether this was deliberate. If there's =
no planned BIT(0), starting at BIT(0) would be more conventional.

- The three ioctl checks are placed at sensible positions =E2=80=94 after t=
he vblank struct is obtained but before any real work is done:

  In `drm_wait_vblank_ioctl`:
  ```c
  +	if (vblank->flags & DRM_VBLANK_FLAG_SIMULATED)
  +		return -EOPNOTSUPP;
  ```

  In `drm_crtc_get_sequence_ioctl`:
  ```c
  +	if (vblank->flags & DRM_VBLANK_FLAG_SIMULATED)
  +		return -EOPNOTSUPP;
  ```

  In `drm_crtc_queue_sequence_ioctl`:
  ```c
  +	if (vblank->flags & DRM_VBLANK_FLAG_SIMULATED)
  +		return -EOPNOTSUPP;
  ```

- **Error code choice:** `-EOPNOTSUPP` is reasonable for "this operation is=
 not supported on this CRTC." The alternative would be `-ENOTSUP` (same val=
ue on Linux) or `-EINVAL`. The choice here is fine =E2=80=94 it clearly com=
municates that the operation itself is understood but not available for thi=
s device.

- **Potential concern in `drm_crtc_queue_sequence_ioctl`:** The flag check =
is placed *after* the `kzalloc_obj(*e)` call... wait, re-reading the diff m=
ore carefully:
  ```c
  +	if (vblank->flags & DRM_VBLANK_FLAG_SIMULATED)
  +		return -EOPNOTSUPP;
  +
   	e =3D kzalloc_obj(*e);
  ```
  Actually the check is placed *before* the allocation. Good =E2=80=94 this=
 avoids allocating and then immediately freeing.

- **UAPI impact:** This changes behavior for existing userspace application=
s. Programs that currently call `DRM_IOCTL_WAIT_VBLANK` on these virtual GP=
U devices will start getting `-EOPNOTSUPP` instead of (potentially misleadi=
ng) success. The cover letter notes that major compositors (Weston, KWin, M=
utter) don't rely on this ioctl for rate-limiting, and this was discussed o=
n IRC. This seems adequately vetted.

---
Generated by Claude Code Patch Reviewer