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

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

On Fri Apr 24, 2026 at 12:09 AM JST, Gary Guo wrote:
> On Thu Apr 23, 2026 at 4:01 PM BST, Alexandre Courbot wrote:
>> Hi Lyude,
>>
>> On Wed Apr 22, 2026 at 8:52 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.
>>> V11:
>>> * s/create_sg_table()/get_sg_table()
>>> * Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
>>> V12:
>>> * Actually move sgt_res in this patch and not the next one
>>>
>>>  rust/kernel/drm/gem/shmem.rs | 192 ++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 190 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
>>> index 11749c36e8695..a477312c8a09b 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, //
>>> +    },
>>>      prelude::*,
>>> +    scatterlist,
>>>      types::{
>>>          ARef,
>>
>> This fails on master:
>>
>>     error[E0432]: unresolved import `crate::sync::ARef`
>>       --> ../rust/kernel/drm/gem/shmem.rs:36:5
>>       |
>>     36 |     sync::ARef,
>>       |     ^^^^^^^^^^ no `ARef` in `sync`
>>
>> Importing `sync::aref::ARef` seems to be the correct way now.
>>
>>>          Opaque, //
>>>      }, //
>>>  };
>>>  use core::{
>>> +    cell::UnsafeCell,
>>>      ops::{
>>>          Deref,
>>>          DerefMut, //
>>>      },
>>> -    ptr::NonNull,
>>> +    ptr::{
>>> +        self,
>>> +        NonNull, //
>>> +    },
>>>  };
>>>  use gem::{
>>>      BaseObjectPrivate,
>>> @@ -61,6 +74,11 @@ pub struct ObjectConfig<'a, T: DriverObject> {
>>>  #[repr(C)]
>>>  #[pin_data]
>>>  pub struct Object<T: DriverObject> {
>>> +    /// Devres object for unmapping any SGTable on driver-unbind.
>>> +    ///
>>> +    /// This is protected by the object's dma_resv lock. It needs to be before `obj` to ensure that
>>> +    /// it is destroyed before `obj` on `Drop`.
>>> +    sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,
>>
>> I didn't like this `UnsafeCell<Option>` since the last time, but only figured how to replace it now:
>>
>>     sgt_res: SetOnce<Devres<SGTableMap<T>>>,
>>
>> It's actually designed for that! And lets you remove at least one unsafe
>> statement, while simplifying `get_sg_table` quite a bit. With the other
>> suggestions I have below, here is my version of `get_sg_table` for
>> reference:
>>
>>     fn get_sg_table<'a>(
>>         &'a self,
>>         dev: &'a device::Device<Bound>,
>>     ) -> Result<&'a Devres<SGTableMap<T>>> {
>>         let _dma_resv = DmaResvGuard::new(self);
>>
>>         if let Some(devres) = self.sgt_res.as_ref() {
>>             Ok(devres)
>>         } else {
>>             // Only called for the side-effect of populating the GEM SG table.
>>             // SAFETY: We grabbed the lock required for calling this function above.
>>             from_err_ptr(unsafe {
>>                 bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
>>             })?;
>>
>>             // INVARIANT:
>>             // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
>>             //   succeeded, fulfilling the invariant of `SGTableMap` that the object's `sgt` field
>>             //   is initialized.
>>             // - We store this Devres in the object itself and don't move it, ensuring that the
>>             //   object it points to remains valid for the lifetime of the `SGTableMap`.
>>             let devres =
>>                 Devres::new(dev, init!(SGTableMap { obj: self.into() })).inspect_err(|_| {
>>                     // We can't make sure that the pages for this object are unmapped on
>>                     // driver-unbind, so we need to release the sgt
>>                     // SAFETY:
>>                     // - We grabbed the lock required for calling this function above
>>                     // - We checked above that get_pages_sgt_locked() was successful
>>                     unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) }
>>                 })?;
>>
>>             self.sgt_res.populate(devres);
>>
>>             // PANIC: `populate` has just succeeded, guaranteeing that `sgt_res` is populated.
>>             Ok(self.sgt_res.as_ref().unwrap())
>>         }
>>     }
>>
>> And if only we could populate the `SetOnce` with a `impl Init<T, E>`,
>> then we could even remove the DMA reservation acquisition on the fast
>> path, because `SetOnce` comes with its own locking and the DMA lock here
>> is used outside of its intended scope. I'll try to push the necessary
>> work for `SetOnce` and maybe we can do that as a follow-up patch.
>
> I have this sitting in my once_wip branch for while
> https://github.com/nbdd0121/linux/commits/once_wip/
> (the specific commit that adds init support is
> https://github.com/nbdd0121/linux/commit/4aabdbcf20b11626c253f203745b1d55c37ab2ee).
>
> This was implemented for lazy revocable support which Alvin has picked up, see
> https://lore.kernel.org/rust-for-linux/20260326-b4-tyr-debugfs-v1-1-074badd18716@linux.dev/

Haha that's pretty close to what I wrote to test the code. Do you have
plans to send it soon?