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: uapi: Add macro for chipset specific event ID region
Date: Sun, 12 Apr 2026 12:13:17 +1000
Message-ID: <review-patch1-20260408163608.361826-2-bence.csokas@arm.com>
In-Reply-To: <20260408163608.361826-2-bence.csokas@arm.com>
References: <20260408163608.361826-2-bence.csokas@arm.com> <20260408163608.361826-2-bence.csokas@arm.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

**Value correctness =E2=80=94 OK**

All conversions are value-preserving:
- `DRM_EXYNOS_G2D_EVENT`: `0x0 | 0x80000000 =3D 0x80000000` (was `0x8000000=
0`) =E2=9C=93
- `DRM_EXYNOS_IPP_EVENT`: `0x2 | 0x80000000 =3D 0x80000002` (was `0x8000000=
2`) =E2=9C=93
- `VIRTGPU_EVENT_FENCE_SIGNALED`: `0x10000000 | 0x80000000 =3D 0x90000000` =
(was `0x90000000`) =E2=9C=93
- `DRM_VMW_EVENT_FENCE_SIGNALED`: `0x0 | 0x80000000 =3D 0x80000000` (was `0=
x80000000`) =E2=9C=93

**Missing conversion =E2=80=94 nouveau_drm.h**

```c
/* nouveau_drm.h line 28 */
#define DRM_NOUVEAU_EVENT_NVIF                                       0x8000=
0000
```

This is a vendor-specific event at `0x80000000` that was not converted. The=
 cause is likely the include ordering in `nouveau_drm.h`:

```c
/* line 28 */ #define DRM_NOUVEAU_EVENT_NVIF   0x80000000
/* line 30 */ #include "drm.h"
```

The macro isn't defined yet at line 28. The fix would be to move the `#incl=
ude "drm.h"` above the define, or move the define below the include. Either=
 way, this omission should be mentioned in the commit message, or better ye=
t, fixed as part of this patch.

**Comment update opportunity**

The existing comment at `drm.h:1387`:

```c
 * Event types 0 - 0x7fffffff are generic DRM events, 0x80000000 and
 * up are chipset specific.
```

could be updated to reference the new `DRM_EVENT_VENDOR_SPECIFIC()` macro, =
reinforcing the link between the comment and the code it describes. This is=
 optional but would improve discoverability.

**Macro design =E2=80=94 minor concern**

```c
#define DRM_EVENT_VENDOR_SPECIFIC(_v) ((_v) | 0x80000000)
```

The OR operation doesn't guard against a caller passing a value that alread=
y has bit 31 set (e.g., `DRM_EVENT_VENDOR_SPECIFIC(0x80000000)` would produ=
ce `0x80000000`, silently masking a likely error). For UAPI this is probabl=
y fine =E2=80=94 adding `BUILD_BUG_ON` style validation would be impractica=
l in a header =E2=80=94 but it's worth noting.

**Naming**

`DRM_EVENT_VENDOR_SPECIFIC` reads somewhat like a constant rather than a fu=
nction-like macro. Names like `DRM_EVENT_VENDOR(_v)` or `DRM_VENDOR_EVENT(_=
v)` might be slightly more conventional, but this is a subjective style poi=
nt and the current name is descriptive enough.

**Summary**: The patch is a net improvement in code clarity. The missing `n=
ouveau_drm.h` conversion is the main issue =E2=80=94 it should either be in=
cluded (with the necessary include reorder) or explicitly explained in the =
commit message. With that addressed, the patch looks ready.

---
Generated by Claude Code Patch Reviewer