Rust bindings for gem shmem + iosys_map

In the future we're going to be introducing more GEM object types in rust then just gem::Object<T>. Since all types of GEM objects have refcounting, let's introduce a macro that we can use in the gem crate in order to copy this boilerplate implementation for each type: impl_aref_for_gem_obj!().

Signed-off-by Lyude Paul <[email protected]>

---
V5:
* Move .as_raw() call to let obj in dec_ref, to ensure that the reference to object is not live by the time that we call drm_gem_object_put().
* Add missing #[macro_export] annotation

rust/kernel/drm/gem/mod.rs | 54 +++++++++++++++++++++++++++-----------

1 file changed, 39 insertions(+), 15 deletions(-)

rust/kernel/drm/gem/mod.rs +0 -0

index 20c2769a8c9d6..32bff2e8463f4 100644
@@ -15,6 +15,44 @@
 };
 use core::{ops::Deref, ptr::NonNull};
 

rust/kernel/drm/gem/mod.rs +16 -15

+            }
+
+            unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
+                // SAFETY: `obj` is a valid pointer to an `Object<T>`.
+                let obj = unsafe { obj.as_ref() }.as_raw();
+
+                // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+                unsafe { bindings::drm_gem_object_put(obj) };
+            }
+        }
+    };
+}
+
+pub(crate) use impl_aref_for_gem_obj;
+
 /// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its
 /// [`DriverObject`] implementation.
 ///

@@ -253,21 +291,7 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
    }
 }
 
-// SAFETY: Instances of `Object<T>` are always reference-counted.
-unsafe impl<T: DriverObject> crate::types::AlwaysRefCounted for Object<T> {
-    fn inc_ref(&self) {
-        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
-        unsafe { bindings::drm_gem_object_get(self.as_raw()) };
-    }
-
-    unsafe fn dec_ref(obj: NonNull<Self>) {
-        // SAFETY: `obj` is a valid pointer to an `Object<T>`.
-        let obj = unsafe { obj.as_ref() };
-
-        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
-        unsafe { bindings::drm_gem_object_put(obj.as_raw()) }
-    }
-}
+impl_aref_for_gem_obj!(impl<T> for Object<T> where T: DriverObject);
 
 impl<T: DriverObject> super::private::Sealed for Object<T> {}
 

Signature · 1 line

2.51.0

This is an associated type that may be used in order to specify a data-type to pass to gem objects when construction them, allowing for drivers to more easily initialize their private-data for gem objects.

Signed-off-by Lyude Paul <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

---
V3:
* s/BaseDriverObject/DriverObject/
V4:
* Fix leftover reference to BaseObjectDriver in rustdoc for DriverObject::Args

 drivers/gpu/drm/nova/gem.rs |  5 +++--
 rust/kernel/drm/gem/mod.rs  | 13 ++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/gpu/drm/nova/gem.rs +0 -0

index 2760ba4f3450b..173077eeb2def 100644

drivers/gpu/drm/nova/gem.rs +1 -1

@@ -33,7 +34,7 @@ pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self
             return Err(EINVAL);
         }
 
-        gem::Object::new(dev, aligned_size)
+        gem::Object::new(dev, aligned_size, ())
     }
 
     /// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.

rust/kernel/drm/gem/mod.rs +10 -3

index 67813cfb0db42..d448c65fe5e13 100644
@@ -65,8 +65,15 @@ pub trait DriverObject: Sync + Send + Sized {
     /// Parent `Driver` for this object.
     type Driver: drm::Driver;
 
+    /// The data type to use for passing arguments to [`DriverObject::new`].
+    type Args;
+
     /// Create a new driver data object for a GEM object of a given size.
-    fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>;
+    fn new(
+        dev: &drm::Device<Self::Driver>,
+        size: usize,
+        args: Self::Args,
+    ) -> impl PinInit<Self, Error>;
 
     /// Open a new handle to an existing object, associated with a File.
     fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) -> Result {

@@ -247,11 +254,11 @@ impl<T: DriverObject> Object<T> {
     };
 
     /// Create a new GEM object.
-    pub fn new(dev: &drm::Device<T::Driver>, size: usize) -> Result<ARef<Self>> {
+    pub fn new(dev: &drm::Device<T::Driver>, size: usize, args: T::Args) -> Result<ARef<Self>> {
         let obj: Pin<KBox<Self>> = KBox::pin_init(
             try_pin_init!(Self {
                 obj: Opaque::new(bindings::drm_gem_object::default()),
-                data <- T::new(dev, size),
+                data <- T::new(dev, size, args),
                 // INVARIANT: The drm subsystem guarantees that the `struct drm_device` will live
                 // as long as the GEM object lives.
                 dev: dev.into(),

Signature · 1 line

2.51.0

This is an associated type that may be used in order to specify a data-type to pass to gem objects when construction them, allowing for drivers to more easily initialize their private-data for gem objects.

Signed-off-by Lyude Paul <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

---
V3:
* s/BaseDriverObject/DriverObject/
V4:
* Fix leftover reference to BaseObjectDriver in rustdoc for DriverObject::Args

drivers/gpu/drm/nova/gem.rs | 5 +++--
rust/kernel/drm/gem/mod.rs | 13 ++++++++++---

2 files changed, 13 insertions(+), 5 deletions(-)

drivers/gpu/drm/nova/gem.rs +3 -2

index 2760ba4f3450b..173077eeb2def 100644
@@ -18,8 +18,9 @@ pub(crate) struct NovaObject {}
 
 impl gem::DriverObject for NovaObject {
    type Driver = NovaDriver;
+    type Args = ();
 
-    fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
+    fn new(_dev: &NovaDevice, _size: usize, _args: Self::Args) -> impl PinInit<Self, Error> {
        try_pin_init!(NovaObject {})
    }
 }

@@ -33,7 +34,7 @@ pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self
            return Err(EINVAL);
        }
 
-        gem::Object::new(dev, aligned_size)
+        gem::Object::new(dev, aligned_size, ())
    }
 
    /// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.

rust/kernel/drm/gem/mod.rs +10 -3

index 67813cfb0db42..d448c65fe5e13 100644
@@ -65,8 +65,15 @@ pub trait DriverObject: Sync + Send + Sized {
    /// Parent `Driver` for this object.
    type Driver: drm::Driver;
 
+    /// The data type to use for passing arguments to [`DriverObject::new`].
+    type Args;
+
    /// Create a new driver data object for a GEM object of a given size.
-    fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>;
+    fn new(
+        dev: &drm::Device<Self::Driver>,
+        size: usize,
+        args: Self::Args,
+    ) -> impl PinInit<Self, Error>;
 
    /// Open a new handle to an existing object, associated with a File.
    fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) -> Result {

@@ -247,11 +254,11 @@ impl<T: DriverObject> Object<T> {
    };
 
    /// Create a new GEM object.
-    pub fn new(dev: &drm::Device<T::Driver>, size: usize) -> Result<ARef<Self>> {
+    pub fn new(dev: &drm::Device<T::Driver>, size: usize, args: T::Args) -> Result<ARef<Self>> {
        let obj: Pin<KBox<Self>> = KBox::pin_init(
            try_pin_init!(Self {
                obj: Opaque::new(bindings::drm_gem_object::default()),
-                data <- T::new(dev, size),
+                data <- T::new(dev, size, args),
                // INVARIANT: The drm subsystem guarantees that the `struct drm_device` will live
                // as long as the GEM object lives.
                dev: dev.into(),

Signature · 1 line

2.51.0

From: Asahi Lina <[email protected]>

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 <[email protected]>

Signed-off-by Daniel Almeida <[email protected]>

Signed-off-by Lyude Paul <[email protected]>

---

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.
V4:
* Switch from OpaqueObject to a normal Object<T> now that we've removed it
* Remove dev from Object, it's redundant as drm_gem_object already has a device pointer we can use.
* Use &raw instead of addr_of!()
V5:
* Fix typo in shmem::Object::as_raw()
* Add type invariant around obj always being a valid drm_gem_shmem_object for the duration of the lifetime of Object.
* Use Opaque::cast_from() instead of unrestricted casts

rust/bindings/bindings_helper.h |   2 +
rust/helpers/drm.c              |  48 ++++++-
rust/kernel/drm/gem/mod.rs      |   3 +-
rust/kernel/drm/gem/shmem.rs    | 225 ++++++++++++++++++++++++++++++++
4 files changed, 276 insertions(+), 2 deletions(-)

create mode 100644 rust/kernel/drm/gem/shmem.rs

rust/bindings/bindings_helper.h +2 -0

index 07f79e125c329..ad644f3d62cc3 100644
@@ -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>

@@ -60,6 +61,7 @@
 #include <linux/interrupt.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>

rust/helpers/drm.c +47 -1

index 450b406c6f273..a4e997d0b4732 100644
@@ -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 */

rust/kernel/drm/gem/mod.rs +2 -1

index d448c65fe5e13..2985dfa144027 100644
@@ -3,6 +3,8 @@
 //! DRM GEM API
 //!
 //! C header: [`include/drm/drm_gem.h`](srctree/include/drm/drm_gem.h)
+#[cfg(CONFIG_DRM_GEM_SHMEM_HELPER = "y")]
+pub mod shmem;
 
 use crate::{
    alloc::flags::*,

@@ -208,7 +210,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 {

rust/kernel/drm/gem/shmem.rs new file +166 -0

new file mode 100644
index 0000000000000..45b95d60a3ec7
@@ -0,0 +1,225 @@
+// 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::{
+    ops::{Deref, DerefMut},
+    ptr::NonNull,
+};
+use gem::{BaseObjectPrivate, DriverObject, IntoGEMObject};
+
+/// 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 Object<T>>,
+}
+
+/// A shmem-backed GEM object.
+///
+/// # Invariants
+///
+/// `obj` contains a valid initialized `struct drm_gem_shmem_object` for the lifetime of this
+/// object.
+#[repr(C)]
+#[pin_data]
+pub struct Object<T: DriverObject> {
+    #[pin]
+    obj: Opaque<bindings::drm_gem_shmem_object>,
+    // Parent object that owns this object's DMA reservation object
+    parent_resv_obj: Option<ARef<Object<T>>>,
+    #[pin]
+    inner: T,
+}
+
+super::impl_aref_for_gem_obj!(impl<T> for 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(),
+                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: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`.
+        unsafe { device::Device::from_raw((*self.as_raw()).dev) }
+    }
+
+    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!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+
+        // 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:

rust/kernel/drm/gem/shmem.rs new file +49 -0

+
+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 safe to dereference.
+        // - `obj` is always a valid drm_gem_shmem_object via our type invariants.
+        unsafe { &raw mut (*self.obj.get()).base }
+    }
+
+    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 = Opaque::cast_from(container_of!(obj, bindings::drm_gem_shmem_object, base));
+
+            &*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,
+    };
+}

Signature · 1 line

2.51.0

Hi Lyude,

+cc Sami Tolvanen

This patch looks fine, but I think we should either introduce splittable vmaps now, or make sure that they can be fully implementable later. Do you think that's the case?

Quoted 342 lines · Lyude Paul

From: Asahi Lina <[email protected]>

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 <[email protected]>

Signed-off-by Daniel Almeida <[email protected]>

Signed-off-by Lyude Paul <[email protected]>

---

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.
V4:
* Switch from OpaqueObject to a normal Object<T> now that we've removed it
* Remove dev from Object, it's redundant as drm_gem_object already has a device pointer we can use.
* Use &raw instead of addr_of!()
V5:
* Fix typo in shmem::Object::as_raw()
* Add type invariant around obj always being a valid drm_gem_shmem_object for the duration of the lifetime of Object.
* Use Opaque::cast_from() instead of unrestricted casts

rust/bindings/bindings_helper.h |   2 +
rust/helpers/drm.c              |  48 ++++++-
rust/kernel/drm/gem/mod.rs      |   3 +-
rust/kernel/drm/gem/shmem.rs    | 225 ++++++++++++++++++++++++++++++++
4 files changed, 276 insertions(+), 2 deletions(-)

create mode 100644 rust/kernel/drm/gem/shmem.rs

rust/bindings/bindings_helper.h +2 -0

index 07f79e125c329..ad644f3d62cc3 100644
@@ -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>

@@ -60,6 +61,7 @@
 #include <linux/interrupt.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>

rust/helpers/drm.c +47 -1

index 450b406c6f273..a4e997d0b4732 100644
@@ -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 */

rust/kernel/drm/gem/mod.rs +2 -1

index d448c65fe5e13..2985dfa144027 100644
@@ -3,6 +3,8 @@
 //! DRM GEM API
 //!
 //! C header: [`include/drm/drm_gem.h`](srctree/include/drm/drm_gem.h)
+#[cfg(CONFIG_DRM_GEM_SHMEM_HELPER = "y")]
+pub mod shmem;
 
 use crate::{
    alloc::flags::*,

@@ -208,7 +210,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 {

rust/kernel/drm/gem/shmem.rs new file +166 -0

new file mode 100644
index 0000000000000..45b95d60a3ec7
@@ -0,0 +1,225 @@
+// 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::{
+    ops::{Deref, DerefMut},
+    ptr::NonNull,
+};
+use gem::{BaseObjectPrivate, DriverObject, IntoGEMObject};
+
+/// 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 Object<T>>,
+}
+
+/// A shmem-backed GEM object.
+///
+/// # Invariants
+///
+/// `obj` contains a valid initialized `struct drm_gem_shmem_object` for the lifetime of this
+/// object.
+#[repr(C)]
+#[pin_data]
+pub struct Object<T: DriverObject> {
+    #[pin]
+    obj: Opaque<bindings::drm_gem_shmem_object>,
+    // Parent object that owns this object's DMA reservation object
+    parent_resv_obj: Option<ARef<Object<T>>>,
+    #[pin]
+    inner: T,
+}
+
+super::impl_aref_for_gem_obj!(impl<T> for 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(),
+                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: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`.
+        unsafe { device::Device::from_raw((*self.as_raw()).dev) }
+    }
+
+    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!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+
+        // 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:

rust/kernel/drm/gem/shmem.rs line 176

Quoted 10 lines

rust/kernel/drm/gem/shmem.rs new file +10 -0

+        // - 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) })
+    }
+}

^ Here, for example: would it be ok to return the sg_table for the whole allocation, even though we might have multiple vmapped regions?

I assume so, since these are two distinct concepts, but it’s worth confirming anyways.

Quoted 53 lines

rust/kernel/drm/gem/shmem.rs new file +49 -0

+
+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 safe to dereference.
+        // - `obj` is always a valid drm_gem_shmem_object via our type invariants.
+        unsafe { &raw mut (*self.obj.get()).base }
+    }
+
+    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 = Opaque::cast_from(container_of!(obj, bindings::drm_gem_shmem_object, base));
+
+            &*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,
+    };
+}

Signature · 1 line

2.51.0

Currently we expose the ability to retrieve an SGTable for an shmem gem object using gem::shmem::Object::<T>::sg_table(). However, this only gives us a borrowed reference. This being said - retrieving an SGTable is a fallible operation, and as such it's reasonable that a driver may want to hold onto an SGTable for longer then a reference would allow in order to avoid having to deal with fallibility every time they want to access the SGTable. One such driver with this usecase is the Asahi driver.

So to support this, let's introduce shmem::SGTable - which both holds a pointer to the SGTable and a reference to its respective GEM object in order to keep the GEM object alive for as long as the shmem::SGTable. The type can be used identically to a normal SGTable.

Signed-off-by Lyude Paul <[email protected]>

---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the SGTable abstractions now has a Owned and Borrowed variant, I think renaming this to shmem::SGTable makes things less confusing. We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is thread-safe.
* Rename from SGTableRef to SGTable

rust/kernel/drm/gem/shmem.rs | 50 ++++++++++++++++++++++++++++++++++++

1 file changed, 50 insertions(+)

rust/kernel/drm/gem/shmem.rs +50 -0

index 45b95d60a3ec7..21ccb6c1824be 100644
@@ -173,6 +173,25 @@ pub fn sg_table(&self) -> Result<&scatterlist::SGTable> {
        // pointer to a scatterlist
        Ok(unsafe { scatterlist::SGTable::from_raw(sgt) })
    }
+
+    /// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages
+    /// for this object.
+    ///
+    /// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an
+    /// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a
+    /// reference to an [`scatterlist::SGTable`].
+    ///
+    /// This will pin the object in memory.
+    ///
+    /// [`shmem::SGTable`]: SGTable
+    pub fn owned_sg_table(&self) -> Result<SGTable<T>> {
+        Ok(SGTable {
+            sgt: self.sg_table()?.into(),
+            // INVARIANT: We take an owned refcount to `self` here, ensuring that `sgt` remains
+            // valid for as long as this `SGTable`.
+            _owner: self.into(),
+        })
+    }
 }
 
 impl<T: DriverObject> Deref for Object<T> {

@@ -223,3 +242,34 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
        dumb_map_offset: None,
    };
 }
+
+/// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object.
+///
+/// This object holds an owned reference to the underlying GEM shmem object, ensuring that the
+/// [`scatterlist::SGTable`] referenced by this type remains valid for the lifetime of this object.
+///
+/// # Invariants
+///
+/// - `sgt` is kept alive by `_owner`, ensuring it remains valid for as long as `Self`.
+/// - `sgt` corresponds to the owned object in `_owner`.
+/// - This object is only exposed in situations where we know the underlying `SGTable` will not be
+///   modified for the lifetime of this object. Thus, it is safe to send/access this type across
+///   threads.
+pub struct SGTable<T: DriverObject> {
+    sgt: NonNull<scatterlist::SGTable>,
+    _owner: ARef<Object<T>>,
+}
+
+// SAFETY: This object is thread-safe via our type invariants.
+unsafe impl<T: DriverObject> Send for SGTable<T> {}
+// SAFETY: This object is thread-safe via our type invariants.
+unsafe impl<T: DriverObject> Sync for SGTable<T> {}
+
+impl<T: DriverObject> Deref for SGTable<T> {
+    type Target = scatterlist::SGTable;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: Creating an immutable reference to this is safe via our type invariants.
+        unsafe { self.sgt.as_ref() }
+    }
+}

Signature · 1 line

2.51.0