Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()

[Alice Ryhl <aliceryhl@google.com>]

On Wed, Jan 21, 2026 at 02:31:26PM -0300, Daniel Almeida wrote:
> Hi Alice,
> 
> This looks good. See a few nits below:
> 
> > On 21 Jan 2026, at 08:31, Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > This provides a mechanism to create (or look up) VMBO instances, which
> > represent the mapping between GPUVM and GEM objects.
> > 
> > The GpuVmBoResident<T> type can be considered like ARef<GpuVm<T>>,
> > except that no way to increment the refcount is provided.
> 
> So this is like GpuVmCore, right? Since that is an ARef whose refcount cannot
> be incremented.

Sort of, except that GpuVmBoResident is not truly unique since you can
call obtain() twice.

> > The GpuVmBoAlloc<T> type is more akin to a pre-allocated GpuVm<T>, so
> > it's not really a GpuVm<T> yet. Its destructor could call
> 
> Maybe you mean a pre-allocated GpuVmBo?

Yes that's what I meant.

> > drm_gpuvm_bo_destroy_not_in_lists(), but as the type is currently
> > private and never called anywhere, this perf optimization does not need
> > to happen now.
> > 
> > Pre-allocating and obtaining the gpuvm_bo object is exposed as a single
> > step. This could theoretically be a problem if one wanted to call
> > drm_gpuvm_bo_obtain_prealloc() during the fence signalling critical
> > path, but that's not a possibility because:
> > 
> > 1. Adding the BO to the extobj list requires the resv lock, so it cannot
> >   happen during the fence signalling critical path.
> > 2. obtain() requires that the BO is not in the extobj list, so obtain()
> >   must be called before adding the BO to the extobj list.
> > 
> > Thus, drm_gpuvm_bo_obtain_prealloc() cannot be called during the fence
> > signalling critical path. (For extobjs at least.)
> 
> What about internal objects? This is merely a sanity check from my side.

There are only two lists: extobj and evicted.

The extobj list is used to find the dma_resv locks of external objects.
This isn't required for internal ones, as they all share the resv lock
of the GPUVM itself.

> > +    #[inline]
> > +    fn raw_resv_lock(&self) -> *mut bindings::dma_resv {
> > +        // SAFETY: `r_obj` is immutable and valid for duration of GPUVM.
> > +        unsafe { (*(*self.as_raw()).r_obj).resv }
> > +    }
> 
> Shouldn’t we call this raw_resv? Adding “lock” to a name may incorrectly
> hint that the lock is actually taken.

Good call.

> > +    /// Custom function for allocating a `drm_gpuvm_bo`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Always safe to call.
> > +    // Unsafe to match function pointer type in C struct.
> 
> Is this missing an extra “/“ token? Also, I think this is a bit hard to parse initially.

Well, I didn't meant to include it in the docs. But I can reformat this
to be less confusing.

> > +    /// Look up whether there is an existing [`GpuVmBo`] for this gem object.
> > +    #[inline]
> > +    pub(super) fn obtain(self) -> GpuVmBoResident<T> {
> > +        let me = ManuallyDrop::new(self);
> > +        // SAFETY: Valid `drm_gpuvm_bo` not already in the lists.
> > +        let ptr = unsafe { bindings::drm_gpuvm_bo_obtain_prealloc(me.as_raw()) };
> > +
> > +        // If the vm_bo does not already exist, ensure that it's in the extobj list.
> 
> Wording is a bit off. Obviously only external objects should be in the extobj
> list. This is correctly captured by the check below, but the wording above
> makes it sound unconditional.

I'll update the comment. The "does not already exist" refers to the
`ptr::eq()` part of the condition, which checks whether the
pre-allocated vm_bo was created, or whether the GPUVM already has a
vm_bo for this GEM object.

> > +        if ptr::eq(ptr, me.as_raw()) && me.gpuvm().is_extobj(me.obj()) {
> > +            let resv_lock = me.gpuvm().raw_resv_lock();
> > +            // SAFETY: The GPUVM is still alive, so its resv lock is too.
> > +            unsafe { bindings::dma_resv_lock(resv_lock, ptr::null_mut()) };
> > +            // SAFETY: We hold the GPUVMs resv lock.
> > +            unsafe { bindings::drm_gpuvm_bo_extobj_add(ptr) };
> > +            // SAFETY: We took the lock, so we can unlock it.
> > +            unsafe { bindings::dma_resv_unlock(resv_lock) };
> > +        }
> > +
> > +        // INVARIANTS: Valid `drm_gpuvm_bo` in the GEM list.
> > +        // SAFETY: `drm_gpuvm_bo_obtain_prealloc` always returns a non-null ptr
> > +        GpuVmBoResident(unsafe { NonNull::new_unchecked(ptr.cast()) })
> > +    }
> > +}

> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Thanks for the reivew!

Alice