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

Alice Ryhl posted 6 patches 2 weeks, 3 days ago
There is a newer version of this series
[PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Alice Ryhl 2 weeks, 3 days ago
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.

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
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.)

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/drm/gpuvm/mod.rs   |  32 +++++-
 rust/kernel/drm/gpuvm/vm_bo.rs | 219 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 248 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
index 81b5e767885d8258c44086444b153c91961ffabc..cb576a7ffa07bc108704e008b7f87de52a837930 100644
--- a/rust/kernel/drm/gpuvm/mod.rs
+++ b/rust/kernel/drm/gpuvm/mod.rs
@@ -25,13 +25,20 @@
 
 use core::{
     cell::UnsafeCell,
+    mem::ManuallyDrop,
     ops::{
         Deref,
         Range, //
     },
-    ptr::NonNull, //
+    ptr::{
+        self,
+        NonNull, //
+    }, //
 };
 
+mod vm_bo;
+pub use self::vm_bo::*;
+
 /// A DRM GPU VA manager.
 ///
 /// This object is refcounted, but the "core" is only accessible using a special unique handle. The
@@ -68,8 +75,8 @@ const fn vtable() -> &'static bindings::drm_gpuvm_ops {
             vm_free: Some(Self::vm_free),
             op_alloc: None,
             op_free: None,
-            vm_bo_alloc: None,
-            vm_bo_free: None,
+            vm_bo_alloc: GpuVmBo::<T>::ALLOC_FN,
+            vm_bo_free: GpuVmBo::<T>::FREE_FN,
             vm_bo_validate: None,
             sm_step_map: None,
             sm_step_unmap: None,
@@ -166,6 +173,16 @@ pub fn va_range(&self) -> Range<u64> {
         Range { start, end }
     }
 
+    /// Get or create the [`GpuVmBo`] for this gem object.
+    #[inline]
+    pub fn obtain(
+        &self,
+        obj: &T::Object,
+        data: impl PinInit<T::VmBoData>,
+    ) -> Result<GpuVmBoResident<T>, AllocError> {
+        Ok(GpuVmBoAlloc::new(self, obj, data)?.obtain())
+    }
+
     /// Clean up buffer objects that are no longer used.
     #[inline]
     pub fn deferred_cleanup(&self) {
@@ -191,6 +208,12 @@ pub fn is_extobj(&self, obj: &T::Object) -> bool {
         // SAFETY: By type invariants we can free it when refcount hits zero.
         drop(unsafe { KBox::from_raw(me) })
     }
+
+    #[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 }
+    }
 }
 
 /// The manager for a GPUVM.
@@ -200,6 +223,9 @@ pub trait DriverGpuVm: Sized {
 
     /// The kind of GEM object stored in this GPUVM.
     type Object: IntoGEMObject;
+
+    /// Data stored with each `struct drm_gpuvm_bo`.
+    type VmBoData;
 }
 
 /// The core of the DRM GPU VA manager.
diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
new file mode 100644
index 0000000000000000000000000000000000000000..310fa569b5bd43f0f872ff859b3624377b93d651
--- /dev/null
+++ b/rust/kernel/drm/gpuvm/vm_bo.rs
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+use super::*;
+
+/// Represents that a given GEM object has at least one mapping on this [`GpuVm`] instance.
+///
+/// Does not assume that GEM lock is held.
+#[repr(C)]
+#[pin_data]
+pub struct GpuVmBo<T: DriverGpuVm> {
+    #[pin]
+    inner: Opaque<bindings::drm_gpuvm_bo>,
+    #[pin]
+    data: T::VmBoData,
+}
+
+impl<T: DriverGpuVm> GpuVmBo<T> {
+    pub(super) const ALLOC_FN: Option<unsafe extern "C" fn() -> *mut bindings::drm_gpuvm_bo> = {
+        use core::alloc::Layout;
+        let base = Layout::new::<bindings::drm_gpuvm_bo>();
+        let rust = Layout::new::<Self>();
+        assert!(base.size() <= rust.size());
+        if base.size() != rust.size() || base.align() != rust.align() {
+            Some(Self::vm_bo_alloc)
+        } else {
+            // This causes GPUVM to allocate a `GpuVmBo<T>` with `kzalloc(sizeof(drm_gpuvm_bo))`.
+            None
+        }
+    };
+
+    pub(super) const FREE_FN: Option<unsafe extern "C" fn(*mut bindings::drm_gpuvm_bo)> = {
+        if core::mem::needs_drop::<Self>() {
+            Some(Self::vm_bo_free)
+        } else {
+            // This causes GPUVM to free a `GpuVmBo<T>` with `kfree`.
+            None
+        }
+    };
+
+    /// Custom function for allocating a `drm_gpuvm_bo`.
+    ///
+    /// # Safety
+    ///
+    /// Always safe to call.
+    // Unsafe to match function pointer type in C struct.
+    unsafe extern "C" fn vm_bo_alloc() -> *mut bindings::drm_gpuvm_bo {
+        KBox::<Self>::new_uninit(GFP_KERNEL | __GFP_ZERO)
+            .map(KBox::into_raw)
+            .unwrap_or(ptr::null_mut())
+            .cast()
+    }
+
+    /// Custom function for freeing a `drm_gpuvm_bo`.
+    ///
+    /// # Safety
+    ///
+    /// The pointer must have been allocated with [`GpuVmBo::ALLOC_FN`], and must not be used after
+    /// this call.
+    unsafe extern "C" fn vm_bo_free(ptr: *mut bindings::drm_gpuvm_bo) {
+        // SAFETY:
+        // * The ptr was allocated from kmalloc with the layout of `GpuVmBo<T>`.
+        // * `ptr->inner` has no destructor.
+        // * `ptr->data` contains a valid `T::VmBoData` that we can drop.
+        drop(unsafe { KBox::<Self>::from_raw(ptr.cast()) });
+    }
+
+    /// Access this [`GpuVmBo`] from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of `'a`, the pointer must reference a valid `drm_gpuvm_bo` associated with
+    /// a [`GpuVm<T>`].
+    #[inline]
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm_bo) -> &'a Self {
+        // SAFETY: `drm_gpuvm_bo` is first field and `repr(C)`.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Returns a raw pointer to underlying C value.
+    #[inline]
+    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
+        self.inner.get()
+    }
+
+    /// The [`GpuVm`] that this GEM object is mapped in.
+    #[inline]
+    pub fn gpuvm(&self) -> &GpuVm<T> {
+        // SAFETY: The `obj` pointer is guaranteed to be valid.
+        unsafe { GpuVm::<T>::from_raw((*self.inner.get()).vm) }
+    }
+
+    /// The [`drm_gem_object`](crate::gem::Object) for these mappings.
+    #[inline]
+    pub fn obj(&self) -> &T::Object {
+        // SAFETY: The `obj` pointer is guaranteed to be valid.
+        unsafe { <T::Object as IntoGEMObject>::from_raw((*self.inner.get()).obj) }
+    }
+
+    /// The driver data with this buffer object.
+    #[inline]
+    pub fn data(&self) -> &T::VmBoData {
+        &self.data
+    }
+}
+
+/// A pre-allocated [`GpuVmBo`] object.
+///
+/// # Invariants
+///
+/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData`, has a refcount of one, and is
+/// absent from any gem, extobj, or evict lists.
+pub(super) struct GpuVmBoAlloc<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
+
+impl<T: DriverGpuVm> GpuVmBoAlloc<T> {
+    /// Create a new pre-allocated [`GpuVmBo`].
+    ///
+    /// It's intentional that the initializer is infallible because `drm_gpuvm_bo_put` will call
+    /// drop on the data, so we don't have a way to free it when the data is missing.
+    #[inline]
+    pub(super) fn new(
+        gpuvm: &GpuVm<T>,
+        gem: &T::Object,
+        value: impl PinInit<T::VmBoData>,
+    ) -> Result<GpuVmBoAlloc<T>, AllocError> {
+        // CAST: `GpuVmBoAlloc::vm_bo_alloc` ensures that this memory was allocated with the layout
+        // of `GpuVmBo<T>`. The type is repr(C), so `container_of` is not required.
+        // SAFETY: The provided gpuvm and gem ptrs are valid for the duration of this call.
+        let raw_ptr = unsafe {
+            bindings::drm_gpuvm_bo_create(gpuvm.as_raw(), gem.as_raw()).cast::<GpuVmBo<T>>()
+        };
+        let ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
+        // SAFETY: `ptr->data` is a valid pinned location.
+        let Ok(()) = unsafe { value.__pinned_init(&raw mut (*raw_ptr).data) };
+        // INVARIANTS: We just created the vm_bo so it's absent from lists, and the data is valid
+        // as we just initialized it.
+        Ok(GpuVmBoAlloc(ptr))
+    }
+
+    /// Returns a raw pointer to underlying C value.
+    #[inline]
+    pub(super) fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
+        // SAFETY: The pointer references a valid `drm_gpuvm_bo`.
+        unsafe { (*self.0.as_ptr()).inner.get() }
+    }
+
+    /// 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.
+        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()) })
+    }
+}
+
+impl<T: DriverGpuVm> Deref for GpuVmBoAlloc<T> {
+    type Target = GpuVmBo<T>;
+    #[inline]
+    fn deref(&self) -> &GpuVmBo<T> {
+        // SAFETY: By the type invariants we may deref while `Self` exists.
+        unsafe { self.0.as_ref() }
+    }
+}
+
+impl<T: DriverGpuVm> Drop for GpuVmBoAlloc<T> {
+    #[inline]
+    fn drop(&mut self) {
+        // TODO: Call drm_gpuvm_bo_destroy_not_in_lists() directly.
+        // SAFETY: It's safe to perform a deferred put in any context.
+        unsafe { bindings::drm_gpuvm_bo_put_deferred(self.as_raw()) };
+    }
+}
+
+/// A [`GpuVmBo`] object in the GEM list.
+///
+/// # Invariants
+///
+/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
+pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
+
+impl<T: DriverGpuVm> GpuVmBoResident<T> {
+    /// Returns a raw pointer to underlying C value.
+    #[inline]
+    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
+        // SAFETY: The pointer references a valid `drm_gpuvm_bo`.
+        unsafe { (*self.0.as_ptr()).inner.get() }
+    }
+}
+
+impl<T: DriverGpuVm> Deref for GpuVmBoResident<T> {
+    type Target = GpuVmBo<T>;
+    #[inline]
+    fn deref(&self) -> &GpuVmBo<T> {
+        // SAFETY: By the type invariants we may deref while `Self` exists.
+        unsafe { self.0.as_ref() }
+    }
+}
+
+impl<T: DriverGpuVm> Drop for GpuVmBoResident<T> {
+    #[inline]
+    fn drop(&mut self) {
+        // SAFETY: It's safe to perform a deferred put in any context.
+        unsafe { bindings::drm_gpuvm_bo_put_deferred(self.as_raw()) };
+    }
+}

-- 
2.52.0.457.g6b5491de43-goog
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Boris Brezillon 1 week, 5 days ago
On Wed, 21 Jan 2026 11:31:19 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> +/// A [`GpuVmBo`] object in the GEM list.
> +///
> +/// # Invariants
> +///
> +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
> +pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);

I find the name a bit confusing: BO residency is often used to refer to
memory backing the buffer object, and in this case, you can end up with
a GpuVmBoResident being returned for a BO that has been evicted (one
that's no longer resident).

> +
> +impl<T: DriverGpuVm> GpuVmBoResident<T> {
> +    /// Returns a raw pointer to underlying C value.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
> +        // SAFETY: The pointer references a valid `drm_gpuvm_bo`.
> +        unsafe { (*self.0.as_ptr()).inner.get() }
> +    }
> +}
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Alice Ryhl 1 week, 5 days ago
On Mon, Jan 26, 2026 at 4:00 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Wed, 21 Jan 2026 11:31:19 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > +/// A [`GpuVmBo`] object in the GEM list.
> > +///
> > +/// # Invariants
> > +///
> > +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
> > +pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
>
> I find the name a bit confusing: BO residency is often used to refer to
> memory backing the buffer object, and in this case, you can end up with
> a GpuVmBoResident being returned for a BO that has been evicted (one
> that's no longer resident).

Good point. I meant it as "present in list" but I guess there are
other things a gpuvm may be present in.

Any naming suggestions?

Alice
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Boris Brezillon 1 week, 5 days ago
On Mon, 26 Jan 2026 16:07:30 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Jan 26, 2026 at 4:00 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Wed, 21 Jan 2026 11:31:19 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >  
> > > +/// A [`GpuVmBo`] object in the GEM list.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
> > > +pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);  
> >
> > I find the name a bit confusing: BO residency is often used to refer to
> > memory backing the buffer object, and in this case, you can end up with
> > a GpuVmBoResident being returned for a BO that has been evicted (one
> > that's no longer resident).  
> 
> Good point. I meant it as "present in list" but I guess there are
> other things a gpuvm may be present in.
> 
> Any naming suggestions?

Valid, Bound, Present, Active?
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Danilo Krummrich 1 week, 5 days ago
On Mon Jan 26, 2026 at 4:35 PM CET, Boris Brezillon wrote:
> On Mon, 26 Jan 2026 16:07:30 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Mon, Jan 26, 2026 at 4:00 PM Boris Brezillon
>> <boris.brezillon@collabora.com> wrote:
>> >
>> > On Wed, 21 Jan 2026 11:31:19 +0000
>> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >  
>> > > +/// A [`GpuVmBo`] object in the GEM list.
>> > > +///
>> > > +/// # Invariants
>> > > +///
>> > > +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
>> > > +pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);  
>> >
>> > I find the name a bit confusing: BO residency is often used to refer to
>> > memory backing the buffer object, and in this case, you can end up with
>> > a GpuVmBoResident being returned for a BO that has been evicted (one
>> > that's no longer resident).  
>> 
>> Good point. I meant it as "present in list" but I guess there are
>> other things a gpuvm may be present in.
>> 
>> Any naming suggestions?
>
> Valid, Bound, Present, Active?

I still have to catch up on this series, but quick drive-by comment: I'd go for
'Registered'.
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Daniel Almeida 2 weeks, 3 days ago
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.

> 
> 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?

> 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.

> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/drm/gpuvm/mod.rs   |  32 +++++-
> rust/kernel/drm/gpuvm/vm_bo.rs | 219 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 248 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
> index 81b5e767885d8258c44086444b153c91961ffabc..cb576a7ffa07bc108704e008b7f87de52a837930 100644
> --- a/rust/kernel/drm/gpuvm/mod.rs
> +++ b/rust/kernel/drm/gpuvm/mod.rs
> @@ -25,13 +25,20 @@
> 
> use core::{
>     cell::UnsafeCell,
> +    mem::ManuallyDrop,
>     ops::{
>         Deref,
>         Range, //
>     },
> -    ptr::NonNull, //
> +    ptr::{
> +        self,
> +        NonNull, //
> +    }, //
> };
> 
> +mod vm_bo;
> +pub use self::vm_bo::*;
> +
> /// A DRM GPU VA manager.
> ///
> /// This object is refcounted, but the "core" is only accessible using a special unique handle. The
> @@ -68,8 +75,8 @@ const fn vtable() -> &'static bindings::drm_gpuvm_ops {
>             vm_free: Some(Self::vm_free),
>             op_alloc: None,
>             op_free: None,
> -            vm_bo_alloc: None,
> -            vm_bo_free: None,
> +            vm_bo_alloc: GpuVmBo::<T>::ALLOC_FN,
> +            vm_bo_free: GpuVmBo::<T>::FREE_FN,
>             vm_bo_validate: None,
>             sm_step_map: None,
>             sm_step_unmap: None,
> @@ -166,6 +173,16 @@ pub fn va_range(&self) -> Range<u64> {
>         Range { start, end }
>     }
> 
> +    /// Get or create the [`GpuVmBo`] for this gem object.
> +    #[inline]
> +    pub fn obtain(
> +        &self,
> +        obj: &T::Object,
> +        data: impl PinInit<T::VmBoData>,
> +    ) -> Result<GpuVmBoResident<T>, AllocError> {
> +        Ok(GpuVmBoAlloc::new(self, obj, data)?.obtain())
> +    }
> +
>     /// Clean up buffer objects that are no longer used.
>     #[inline]
>     pub fn deferred_cleanup(&self) {
> @@ -191,6 +208,12 @@ pub fn is_extobj(&self, obj: &T::Object) -> bool {
>         // SAFETY: By type invariants we can free it when refcount hits zero.
>         drop(unsafe { KBox::from_raw(me) })
>     }
> +
> +    #[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.

> }
> 
> /// The manager for a GPUVM.
> @@ -200,6 +223,9 @@ pub trait DriverGpuVm: Sized {
> 
>     /// The kind of GEM object stored in this GPUVM.
>     type Object: IntoGEMObject;
> +
> +    /// Data stored with each `struct drm_gpuvm_bo`.
> +    type VmBoData;
> }
> 
> /// The core of the DRM GPU VA manager.
> diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..310fa569b5bd43f0f872ff859b3624377b93d651
> --- /dev/null
> +++ b/rust/kernel/drm/gpuvm/vm_bo.rs
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +use super::*;
> +
> +/// Represents that a given GEM object has at least one mapping on this [`GpuVm`] instance.
> +///
> +/// Does not assume that GEM lock is held.
> +#[repr(C)]
> +#[pin_data]
> +pub struct GpuVmBo<T: DriverGpuVm> {
> +    #[pin]
> +    inner: Opaque<bindings::drm_gpuvm_bo>,
> +    #[pin]
> +    data: T::VmBoData,
> +}
> +
> +impl<T: DriverGpuVm> GpuVmBo<T> {
> +    pub(super) const ALLOC_FN: Option<unsafe extern "C" fn() -> *mut bindings::drm_gpuvm_bo> = {
> +        use core::alloc::Layout;
> +        let base = Layout::new::<bindings::drm_gpuvm_bo>();
> +        let rust = Layout::new::<Self>();
> +        assert!(base.size() <= rust.size());
> +        if base.size() != rust.size() || base.align() != rust.align() {
> +            Some(Self::vm_bo_alloc)
> +        } else {
> +            // This causes GPUVM to allocate a `GpuVmBo<T>` with `kzalloc(sizeof(drm_gpuvm_bo))`.
> +            None
> +        }
> +    };
> +
> +    pub(super) const FREE_FN: Option<unsafe extern "C" fn(*mut bindings::drm_gpuvm_bo)> = {
> +        if core::mem::needs_drop::<Self>() {
> +            Some(Self::vm_bo_free)
> +        } else {
> +            // This causes GPUVM to free a `GpuVmBo<T>` with `kfree`.
> +            None
> +        }
> +    };
> +
> +    /// 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.

> +    unsafe extern "C" fn vm_bo_alloc() -> *mut bindings::drm_gpuvm_bo {
> +        KBox::<Self>::new_uninit(GFP_KERNEL | __GFP_ZERO)
> +            .map(KBox::into_raw)
> +            .unwrap_or(ptr::null_mut())
> +            .cast()
> +    }
> +
> +    /// Custom function for freeing a `drm_gpuvm_bo`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must have been allocated with [`GpuVmBo::ALLOC_FN`], and must not be used after
> +    /// this call.
> +    unsafe extern "C" fn vm_bo_free(ptr: *mut bindings::drm_gpuvm_bo) {
> +        // SAFETY:
> +        // * The ptr was allocated from kmalloc with the layout of `GpuVmBo<T>`.
> +        // * `ptr->inner` has no destructor.
> +        // * `ptr->data` contains a valid `T::VmBoData` that we can drop.
> +        drop(unsafe { KBox::<Self>::from_raw(ptr.cast()) });
> +    }
> +
> +    /// Access this [`GpuVmBo`] from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of `'a`, the pointer must reference a valid `drm_gpuvm_bo` associated with
> +    /// a [`GpuVm<T>`].
> +    #[inline]
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm_bo) -> &'a Self {
> +        // SAFETY: `drm_gpuvm_bo` is first field and `repr(C)`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Returns a raw pointer to underlying C value.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
> +        self.inner.get()
> +    }
> +
> +    /// The [`GpuVm`] that this GEM object is mapped in.
> +    #[inline]
> +    pub fn gpuvm(&self) -> &GpuVm<T> {
> +        // SAFETY: The `obj` pointer is guaranteed to be valid.
> +        unsafe { GpuVm::<T>::from_raw((*self.inner.get()).vm) }
> +    }
> +
> +    /// The [`drm_gem_object`](crate::gem::Object) for these mappings.
> +    #[inline]
> +    pub fn obj(&self) -> &T::Object {
> +        // SAFETY: The `obj` pointer is guaranteed to be valid.
> +        unsafe { <T::Object as IntoGEMObject>::from_raw((*self.inner.get()).obj) }
> +    }
> +
> +    /// The driver data with this buffer object.
> +    #[inline]
> +    pub fn data(&self) -> &T::VmBoData {
> +        &self.data
> +    }
> +}
> +
> +/// A pre-allocated [`GpuVmBo`] object.
> +///
> +/// # Invariants
> +///
> +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData`, has a refcount of one, and is
> +/// absent from any gem, extobj, or evict lists.
> +pub(super) struct GpuVmBoAlloc<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
> +
> +impl<T: DriverGpuVm> GpuVmBoAlloc<T> {
> +    /// Create a new pre-allocated [`GpuVmBo`].
> +    ///
> +    /// It's intentional that the initializer is infallible because `drm_gpuvm_bo_put` will call
> +    /// drop on the data, so we don't have a way to free it when the data is missing.
> +    #[inline]
> +    pub(super) fn new(
> +        gpuvm: &GpuVm<T>,
> +        gem: &T::Object,
> +        value: impl PinInit<T::VmBoData>,
> +    ) -> Result<GpuVmBoAlloc<T>, AllocError> {
> +        // CAST: `GpuVmBoAlloc::vm_bo_alloc` ensures that this memory was allocated with the layout
> +        // of `GpuVmBo<T>`. The type is repr(C), so `container_of` is not required.
> +        // SAFETY: The provided gpuvm and gem ptrs are valid for the duration of this call.
> +        let raw_ptr = unsafe {
> +            bindings::drm_gpuvm_bo_create(gpuvm.as_raw(), gem.as_raw()).cast::<GpuVmBo<T>>()
> +        };
> +        let ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
> +        // SAFETY: `ptr->data` is a valid pinned location.
> +        let Ok(()) = unsafe { value.__pinned_init(&raw mut (*raw_ptr).data) };
> +        // INVARIANTS: We just created the vm_bo so it's absent from lists, and the data is valid
> +        // as we just initialized it.
> +        Ok(GpuVmBoAlloc(ptr))
> +    }
> +
> +    /// Returns a raw pointer to underlying C value.
> +    #[inline]
> +    pub(super) fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
> +        // SAFETY: The pointer references a valid `drm_gpuvm_bo`.
> +        unsafe { (*self.0.as_ptr()).inner.get() }
> +    }
> +
> +    /// 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.

> +        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()) })
> +    }
> +}
> +
> +impl<T: DriverGpuVm> Deref for GpuVmBoAlloc<T> {
> +    type Target = GpuVmBo<T>;
> +    #[inline]
> +    fn deref(&self) -> &GpuVmBo<T> {
> +        // SAFETY: By the type invariants we may deref while `Self` exists.
> +        unsafe { self.0.as_ref() }
> +    }
> +}
> +
> +impl<T: DriverGpuVm> Drop for GpuVmBoAlloc<T> {
> +    #[inline]
> +    fn drop(&mut self) {
> +        // TODO: Call drm_gpuvm_bo_destroy_not_in_lists() directly.
> +        // SAFETY: It's safe to perform a deferred put in any context.
> +        unsafe { bindings::drm_gpuvm_bo_put_deferred(self.as_raw()) };
> +    }
> +}
> +
> +/// A [`GpuVmBo`] object in the GEM list.
> +///
> +/// # Invariants
> +///
> +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is present in the gem list.
> +pub struct GpuVmBoResident<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
> +
> +impl<T: DriverGpuVm> GpuVmBoResident<T> {
> +    /// Returns a raw pointer to underlying C value.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
> +        // SAFETY: The pointer references a valid `drm_gpuvm_bo`.
> +        unsafe { (*self.0.as_ptr()).inner.get() }
> +    }
> +}
> +
> +impl<T: DriverGpuVm> Deref for GpuVmBoResident<T> {
> +    type Target = GpuVmBo<T>;
> +    #[inline]
> +    fn deref(&self) -> &GpuVmBo<T> {
> +        // SAFETY: By the type invariants we may deref while `Self` exists.
> +        unsafe { self.0.as_ref() }
> +    }
> +}
> +
> +impl<T: DriverGpuVm> Drop for GpuVmBoResident<T> {
> +    #[inline]
> +    fn drop(&mut self) {
> +        // SAFETY: It's safe to perform a deferred put in any context.
> +        unsafe { bindings::drm_gpuvm_bo_put_deferred(self.as_raw()) };
> +    }
> +}
> 
> -- 
> 2.52.0.457.g6b5491de43-goog
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()
Posted by Alice Ryhl 2 weeks, 2 days ago
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