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

Gary Guo
2026-04-23T15:09:38Z

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/

Best,
Gary