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/i915/gem: Drop check for changed VM in EXECBUF Date: Sun, 12 Apr 2026 11:46:35 +1000 Message-ID: In-Reply-To: <20260409053111.8914-1-joonas.lahtinen@linux.intel.com> References: <20260409053111.8914-1-joonas.lahtinen@linux.intel.com> <20260409053111.8914-1-joonas.lahtinen@linux.intel.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 **Correctness: Good.** I verified in the current tree: - `ctx->vm` is only assigned during context creation (`i915_gem_context.c:1= 642`) and in mock test code. There is no runtime setter =E2=80=94 the proto= -context design made it immutable. - All VMAs inserted into `handles_vma` are created via `i915_vma_instance(o= bj, vm, NULL)` at `i915_gem_execbuffer.c:927`, using the same `vm` that is = checked. The radix tree is per-context, and the VM is per-context, so any V= MA found in the tree will always have `vma->vm =3D=3D vm`. **The diff:** ```c - if (likely(vma && vma->vm =3D=3D vm)) + if (likely(vma)) vma =3D i915_vma_tryget(vma); ``` This correctly preserves the NULL check (the `radix_tree_lookup` can return= NULL) while removing the redundant VM comparison. The `else vma =3D NULL` = branch on line 901-902 is now only reached when `vma` is already NULL from = the lookup, which is fine =E2=80=94 the code still sets `vma =3D NULL` for = the `else` case and falls through to the slow path. **Minor observations:** 1. The `else vma =3D NULL` clause (lines 901-902, not shown in diff context= but present in the tree) is now technically redundant since the only way t= o reach it is when `vma` is already NULL. It's harmless and keeps the code = readable, but a pedantic follow-up could simplify the entire block to just = `vma =3D radix_tree_lookup(...); if (vma) vma =3D i915_vma_tryget(vma);`. N= ot worth blocking on. 2. The local variable `struct i915_address_space *vm =3D eb->context->vm;` = on line 890 becomes unused by this patch. The compiler should warn about th= is. However, `vm` is still used at line 927 (`i915_vma_instance(obj, vm, NU= LL)`), so it remains needed. No issue here. 3. As noted in the overall review, the v3 mentioned a `WARN_ON_ONCE` that w= as dropped in v4. If reviewers want belt-and-suspenders validation, somethi= ng like `WARN_ON_ONCE(vma && vma->vm !=3D vm)` before the `i915_vma_tryget`= would catch any future regression where a VMA ends up in the wrong context= 's radix tree. But this is a stylistic/policy choice, not a correctness con= cern. **Commit message quality:** Excellent. The historical analysis is thorough,= references are provided, and the reasoning is clear. --- Generated by Claude Code Patch Reviewer