Re: [PATCH v10 4/5] rust: drm: gem: Introduce shmem::SGTable

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Apr 9, 2026 at 9:12 AM JST, Lyude Paul wrote:
> In order to do this, we need to be careful to ensure that any interface we
> expose for scatterlists ensures that any mappings created from one are
> destroyed on driver-unbind. To do this, we introduce a Devres resource into
> shmem::Object that we use in order to ensure that we release any SGTable
> mappings on driver-unbind. We store this in an UnsafeCell and protect
> access to it using the dma_resv lock that we already have from the shmem
> gem object, which is the same lock that currently protects
> drm_gem_object_shmem->sgt.
>
> We also provide two different methods for acquiring an sg table:
> self.sg_table(), and self.owned_sg_table(). The first function is for
> short-term uses of mapped SGTables, the second is for callers that need to
> hold onto the mapped SGTable for an extended period of time. The second
> variant uses Devres of course, whereas the first simply relies on rust's
> borrow checker to prevent driver-unbind when using the mapped SGTable.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
>
> ---
> 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
> V10:
> * Use Devres in order to ensure that SGTables are revocable, and are
>   unmapped on driver-unbind.
>
>  rust/kernel/drm/gem/shmem.rs | 191 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 189 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index c643f18b20838..111be446213df 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -11,25 +11,38 @@
>  
>  use crate::{
>      container_of,
> +    device::{
> +        self,
> +        Bound, //
> +    },
> +    devres::*,
>      drm::{
>          driver,
>          gem,
>          private::Sealed, //
>          Device,
>      },
> -    error::to_result,
> +    error::{
> +        from_err_ptr, //
> +        to_result,

nit: `//` guard should be on last item.

> +    },
>      prelude::*,
> +    scatterlist,
>      types::{
>          ARef,
>          Opaque, //
>      }, //
>  };
>  use core::{
> +    cell::UnsafeCell,
>      ops::{
>          Deref,
>          DerefMut, //
>      },
> -    ptr::NonNull,
> +    ptr::{
> +        self,
> +        NonNull, //
> +    },
>  };
>  use gem::{
>      BaseObjectPrivate,
> @@ -65,6 +78,10 @@ pub struct Object<T: DriverObject> {
>      obj: Opaque<bindings::drm_gem_shmem_object>,
>      /// Parent object that owns this object's DMA reservation object.
>      parent_resv_obj: Option<ARef<Object<T>>>,
> +    /// Devres object for unmapping any SGTable on driver-unbind.
> +    ///
> +    /// This is protected by the object's dma_resv lock.
> +    sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,
>      #[pin]
>      inner: T,
>  }
> @@ -117,6 +134,7 @@ pub fn new(
>              try_pin_init!(Self {
>                  obj <- Opaque::init_zeroed(),
>                  parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
> +                sgt_res: UnsafeCell::new(None),
>                  inner <- T::new(dev, size, args),
>              }),
>              GFP_KERNEL,
> @@ -176,6 +194,100 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
>          let _ = unsafe { KBox::from_raw(this) };
>      }
> +
> +    // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
> +    // that it is unmapped on driver unbind.
> +    fn create_sg_table<'a>(

This method looks like it is misnamed - it won't create the SG table if
it already exists, just return a reference to it. Maybe `get_sg_table`
is a more fitting name.

I would have suggesting splitting the creation part into a dedicated
method, but it would require the resv lock to be acquired as a
precondition so not sure that's worth it.

> +        &'a self,
> +        dev: &'a device::Device<Bound>,
> +    ) -> Result<&'a Devres<SGTableMap<T>>> {
> +        let ret;
> +        let sgt_res_ptr = self.sgt_res.get();
> +
> +        // SAFETY: This lock is initialized throughout the lifetime of the gem object
> +        unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
> +
> +        // SAFETY: We just grabbed the lock required for reading this data above.
> +        let sgt_res = unsafe { (*sgt_res_ptr).as_ref() };
> +        if let Some(sgt_res) = sgt_res {
> +            // We already have a Devres object for this sg table, return it
> +            ret = Ok(sgt_res);

nit: let's use `let ret = if let Some(sgt_res) ...` to avoid the
multiple `ret = `statements?

Or maybe even better, you might be able to use
`Option::get_or_insert_with`.