Re: [PATCH v4 1/6] rust: drm: add base GPUVM immediate mode abstraction

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Jan 30, 2026 at 3:24 PM CET, Alice Ryhl wrote:
> +/// A DRM GPU VA manager.
> +///
> +/// This object is refcounted, but the "core" is only accessible using a special unique handle. The
> +/// core consists of the `core` field and the GPUVM's interval tree.

I think this is still a bit confusing, I think we should just rename GpuVmCore
to UniqueGpuVm and rewrite this to something like:

"The driver specific data of of `GpuVm` is only accessible through a
[`UniqueGpuVm`], which guarantees exclusive access."

> +/// # Invariants
> +///
> +/// * Stored in an allocation managed by the refcount in `self.vm`.
> +/// * Access to `data` and the gpuvm interval tree is controlled via the [`GpuVmCore`] type.
> +#[pin_data]
> +pub struct GpuVm<T: DriverGpuVm> {
> +    #[pin]
> +    vm: Opaque<bindings::drm_gpuvm>,
> +    /// Accessed only through the [`GpuVmCore`] reference.
> +    data: UnsafeCell<T>,
> +}
> +
> +// SAFETY: By type invariants, the allocation is managed by the refcount in `self.vm`.
> +unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVm<T> {
> +    fn inc_ref(&self) {
> +        // SAFETY: By type invariants, the allocation is managed by the refcount in `self.vm`.
> +        unsafe { bindings::drm_gpuvm_get(self.vm.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: By type invariants, the allocation is managed by the refcount in `self.vm`.
> +        unsafe { bindings::drm_gpuvm_put((*obj.as_ptr()).vm.get()) };
> +    }
> +}
> +
> +impl<T: DriverGpuVm> GpuVm<T> {
> +    const fn vtable() -> &'static bindings::drm_gpuvm_ops {
> +        &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_validate: None,
> +            sm_step_map: None,
> +            sm_step_unmap: None,
> +            sm_step_remap: None,
> +        }
> +    }
> +
> +    /// Creates a GPUVM instance.
> +    #[expect(clippy::new_ret_no_self)]
> +    pub fn new<E>(
> +        name: &'static CStr,
> +        dev: &drm::Device<T::Driver>,
> +        r_obj: &T::Object,
> +        range: Range<u64>,
> +        reserve_range: Range<u64>,
> +        data: T,

Let's be flexibile and also accept an impl PinInit<T, E> instead.

> +    ) -> Result<GpuVmCore<T>, E>
> +    where
> +        E: From<AllocError>,
> +        E: From<core::convert::Infallible>,
> +    {
> +        let obj = KBox::try_pin_init::<E>(
> +            try_pin_init!(Self {
> +                data: UnsafeCell::new(data),
> +                vm <- Opaque::ffi_init(|vm| {
> +                    // SAFETY: These arguments are valid. `vm` is valid until refcount drops to
> +                    // zero.
> +                    unsafe {
> +                        bindings::drm_gpuvm_init(
> +                            vm,
> +                            name.as_char_ptr(),
> +                            bindings::drm_gpuvm_flags_DRM_GPUVM_IMMEDIATE_MODE
> +                                | bindings::drm_gpuvm_flags_DRM_GPUVM_RESV_PROTECTED,
> +                            dev.as_raw(),
> +                            r_obj.as_raw(),
> +                            range.start,
> +                            range.end - range.start,
> +                            reserve_range.start,
> +                            reserve_range.end - reserve_range.start,
> +                            const { Self::vtable() },
> +                        )
> +                    }
> +                }),
> +            }? E),
> +            GFP_KERNEL,
> +        )?;
> +        // SAFETY: This transfers the initial refcount to the ARef.
> +        Ok(GpuVmCore(unsafe {
> +            ARef::from_raw(NonNull::new_unchecked(KBox::into_raw(
> +                Pin::into_inner_unchecked(obj),
> +            )))
> +        }))
> +    }
> +
> +    /// Access this [`GpuVm`] from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must reference the `struct drm_gpuvm` in a valid [`GpuVm<T>`] that remains
> +    /// valid for at least `'a`.
> +    #[inline]
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm) -> &'a Self {
> +        // SAFETY: Caller passes a pointer to the `drm_gpuvm` in a `GpuVm<T>`. Caller ensures the
> +        // pointer is valid for 'a.
> +        unsafe { &*kernel::container_of!(Opaque::cast_from(ptr), Self, vm) }

I'd pull the Opaque::cast_from() call out of the unsafe block.

> +    }
> +
> +    /// Returns a raw pointer to the embedded `struct drm_gpuvm`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::drm_gpuvm {
> +        self.vm.get()
> +    }
> +
> +    /// The start of the VA space.
> +    #[inline]
> +    pub fn va_start(&self) -> u64 {
> +        // SAFETY: The `mm_start` field is immutable.
> +        unsafe { (*self.as_raw()).mm_start }
> +    }
> +
> +    /// The length of the GPU's virtual address space.
> +    #[inline]
> +    pub fn va_length(&self) -> u64 {
> +        // SAFETY: The `mm_range` field is immutable.
> +        unsafe { (*self.as_raw()).mm_range }
> +    }
> +
> +    /// Returns the range of the GPU virtual address space.
> +    #[inline]
> +    pub fn va_range(&self) -> Range<u64> {
> +        let start = self.va_start();
> +        // OVERFLOW: This reconstructs the Range<u64> passed to the constructor, so it won't fail.
> +        let end = start + self.va_length();
> +        Range { start, end }
> +    }
> +
> +    /// Clean up buffer objects that are no longer used.
> +    #[inline]
> +    pub fn deferred_cleanup(&self) {
> +        // SAFETY: This GPUVM uses immediate mode.
> +        unsafe { bindings::drm_gpuvm_bo_deferred_cleanup(self.as_raw()) }
> +    }
> +
> +    /// Check if this GEM object is an external object for this GPUVM.
> +    #[inline]
> +    pub fn is_extobj(&self, obj: &T::Object) -> bool {
> +        // SAFETY: We may call this with any GPUVM and GEM object.
> +        unsafe { bindings::drm_gpuvm_is_extobj(self.as_raw(), obj.as_raw()) }
> +    }
> +
> +    /// Free this GPUVM.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Called when refcount hits zero.
> +    unsafe extern "C" fn vm_free(me: *mut bindings::drm_gpuvm) {
> +        // SAFETY: Caller passes a pointer to the `drm_gpuvm` in a `GpuVm<T>`.
> +        let me = unsafe { kernel::container_of!(Opaque::cast_from(me), Self, vm).cast_mut() };
> +        // SAFETY: By type invariants we can free it when refcount hits zero.
> +        drop(unsafe { KBox::from_raw(me) })
> +    }
> +}
> +
> +/// The manager for a GPUVM.

This description seems a bit odd. In the end, the trait makes GPUVM aware of
other driver specific types. So, maybe a better name would be
gpuvm::DriverAttributes, gpuvm::DriverTypes, gpuvm::DriverInfo or just
gpuvm::Driver. My favorite is gpuvm::DriverInfo.

We should also change the doc-comment accordingly. Maybe somthing like: "This
trait make the [`GpuVm`] aware of the other driver specific DRM types."

> +pub trait DriverGpuVm: Sized {
> +    /// Parent `Driver` for this object.
> +    type Driver: drm::Driver<Object = Self::Object>;
> +
> +    /// The kind of GEM object stored in this GPUVM.
> +    type Object: IntoGEMObject;
> +}