Message-Id: <C31C1A08-D626-48D6-8F8C-39209BF94B50@collabora.com>
Date: Wed, 21 Jan 2026 14:31:26 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Janne Grunau <j@...nau.net>,
 Matthew Brost <matthew.brost@...el.com>,
 Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
 Lyude Paul <lyude@...hat.com>,
 Asahi Lina <lina+kernel@...hilina.net>,
 dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 3/6] rust: gpuvm: add GpuVm::obtain()

Hi Alice,

This looks good. See a few nits below:

> On 21 Jan 2026, at 08:31, Alice Ryhl <aliceryhl@...gle.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@...gle.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@...labora.com>

Powered by blists - more mailing lists