Message-Id: <2B3FB52A-2E5E-4908-B196-F55ACB42CBD0@collabora.com>
Date: Fri, 5 Sep 2025 14:04:55 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 Asahi Lina <lina@...hilina.net>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Asahi Lina <lina+kernel@...hilina.net>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Wedson Almeida Filho <wedsonaf@...il.com>,
 Alyssa Rosenzweig <alyssa@...enzweig.io>
Subject: Re: [PATCH v3 10/14] rust: drm: gem: shmem: Add DRM shmem helper
 abstraction

Hi Lyude,

> On 29 Aug 2025, at 19:35, Lyude Paul <lyude@...hat.com> wrote:
> 
> From: Asahi Lina <lina@...hilina.net>
> 
> The DRM shmem helper includes common code useful for drivers which
> allocate GEM objects as anonymous shmem. Add a Rust abstraction for
> this. Drivers can choose the raw GEM implementation or the shmem layer,
> depending on their needs.
> 
> Signed-off-by: Asahi Lina <lina@...hilina.net>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> V2:
> * Use the drm_gem_shmem_init() and drm_gem_shmem_release() that I extracted
>  so we can handle memory allocation in rust, which means we no longer have
>  to handle freeing rust members of the struct by hand and have a closer
>  implementation to the main gem object
>  (this also gets rid of gem_create_object)
> * Get rid of GemObjectRef and UniqueGemObjectRef, we have ARef<T> at home.
> * Use Device<T::Driver> in Object<T>
> * Cleanup Object::<T>::new() a bit:
>  * Cleanup safety comment
>  * Use cast_mut()
> * Just import container_of!(), we use it all over anyhow
> * mut_shmem() -> as_shmem(), make it safe (there's no reason for being unsafe)
> * Remove any *const and *muts in structs, just use NonNull
> * Get rid of the previously hand-rolled sg_table bindings in shmem, use the
>  bindings from Abdiel's sg_table patch series
> * Add a TODO at the top about DMA reservation APIs and a desire for WwMutex
> * Get rid of map_wc() and replace it with a new ObjectConfig struct. While
>  it currently only specifies the map_wc flag, the idea here is that
>  settings like map_wc() and parent_resv_obj() shouldn't be exposed as
>  normal functions since the only place where it's safe to set them is
>  when we're still guaranteed unique access to the GEM object, e.g. before
>  returning it to the caller. Using a struct instead of individual
>  arguments here is mainly because we'll be adding at least one more
>  argument, and there's enough other gem shmem settings that trying to add
>  all of them as individual function arguments in the future would be a bit
>  messy.
> * Get rid of vm_numa_fields!, Lina didn't like this macro much either and I
>  think that it's fine for us to just specify the #[cfg(…)] attributes by
>  hand since we only need to do it twice.
> * Set drm_gem_object_funcs.vm_ops directly to drm_gem_shmem_vm_ops, don't
>  export the various shmem funcs. I'm not sure why this wasn't possible
>  before but it seems to work fine now.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> rust/bindings/bindings_helper.h |   2 +
> rust/helpers/drm.c              |  48 ++++-
> rust/kernel/drm/gem/mod.rs      |   3 +-
> rust/kernel/drm/gem/shmem.rs    | 311 ++++++++++++++++++++++++++++++++
> 4 files changed, 362 insertions(+), 2 deletions(-)
> create mode 100644 rust/kernel/drm/gem/shmem.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 00fe0449ed6de..e8386e3772376 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -33,6 +33,7 @@
> #include <drm/drm_drv.h>
> #include <drm/drm_file.h>
> #include <drm/drm_gem.h>
> +#include <drm/drm_gem_shmem_helper.h>
> #include <drm/drm_ioctl.h>
> #include <kunit/test.h>
> #include <linux/auxiliary_bus.h>
> @@ -56,6 +57,7 @@
> #include <linux/firmware.h>
> #include <linux/fs.h>
> #include <linux/ioport.h>
> +#include <linux/iosys-map.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> #include <linux/mdio.h>
> diff --git a/rust/helpers/drm.c b/rust/helpers/drm.c
> index 450b406c6f273..a4e997d0b4732 100644
> --- a/rust/helpers/drm.c
> +++ b/rust/helpers/drm.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> 
> #include <drm/drm_gem.h>
> +#include <drm/drm_gem_shmem_helper.h>
> #include <drm/drm_vma_manager.h>
> 
> #ifdef CONFIG_DRM
> @@ -20,4 +21,49 @@ __u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
> return drm_vma_node_offset_addr(node);
> }
> 
> -#endif
> +#ifdef CONFIG_DRM_GEM_SHMEM_HELPER
> +void rust_helper_drm_gem_shmem_object_free(struct drm_gem_object *obj)
> +{
> + return drm_gem_shmem_object_free(obj);
> +}
> +
> +void rust_helper_drm_gem_shmem_object_print_info(struct drm_printer *p, unsigned int indent,
> +                                                  const struct drm_gem_object *obj)
> +{
> + drm_gem_shmem_object_print_info(p, indent, obj);
> +}
> +
> +int rust_helper_drm_gem_shmem_object_pin(struct drm_gem_object *obj)
> +{
> + return drm_gem_shmem_object_pin(obj);
> +}
> +
> +void rust_helper_drm_gem_shmem_object_unpin(struct drm_gem_object *obj)
> +{
> + drm_gem_shmem_object_unpin(obj);
> +}
> +
> +struct sg_table *rust_helper_drm_gem_shmem_object_get_sg_table(struct drm_gem_object *obj)
> +{
> + return drm_gem_shmem_object_get_sg_table(obj);
> +}
> +
> +int rust_helper_drm_gem_shmem_object_vmap(struct drm_gem_object *obj,
> +                                           struct iosys_map *map)
> +{
> + return drm_gem_shmem_object_vmap(obj, map);
> +}
> +
> +void rust_helper_drm_gem_shmem_object_vunmap(struct drm_gem_object *obj,
> +                                              struct iosys_map *map)
> +{
> + drm_gem_shmem_object_vunmap(obj, map);
> +}
> +
> +int rust_helper_drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> + return drm_gem_shmem_object_mmap(obj, vma);
> +}
> +
> +#endif /* CONFIG_DRM_GEM_SHMEM_HELPER */
> +#endif /* CONFIG_DRM */
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index fe6ff3762a504..f9f9727f14e4a 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -3,6 +3,8 @@
> //! DRM GEM API
> //!
> //! C header: [`include/linux/drm/drm_gem.h`](srctree/include/linux/drm/drm_gem.h)
> +#[cfg(CONFIG_DRM_GEM_SHMEM_HELPER = "y")]
> +pub mod shmem;
> 
> use crate::{
>     alloc::flags::*,
> @@ -215,7 +217,6 @@ fn create_mmap_offset(&self) -> Result<u64> {
> impl<T: IntoGEMObject> BaseObject for T {}
> 
> /// Crate-private base operations shared by all GEM object classes.
> -#[expect(unused)]
> pub(crate) trait BaseObjectPrivate: IntoGEMObject {
>     /// Return a pointer to this object's dma_resv.
>     fn raw_dma_resv(&self) -> *mut bindings::dma_resv {
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> new file mode 100644
> index 0000000000000..6a8a392c3691b
> --- /dev/null
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! DRM GEM shmem helper objects
> +//!
> +//! C header: [`include/linux/drm/drm_gem_shmem_helper.h`](srctree/include/linux/drm/drm_gem_shmem_helper.h)
> +
> +// TODO:
> +// - There are a number of spots here that manually acquire/release the DMA reservation lock using
> +//   dma_resv_(un)lock(). In the future we should add support for ww mutex, expose a method to
> +//   acquire a reference to the WwMutex, and then use that directly instead of the C functions here.
> +
> +use crate::{
> +    container_of,
> +    drm::{device, driver, gem, private::Sealed},
> +    error::{from_err_ptr, to_result},
> +    prelude::*,
> +    scatterlist,
> +    types::{ARef, Opaque},
> +};
> +use core::{
> +    mem::MaybeUninit,
> +    ops::{Deref, DerefMut},
> +    ptr::{addr_of_mut, NonNull},
> +    slice,
> +};
> +use gem::{BaseObject, BaseObjectPrivate, DriverObject, IntoGEMObject, OpaqueObject};
> +
> +/// A struct for controlling the creation of shmem-backed GEM objects.
> +///
> +/// This is used with [`Object::new()`] to control various properties that can only be set when
> +/// initially creating a shmem-backed GEM object.
> +#[derive(Default)]
> +pub struct ObjectConfig<'a, T: DriverObject> {
> +    /// Whether to set the write-combine map flag.
> +    pub map_wc: bool,
> +
> +    /// Reuse the DMA reservation from another GEM object.
> +    ///
> +    /// The newly created [`Object`] will hold an owned refcount to `parent_resv_obj` if specified.
> +    pub parent_resv_obj: Option<&'a OpaqueObject<T::Driver>>,
> +}
> +
> +/// A shmem-backed GEM object.
> +///
> +/// # Invariants
> +///
> +/// The DRM core ensures that `dev` will remain valid for as long as the object.
> +#[repr(C)]
> +#[pin_data]
> +pub struct Object<T: DriverObject> {
> +    #[pin]
> +    obj: Opaque<bindings::drm_gem_shmem_object>,
> +    dev: NonNull<device::Device<T::Driver>>,
> +    // Parent object that owns this object's DMA reservation object
> +    parent_resv_obj: Option<ARef<OpaqueObject<T::Driver>>>,
> +    #[pin]
> +    inner: T,
> +}
> +
> +super::impl_as_opaque!(Object<T> where T: DriverObject);
> +
> +impl<T: DriverObject> Object<T> {
> +    /// `drm_gem_object_funcs` vtable suitable for GEM shmem objects.
> +    const VTABLE: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {
> +        free: Some(Self::free_callback),
> +        open: Some(super::open_callback::<T>),
> +        close: Some(super::close_callback::<T>),
> +        print_info: Some(bindings::drm_gem_shmem_object_print_info),
> +        export: None,
> +        pin: Some(bindings::drm_gem_shmem_object_pin),
> +        unpin: Some(bindings::drm_gem_shmem_object_unpin),
> +        get_sg_table: Some(bindings::drm_gem_shmem_object_get_sg_table),
> +        vmap: Some(bindings::drm_gem_shmem_object_vmap),
> +        vunmap: Some(bindings::drm_gem_shmem_object_vunmap),
> +        mmap: Some(bindings::drm_gem_shmem_object_mmap),
> +        status: None,
> +        rss: None,
> +        // SAFETY: `drm_gem_shmem_vm_ops` is static const on the C side, so immutable references are
> +        // safe here and such references shall be valid forever
> +        vm_ops: unsafe { &bindings::drm_gem_shmem_vm_ops },
> +        evict: None,
> +    };
> +
> +    /// Return a raw pointer to the embedded drm_gem_shmem_object.
> +    fn as_shmem(&self) -> *mut bindings::drm_gem_shmem_object {
> +        self.obj.get()
> +    }
> +
> +    /// Create a new shmem-backed DRM object of the given size.
> +    ///
> +    /// Additional config options can be specified using `config`.
> +    pub fn new(
> +        dev: &device::Device<T::Driver>,
> +        size: usize,
> +        config: ObjectConfig<'_, T>,
> +        args: T::Args,
> +    ) -> Result<ARef<Self>> {
> +        let new: Pin<KBox<Self>> = KBox::try_pin_init(
> +            try_pin_init!(Self {
> +                obj <- Opaque::init_zeroed(),
> +                dev: NonNull::from(dev),
> +                parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
> +                inner <- T::new(dev, size, args),
> +            }),
> +            GFP_KERNEL,
> +        )?;
> +
> +        // SAFETY: `obj.as_raw()` is guaranteed to be valid by the initialization above.
> +        unsafe { (*new.as_raw()).funcs = &Self::VTABLE };
> +
> +        // SAFETY: The arguments are all valid via the type invariants.
> +        to_result(unsafe { bindings::drm_gem_shmem_init(dev.as_raw(), new.as_shmem(), size) })?;
> +
> +        // SAFETY: We never move out of `self`.
> +        let new = KBox::into_raw(unsafe { Pin::into_inner_unchecked(new) });
> +
> +        // SAFETY: We're taking over the owned refcount from `drm_gem_shmem_init`.
> +        let obj = unsafe { ARef::from_raw(NonNull::new_unchecked(new)) };
> +
> +        // Start filling out values from `config`
> +        if let Some(parent_resv) = config.parent_resv_obj {
> +            // SAFETY: We have yet to expose the new gem object outside of this function, so it is
> +            // safe to modify this field.
> +            unsafe { (*obj.obj.get()).base.resv = parent_resv.raw_dma_resv() };
> +        }
> +
> +        // SAFETY: We have yet to expose this object outside of this function, so we're guaranteed
> +        // to have exclusive access - thus making this safe to hold a mutable reference to.
> +        let shmem = unsafe { &mut *obj.as_shmem() };
> +        shmem.set_map_wc(config.map_wc);
> +
> +        Ok(obj)
> +    }
> +
> +    /// Returns the `Device` that owns this GEM object.
> +    pub fn dev(&self) -> &device::Device<T::Driver> {
> +        // SAFETY: We are guaranteed that `dev` is valid for as long as this object is valid by our
> +        // type invariants
> +        unsafe { self.dev.as_ref() }
> +    }
> +
> +    extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
> +        // SAFETY:
> +        // - DRM always passes a valid gem object here
> +        // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
> +        //   `obj` is contained within a drm_gem_shmem_object
> +        let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
> +
> +        // SAFETY:
> +        // - We're in free_callback - so this function is safe to call.
> +        // - We won't be using the gem resources on `this` after this call.
> +        unsafe { bindings::drm_gem_shmem_release(this) };
> +
> +        // SAFETY:
> +        // - We verified above that `obj` is valid, which makes `this` valid
> +        // - This function is set in AllocOps, so we know that `this` is contained within a
> +        //   `Object<T>`
> +        let this = unsafe { container_of!(this.cast::<Opaque<_>>(), Self, obj) };

nit: having “_” here makes it a bit harder to see what is going on.

> +
> +        // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
> +        let _ = unsafe { KBox::from_raw(this) };
> +    }
> +
> +    /// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA
> +    /// pages for this object.
> +    ///
> +    /// This will pin the object in memory.
> +    #[inline]
> +    pub fn sg_table(&self) -> Result<&scatterlist::SGTable> {
> +        // SAFETY:
> +        // - drm_gem_shmem_get_pages_sgt is thread-safe.
> +        // - drm_gem_shmem_get_pages_sgt returns either a valid pointer to a scatterlist, or an
> +        //   error pointer.
> +        let sgt = from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(self.as_shmem()) })?;
> +
> +        // SAFETY: We checked above that `sgt` is not an error pointer, so it must be a valid
> +        // pointer to a scatterlist
> +        Ok(unsafe { scatterlist::SGTable::from_raw(sgt) })
> +    }
> +
> +    /// Creates and returns a virtual kernel memory mapping for this object.
> +    pub fn vmap(&self) -> Result<VMap<T>> {
> +        let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
> +
> +        // SAFETY:
> +        // - drm_gem_shmem_vmap can be called with the DMA reservation lock held
> +        // - Our ARef is proof that `obj` is safe to deref
> +        to_result(unsafe {
> +            // TODO: see top of file
> +            bindings::dma_resv_lock(self.raw_dma_resv(), core::ptr::null_mut());
> +            let ret = bindings::drm_gem_shmem_vmap_locked(self.as_shmem(), map.as_mut_ptr());
> +            bindings::dma_resv_unlock(self.raw_dma_resv());
> +            ret
> +        })?;
> +
> +        // SAFETY: if drm_gem_shmem_vmap did not fail, map is initialized now
> +        let map = unsafe { map.assume_init() };
> +
> +        Ok(VMap {
> +            map,
> +            owner: self.into(),
> +        })
> +    }
> +}
> +
> +impl<T: DriverObject> Deref for Object<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.inner
> +    }
> +}
> +
> +impl<T: DriverObject> DerefMut for Object<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.inner
> +    }
> +}
> +
> +impl<T: DriverObject> Sealed for Object<T> {}
> +
> +impl<T: DriverObject> gem::IntoGEMObject for Object<T> {
> +    fn as_raw(&self) -> *mut bindings::drm_gem_object {
> +        // SAFETY: Our immutable reference is proof that this is afe to dereference

Typo ^

> +        unsafe { addr_of_mut!((*self.obj.get()).base) }

&raw mut

> +    }
> +
> +    unsafe fn from_raw<'a>(obj: *mut bindings::drm_gem_object) -> &'a Object<T> {
> +        // SAFETY: The safety contract of from_gem_obj() guarantees that `obj` is contained within
> +        // `Self`
> +        unsafe {
> +            let obj: *mut Opaque<_> =

We should avoid “_” here.
 
> +                container_of!(obj, bindings::drm_gem_shmem_object, base).cast();
> +
> +            &*container_of!(obj, Object<T>, obj)
> +        }
> +    }
> +}
> +
> +impl<T: DriverObject> driver::AllocImpl for Object<T> {
> +    type Driver = T::Driver;
> +
> +    const ALLOC_OPS: driver::AllocOps = driver::AllocOps {
> +        gem_create_object: None,
> +        prime_handle_to_fd: None,
> +        prime_fd_to_handle: None,
> +        gem_prime_import: None,
> +        gem_prime_import_sg_table: Some(bindings::drm_gem_shmem_prime_import_sg_table),
> +        dumb_create: Some(bindings::drm_gem_shmem_dumb_create),
> +        dumb_map_offset: None,
> +    };
> +}
> +
> +/// A virtual mapping for a shmem-backed GEM object in kernel address space.
> +pub struct VMap<T: DriverObject> {
> +    map: bindings::iosys_map,
> +    owner: ARef<Object<T>>,
> +}
> +
> +impl<T: DriverObject> VMap<T> {
> +    /// Returns a const raw pointer to the start of the mapping.
> +    pub fn as_ptr(&self) -> *const core::ffi::c_void {
> +        // SAFETY: The shmem helpers always return non-iomem maps
> +        unsafe { self.map.__bindgen_anon_1.vaddr }
> +    }
> +
> +    /// Returns a mutable raw pointer to the start of the mapping.
> +    pub fn as_mut_ptr(&mut self) -> *mut core::ffi::c_void {
> +        // SAFETY: The shmem helpers always return non-iomem maps
> +        unsafe { self.map.__bindgen_anon_1.vaddr }
> +    }
> +
> +    /// Returns a byte slice view of the mapping.
> +    pub fn as_slice(&self) -> &[u8] {
> +        // SAFETY: The vmap maps valid memory up to the owner size
> +        unsafe { slice::from_raw_parts(self.as_ptr().cast(), self.owner.size()) }
> +    }

I think what we are seeing here is the same issue with the dma code.

If we are going to offer slices (which we definitely should!), then IMHO one of those should apply:

a) The functions are unsafe or,
b) There can only be one VMap object.

IIUC, it’s trivial to get two VMaps in the current code and then call
as_mut_slice() on each of them, which technically grants mutable access to the
same memory region.

> +
> +    /// Returns mutable a byte slice view of the mapping.
> +    pub fn as_mut_slice(&mut self) -> &mut [u8] {
> +        // SAFETY: The vmap maps valid memory up to the owner size
> +        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().cast(), self.owner.size()) }
> +    }
> +
> +    /// Borrows a reference to the object that owns this virtual mapping.
> +    pub fn owner(&self) -> &Object<T> {
> +        &self.owner
> +    }
> +}
> +
> +impl<T: DriverObject> Drop for VMap<T> {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - This function is safe to call with the DMA reservation lock held
> +        // - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to
> +        //   dereference.
> +        unsafe {
> +            let resv = self.owner.raw_dma_resv();
> +
> +            // TODO: see top of file

Note: Onur is working on ww_mutexes, and IMHO his latest patches [0] seem to be
converging towards the API we want . We should perhaps check whether his work
is a good fit here?

> +            bindings::dma_resv_lock(resv, core::ptr::null_mut());
> +            bindings::drm_gem_shmem_vunmap_locked(self.owner.as_shmem(), &mut self.map);
> +            bindings::dma_resv_unlock(resv);
> +        }
> +    }
> +}
> +
> +/// SAFETY: `iosys_map` objects are safe to send across threads.
> +unsafe impl<T: DriverObject> Send for VMap<T> {}
> +/// SAFETY: `iosys_map` objects are safe to send across threads.
> +unsafe impl<T: DriverObject> Sync for VMap<T> {}
> -- 
> 2.50.0
> 

[0]: https://lore.kernel.org/rust-for-linux/20250903131313.4365-1-work@onurozkan.dev/

Powered by blists - more mailing lists