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

Alexandre Courbot
2026-04-10T07:55:38Z

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`.