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: rust: gpuvm: add GpuVmCore::sm_unmap()
Date: Sun, 12 Apr 2026 11:16:41 +1000
Message-ID: <review-patch4-20260409-gpuvm-rust-v6-4-b16e6ada7261@google.com>
In-Reply-To: <20260409-gpuvm-rust-v6-4-b16e6ada7261@google.com>
References: <20260409-gpuvm-rust-v6-0-b16e6ada7261@google.com> <20260409-gpuvm-rust-v6-4-b16e6ada7261@google.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

Introduces the split/merge unmap machinery: `SmData`, `OpUnmap`, `OpRemap`, `OpRemapMapData`, `OpRemapRet`, and the `sm_unmap` entrypoint.

**Observations:**

1. **`OpRemap::remap` -- `prev_data`/`next_data` initialization when not needed**: When `prev` or `next` is `None`, the corresponding `va_data` initializer is still passed but never used. The `prev_data` / `next_data` arguments are consumed regardless:
   ```rust
   if self.prev().is_some() {
       prev_ptr = va1.prepare(prev_data);
   } else {
       unused_va = Some(va1);
   }
   if self.next().is_some() {
       next_ptr = va2.prepare(next_data);
   } else {
       unused_va = Some(va2);
   }
   ```
   When `prev` is `None`, `prev_data` is consumed (dropped) implicitly. This is fine functionally -- the data initializer is just dropped. But if `prev_data` is expensive to construct, this could be wasteful. The API could accept `Option<impl PinInit<T::VaData>>` instead, but that would complicate the interface. This is a minor ergonomic tradeoff, not a bug.

2. **`OpRemap::remap` -- `unused_va` can only hold one**: If both `prev` and `next` are `None`, `unused_va` would be overwritten by `va2` (losing `va1`). However, this situation shouldn't occur in practice: `drm_gpuva_remap` always has at least `prev` or `next` (otherwise it would be a plain unmap, not a remap). The C code guarantees at least one of prev/next is non-null. Still, if this invariant is violated, one `GpuVaAlloc` would leak. A debug assertion might be warranted:
   ```rust
   debug_assert!(self.prev().is_some() || self.next().is_some());
   ```

3. **`OpUnmap::remove` -- unlink ordering**: The code calls `drm_gpuva_unmap` (removes from interval tree) then `drm_gpuva_unlink_defer` (defers removal from the GEM list). The unlink is deferred rather than immediate, matching the immediate-mode pattern where cleanup happens later via `deferred_cleanup()`. This is correct.

4. **`lock_gpuva` in `vm_bo.rs`**: The new helper:
   ```rust
   pub(super) fn lock_gpuva(&self) -> crate::sync::MutexGuard<'_, ()> {
       let ptr = unsafe { &raw mut (*self.obj().as_raw()).gpuva.lock };
       let mutex = unsafe { crate::sync::Mutex::from_raw(ptr) };
       mutex.lock()
   }
   ```
   This constructs a `Mutex` from a raw pointer to the GEM object's `gpuva.lock`. The `Mutex::from_raw` creates a reference to an existing mutex. The lock guard ensures the mutex is properly released. This is correct and needed for protecting the gpuva list during link/unlink.

5. **`SmData` holds `&'a mut UniqueRefGpuVm<T>`**: This is then cast to `*mut c_void` and passed through C callbacks. The `'a` lifetime ties the borrow to the `sm_unmap` call duration. In the callback, it's cast back:
   ```rust
   let p = unsafe { &mut *p.cast::<SmData<'_, '_, T>>() };
   ```
   This is sound because the original `SmData` lives on the stack of `sm_unmap` and the C callbacks are synchronous (called during `drm_gpuvm_sm_unmap`).

6. **Invariant lifetime pattern**: The `PhantomData<*mut &'op mut T>` used in `OpUnmap`, `OpRemap`, etc. makes `'op` invariant, preventing the driver from constructing `OpUnmapped`/`OpRemapped` tokens from different operations. This is a clever and correct approach to enforce that the completion token matches the operation.

---
Generated by Claude Code Patch Reviewer