From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: Remove DRIVER_GEM_GPUVA feature flag Date: Thu, 23 Apr 2026 08:39:45 +1000 Message-ID: In-Reply-To: <20260421084701.24227-1-laura.nao@collabora.com> References: <20260421084701.24227-1-laura.nao@collabora.com> <20260421084701.24227-1-laura.nao@collabora.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 **Core DRM changes (drm_gem.c, drm_drv.h, drm_gem.h, drm_debugfs.h):** These are correct. The key observations: 1. `drm_gem_gpuva_init()` is just `INIT_LIST_HEAD(&obj->gpuva.list)` =E2=80= =94 calling it unconditionally for every GEM object is harmless. The `mutex= _init(&obj->gpuva.lock)` at line 230 of `drm_gem.c` is already unconditiona= l, so the list head init is the only thing that was gated. 2. The `DRM_DEBUGFS_GPUVA_INFO` change from `DRIVER_GEM_GPUVA` to `0` is co= rrect. Looking at `drm_debugfs_create_files()`: ```c if (features && !drm_core_check_all_features(dev, features)) continue; ``` Passing `0` means `features` is falsy, so the condition short-circuits a= nd the entry is always created =E2=80=94 but only for drivers that actually= use `DRM_DEBUGFS_GPUVA_INFO` in their debugfs info list (panthor and nouve= au). Non-GPUVA drivers never reference this macro, so they don't get a spur= ious "gpuvas" debugfs entry. This is fine. 3. Removing the enum value and doc comment from `drm_drv.h` is clean. Note = that `BIT(8)` is left as a gap =E2=80=94 this is normal for DRM driver feat= ure flags and avoids ABI issues with out-of-tree drivers. 4. The doc update in `drm_gem.h` removing the "only necessary for drivers i= ntending to support DRIVER_GEM_GPUVA" comment is appropriate. **Driver changes (pvr_drv.c, msm_drv.c, nouveau_drm.c, panthor_drv.c, xe_de= vice.c):** All driver removals of the flag from `.driver_features` look correct and me= chanical. **One point to note =E2=80=94 nouveau double-init:** In `nouveau_bo.c:394`, nouveau has an explicit call: ```c drm_gem_gpuva_init(&nvbo->bo.base); ``` This exists because nouveau needs the list initialized *before* `ttm_bo_ini= t_reserved()` (as the comment explains: "Subsequent bo_move() callbacks mig= ht already iterate the GEMs GPUVA list"). Previously, the core's `drm_gem_p= rivate_object_init()` would also call it (gated by the feature flag). With = this patch, the core now calls it unconditionally, and then nouveau calls i= t again. Since it's just `INIT_LIST_HEAD()`, double-calling is harmless (re= -initializing an empty list head is a no-op in effect). However, it would b= e tidier to also remove the now-redundant explicit call in `nouveau_bo.c` a= nd its associated comment. This is a minor cleanup opportunity, not a corre= ctness issue =E2=80=94 but it would make the patch more complete. **Summary:** The patch is correct and well-reasoned. The only suggestion is= optionally removing the now-redundant `drm_gem_gpuva_init()` call in `driv= ers/gpu/drm/nouveau/nouveau_bo.c:394` (and its comment block at lines 391-3= 93), since the core will now always handle this initialization. --- Generated by Claude Code Patch Reviewer