Message-Id: <180E0324-383E-4813-8CD0-CA15EC3B3EFB@collabora.com>
Date: Thu, 22 Jan 2026 19:43:11 -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 5/6] rust: gpuvm: add GpuVmCore::sm_unmap()

Hi Alice,

> On 21 Jan 2026, at 08:31, Alice Ryhl <aliceryhl@...gle.com> wrote:
> 
> Add the entrypoint for unmapping ranges in the GPUVM, and provide
> callbacks and VA types for the implementation.
> 
> Co-developed-by: Asahi Lina <lina+kernel@...hilina.net>
> Signed-off-by: Asahi Lina <lina+kernel@...hilina.net>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> rust/kernel/drm/gpuvm/mod.rs    |  30 ++++-
> rust/kernel/drm/gpuvm/sm_ops.rs | 264 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/drm/gpuvm/va.rs     |   1 -
> rust/kernel/drm/gpuvm/vm_bo.rs  |   8 ++
> 4 files changed, 298 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
> index 2179ddd717d8728bbe231bd94ea7b5d1e2652543..165a25666ccc3d62e59b73483d4eedff044423e9 100644
> --- a/rust/kernel/drm/gpuvm/mod.rs
> +++ b/rust/kernel/drm/gpuvm/mod.rs
> @@ -18,6 +18,7 @@
>     bindings,
>     drm,
>     drm::gem::IntoGEMObject,
> +    error::to_result,
>     prelude::*,
>     sync::aref::{
>         ARef,
> @@ -28,6 +29,7 @@
> 
> use core::{
>     cell::UnsafeCell,
> +    marker::PhantomData,
>     mem::{
>         ManuallyDrop,
>         MaybeUninit, //
> @@ -43,12 +45,15 @@
>     }, //
> };
> 
> -mod va;
> -pub use self::va::*;
> +mod sm_ops;
> +pub use self::sm_ops::*;
> 
> mod vm_bo;
> pub use self::vm_bo::*;
> 
> +mod va;
> +pub use self::va::*;
> +
> /// A DRM GPU VA manager.
> ///
> /// This object is refcounted, but the "core" is only accessible using a special unique handle. The
> @@ -89,8 +94,8 @@ const fn vtable() -> &'static bindings::drm_gpuvm_ops {
>             vm_bo_free: GpuVmBo::<T>::FREE_FN,
>             vm_bo_validate: None,
>             sm_step_map: None,
> -            sm_step_unmap: None,
> -            sm_step_remap: None,
> +            sm_step_unmap: Some(Self::sm_step_unmap),
> +            sm_step_remap: Some(Self::sm_step_remap),
>         }
>     }
> 
> @@ -239,6 +244,23 @@ pub trait DriverGpuVm: Sized {
> 
>     /// Data stored with each `struct drm_gpuvm_bo`.
>     type VmBoData;
> +
> +    /// The private data passed to callbacks.
> +    type SmContext<'ctx>;

As I said, this lifetime fixed an issue that Deborah was having. Thanks a lot!

> +
> +    /// Indicates that an existing mapping should be removed.
> +    fn sm_step_unmap<'op, 'ctx>(
> +        &mut self,
> +        op: OpUnmap<'op, Self>,
> +        context: &mut Self::SmContext<'ctx>,
> +    ) -> Result<OpUnmapped<'op, Self>, Error>;
> +
> +    /// Indicates that an existing mapping should be split up.
> +    fn sm_step_remap<'op, 'ctx>(
> +        &mut self,
> +        op: OpRemap<'op, Self>,
> +        context: &mut Self::SmContext<'ctx>,
> +    ) -> Result<OpRemapped<'op, Self>, Error>;
> }
> 
> /// The core of the DRM GPU VA manager.
> diff --git a/rust/kernel/drm/gpuvm/sm_ops.rs b/rust/kernel/drm/gpuvm/sm_ops.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..3c29d10d63f0b0a1976c714a86d486948ba81a15
> --- /dev/null
> +++ b/rust/kernel/drm/gpuvm/sm_ops.rs
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +use super::*;
> +
> +/// The actual data that gets threaded through the callbacks.
> +struct SmData<'a, 'ctx, T: DriverGpuVm> {
> +    gpuvm: &'a mut GpuVmCore<T>,
> +    user_context: &'a mut T::SmContext<'ctx>,
> +}
> +
> +/// Represents an `sm_step_unmap` operation that has not yet been completed.
> +pub struct OpUnmap<'op, T: DriverGpuVm> {
> +    op: &'op bindings::drm_gpuva_op_unmap,
> +    _invariant: PhantomData<*mut &'op mut T>,

Would have been cool to explain why we have a pointer in this PhantomData.

Same elsewhere, IMHO. Helps with maintainability in the future.

(To be honest, I’m not really sure what’s going on here..)


> +}
> +
> +impl<'op, T: DriverGpuVm> OpUnmap<'op, T> {
> +    /// Indicates whether this `drm_gpuva` is physically contiguous with the
> +    /// original mapping request.
> +    ///
> +    /// Optionally, if `keep` is set, drivers may keep the actual page table
> +    /// mappings for this `drm_gpuva`, adding the missing page table entries
> +    /// only and update the `drm_gpuvm` accordingly.
> +    pub fn keep(&self) -> bool {
> +        self.op.keep
> +    }
> +
> +    /// The range being unmapped.
> +    pub fn va(&self) -> &GpuVa<T> {
> +        // SAFETY: This is a valid va.
> +        unsafe { GpuVa::<T>::from_raw(self.op.va) }
> +    }
> +
> +    /// Remove the VA.
> +    pub fn remove(self) -> (OpUnmapped<'op, T>, GpuVaRemoved<T>) {
> +        // SAFETY: The op references a valid drm_gpuva in the GPUVM.
> +        unsafe { bindings::drm_gpuva_unmap(self.op) };
> +        // SAFETY: The va is no longer in the interval tree so we may unlink it.
> +        unsafe { bindings::drm_gpuva_unlink_defer(self.op.va) };
> +
> +        // SAFETY: We just removed this va from the `GpuVm<T>`.
> +        let va = unsafe { GpuVaRemoved::from_raw(self.op.va) };
> +
> +        (
> +            OpUnmapped {
> +                _invariant: self._invariant,
> +            },
> +            va,
> +        )
> +    }
> +}
> +
> +/// Represents a completed [`OpUnmap`] operation.
> +pub struct OpUnmapped<'op, T> {
> +    _invariant: PhantomData<*mut &'op mut T>,
> +}
> +
> +/// Represents an `sm_step_remap` operation that has not yet been completed.
> +pub struct OpRemap<'op, T: DriverGpuVm> {
> +    op: &'op bindings::drm_gpuva_op_remap,
> +    _invariant: PhantomData<*mut &'op mut T>,
> +}
> +
> +impl<'op, T: DriverGpuVm> OpRemap<'op, T> {
> +    /// The preceding part of a split mapping.
> +    #[inline]
> +    pub fn prev(&self) -> Option<&OpRemapMapData> {
> +        // SAFETY: We checked for null, so the pointer must be valid.
> +        NonNull::new(self.op.prev).map(|ptr| unsafe { OpRemapMapData::from_raw(ptr) })
> +    }
> +
> +    /// The subsequent part of a split mapping.
> +    #[inline]
> +    pub fn next(&self) -> Option<&OpRemapMapData> {
> +        // SAFETY: We checked for null, so the pointer must be valid.
> +        NonNull::new(self.op.next).map(|ptr| unsafe { OpRemapMapData::from_raw(ptr) })
> +    }
> +
> +    /// Indicates whether the `drm_gpuva` being removed is physically contiguous with the original
> +    /// mapping request.

We should probably link to the Rust VA type using [`GpuVa`] or something? Nbd either way.

> +    ///
> +    /// Optionally, if `keep` is set, drivers may keep the actual page table mappings for this
> +    /// `drm_gpuva`, adding the missing page table entries only and update the `drm_gpuvm`
> +    /// accordingly.
> +    #[inline]
> +    pub fn keep(&self) -> bool {
> +        // SAFETY: The unmap pointer is always valid.
> +        unsafe { (*self.op.unmap).keep }
> +    }
> +
> +    /// The range being unmapped.
> +    #[inline]
> +    pub fn va_to_unmap(&self) -> &GpuVa<T> {
> +        // SAFETY: This is a valid va.
> +        unsafe { GpuVa::<T>::from_raw((*self.op.unmap).va) }
> +    }
> +
> +    /// The [`drm_gem_object`](crate::gem::Object) whose VA is being remapped.
> +    #[inline]
> +    pub fn obj(&self) -> &T::Object {
> +        self.va_to_unmap().obj()
> +    }
> +
> +    /// The [`GpuVmBo`] that is being remapped.
> +    #[inline]
> +    pub fn vm_bo(&self) -> &GpuVmBo<T> {
> +        self.va_to_unmap().vm_bo()
> +    }
> +
> +    /// Update the GPUVM to perform the remapping.
> +    pub fn remap(
> +        self,
> +        va_alloc: [GpuVaAlloc<T>; 2],
> +        prev_data: impl PinInit<T::VaData>,
> +        next_data: impl PinInit<T::VaData>,
> +    ) -> (OpRemapped<'op, T>, OpRemapRet<T>) {
> +        let [va1, va2] = va_alloc;
> +
> +        let mut unused_va = None;
> +        let mut prev_ptr = ptr::null_mut();
> +        let mut next_ptr = ptr::null_mut();
> +        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);
> +        }
> +
> +        // SAFETY: the pointers are non-null when required
> +        unsafe { bindings::drm_gpuva_remap(prev_ptr, next_ptr, self.op) };
> +
> +        let gpuva_guard = self.vm_bo().lock_gpuva();
> +        if !prev_ptr.is_null() {
> +            // SAFETY: The prev_ptr is a valid drm_gpuva prepared for insertion. The vm_bo is still
> +            // valid as the not-yet-unlinked gpuva holds a refcount on the vm_bo.
> +            unsafe { bindings::drm_gpuva_link(prev_ptr, self.vm_bo().as_raw()) };
> +        }
> +        if !next_ptr.is_null() {
> +            // SAFETY: The next_ptr is a valid drm_gpuva prepared for insertion. The vm_bo is still
> +            // valid as the not-yet-unlinked gpuva holds a refcount on the vm_bo.
> +            unsafe { bindings::drm_gpuva_link(next_ptr, self.vm_bo().as_raw()) };
> +        }
> +        drop(gpuva_guard);
> +
> +        // SAFETY: The va is no longer in the interval tree so we may unlink it.
> +        unsafe { bindings::drm_gpuva_unlink_defer((*self.op.unmap).va) };
> +
> +        (
> +            OpRemapped {
> +                _invariant: self._invariant,
> +            },
> +            OpRemapRet {
> +                // SAFETY: We just removed this va from the `GpuVm<T>`.
> +                unmapped_va: unsafe { GpuVaRemoved::from_raw((*self.op.unmap).va) },
> +                unused_va,
> +            },
> +        )
> +    }
> +}
> +
> +/// Part of an [`OpRemap`] that represents a new mapping.
> +#[repr(transparent)]
> +pub struct OpRemapMapData(bindings::drm_gpuva_op_map);
> +
> +impl OpRemapMapData {
> +    /// # Safety
> +    /// Must reference a valid `drm_gpuva_op_map` for duration of `'a`.
> +    unsafe fn from_raw<'a>(ptr: NonNull<bindings::drm_gpuva_op_map>) -> &'a Self {
> +        // SAFETY: ok per safety requirements
> +        unsafe { ptr.cast().as_ref() }
> +    }
> +
> +    /// The base address of the new mapping.
> +    pub fn addr(&self) -> u64 {
> +        self.0.va.addr
> +    }
> +
> +    /// The length of the new mapping.
> +    pub fn length(&self) -> u64 {
> +        self.0.va.range
> +    }
> +
> +    /// The offset within the [`drm_gem_object`](crate::gem::Object).
> +    pub fn gem_offset(&self) -> u64 {
> +        self.0.gem.offset
> +    }
> +}
> +
> +/// Struct containing objects removed or not used by [`OpRemap::remap`].
> +pub struct OpRemapRet<T: DriverGpuVm> {
> +    /// The `drm_gpuva` that was removed.
> +    pub unmapped_va: GpuVaRemoved<T>,
> +    /// If the remap did not split the region into two pieces, then the unused `drm_gpuva` is
> +    /// returned here.
> +    pub unused_va: Option<GpuVaAlloc<T>>,
> +}
> +
> +/// Represents a completed [`OpRemap`] operation.
> +pub struct OpRemapped<'op, T> {
> +    _invariant: PhantomData<*mut &'op mut T>,
> +}
> +
> +impl<T: DriverGpuVm> GpuVmCore<T> {
> +    /// Remove any mappings in the given region.
> +    ///
> +    /// Internally calls [`DriverGpuVm::sm_step_unmap`] for ranges entirely contained within the
> +    /// given range, and [`DriverGpuVm::sm_step_remap`] for ranges that overlap with the range.
> +    #[inline]
> +    pub fn sm_unmap(&mut self, addr: u64, length: u64, context: &mut T::SmContext<'_>) -> Result {
> +        let gpuvm = self.as_raw();
> +        let mut p = SmData {
> +            gpuvm: self,
> +            user_context: context,
> +        };
> +        // SAFETY:
> +        // * raw_request() creates a valid request.
> +        // * The private data is valid to be interpreted as SmData.
> +        to_result(unsafe { bindings::drm_gpuvm_sm_unmap(gpuvm, (&raw mut p).cast(), addr, length) })
> +    }
> +}
> +
> +impl<T: DriverGpuVm> GpuVm<T> {
> +    /// # Safety
> +    /// Must be called from `sm_unmap` with a pointer to `SmData`.
> +    pub(super) unsafe extern "C" fn sm_step_unmap(
> +        op: *mut bindings::drm_gpuva_op,
> +        p: *mut c_void,
> +    ) -> c_int {
> +        // SAFETY: The caller provides a pointer to `SmData`.
> +        let p = unsafe { &mut *p.cast::<SmData<'_, '_, T>>() };
> +        let op = OpUnmap {
> +            // SAFETY: sm_step_unmap is called with an unmap operation.
> +            op: unsafe { &(*op).__bindgen_anon_1.unmap },
> +            _invariant: PhantomData,
> +        };
> +        match p.gpuvm.data().sm_step_unmap(op, p.user_context) {
> +            Ok(OpUnmapped { .. }) => 0,
> +            Err(err) => err.to_errno(),
> +        }
> +    }
> +
> +    /// # Safety
> +    /// Must be called from `sm_unmap` with a pointer to `SmData`.
> +    pub(super) unsafe extern "C" fn sm_step_remap(
> +        op: *mut bindings::drm_gpuva_op,
> +        p: *mut c_void,
> +    ) -> c_int {
> +        // SAFETY: The caller provides a pointer to `SmData`.
> +        let p = unsafe { &mut *p.cast::<SmData<'_, '_, T>>() };
> +        let op = OpRemap {
> +            // SAFETY: sm_step_remap is called with a remap operation.
> +            op: unsafe { &(*op).__bindgen_anon_1.remap },
> +            _invariant: PhantomData,
> +        };
> +        match p.gpuvm.data().sm_step_remap(op, p.user_context) {
> +            Ok(OpRemapped { .. }) => 0,
> +            Err(err) => err.to_errno(),
> +        }
> +    }
> +}
> diff --git a/rust/kernel/drm/gpuvm/va.rs b/rust/kernel/drm/gpuvm/va.rs
> index c96796a6b2c8c7c4b5475324562968ca0f07fd09..a31122ff22282186a1d76d4bb085714f6465722b 100644
> --- a/rust/kernel/drm/gpuvm/va.rs
> +++ b/rust/kernel/drm/gpuvm/va.rs
> @@ -1,6 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0 OR MIT
> 
> -#![expect(dead_code)]
> use super::*;
> 
> /// Represents that a range of a GEM object is mapped in this [`GpuVm`] instance.
> diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
> index 310fa569b5bd43f0f872ff859b3624377b93d651..f600dfb15379233111b5893f6aa85a12e6c9e131 100644
> --- a/rust/kernel/drm/gpuvm/vm_bo.rs
> +++ b/rust/kernel/drm/gpuvm/vm_bo.rs
> @@ -101,6 +101,14 @@ pub fn obj(&self) -> &T::Object {
>     pub fn data(&self) -> &T::VmBoData {
>         &self.data
>     }
> +
> +    pub(super) fn lock_gpuva(&self) -> crate::sync::MutexGuard<'_, ()> {
> +        // SAFETY: The GEM object is valid.
> +        let ptr = unsafe { &raw mut (*self.obj().as_raw()).gpuva.lock };
> +        // SAFETY: The GEM object is valid, so the mutex is properly initialized.
> +        let mutex = unsafe { crate::sync::Mutex::from_raw(ptr) };
> +        mutex.lock()
> +    }
> }
> 
> /// A pre-allocated [`GpuVmBo`] object.
> 
> -- 
> 2.52.0.457.g6b5491de43-goog
> 
> 


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

Powered by blists - more mailing lists