Re: [PATCH v18 4/4] rust: drm: gem: Introduce shmem::Object::sg_table()

[Alexandre Courbot]

On Fri Jun 5, 2026 at 4:24 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.
>
> There's some other slightly unfortunate caveats of this:
>
> * Drivers don't have explicit control at the moment over when unmapping
>   happens (which is exactly the same as the C side atm, so it might not be
>   a problem).
> * We can't just return `SGTableMap` to the user through an Arc to attempt
>   to fix the last caveat - because that implies the gem object would need
>   to hold a reference count to the scatterlist mapping, which just leaves
>   us with the same problem.
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>

I really like how simplified `sg_table` has become!

Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

With the customary final nits below.

<...>
> +
> +    /// 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. It is expected that `dev` should be a pointer to the
> +    /// same [`device::Device`] which `self` belongs to, otherwise this function will return
> +    /// `Err(EINVAL)`.
> +    pub fn sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,
> +    ) -> Result<&'a scatterlist::SGTable> {
> +        if dev.as_raw() != self.dev().as_ref().as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        let sgt_res = 'out: {
> +            // Fast path: sgt_res is already initialized
> +            if let Some(sgt_res) = self.sgt_res.as_ref() {
> +                break 'out sgt_res;
> +            }
> +
> +            // Slow path: Grab the lock and see if we need to initialize sgt_res.
> +            let _guard = self.sgt_lock.lock();
> +
> +            // If someone initialized it while we were waiting, we can exit early.
> +            if let Some(sgt_res) = self.sgt_res.as_ref() {
> +                break 'out sgt_res;
> +            }
> +
> +            // If not, finish initializing and return.
> +            self.sgt_res
> +                .populate(Devres::new(dev, SGTableMap::new(self))?);

Maybe add a comment explaining that `populate` cannot return `false`, as
its invocation it protected by the mutex? This helps understanding that
the following unsafe block is ok.

> +
> +            // SAFETY: We just populated sgt_res above.
> +            unsafe { self.sgt_res.as_ref().unwrap_unchecked() }
> +        };
> +
> +        Ok(sgt_res.access(dev)?)
> +    }
>  }
>  
>  impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> {
> @@ -474,6 +545,63 @@ impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, R, C, SIZE>
>  #[cfg(CONFIG_64BIT)]
>  impl_vmap_io_capable!(VMap, u64);
>  
> +/// A reference to a GEM object that is known to have a mapped [`SGTable`].
> +///
> +/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables
> +/// on GEM shmem objects are revoked on driver-unbind.
> +///
> +/// # Invariants
> +///
> +/// - `self.obj` always points to a valid GEM object.
> +/// - This object is proof that `self.obj.owner.sgt` has an initialized and valid
> +///   [`scatterlist::SGTable`].

The SGTable is not in `owner.sgt` anymore.

> +pub struct SGTableMap<T: DriverObject, C: DeviceContext> {
> +    obj: NonNull<Object<T, C>>,
> +}
> +
> +impl<T: DriverObject, C: DeviceContext> Deref for SGTableMap<T, C> {
> +    type Target = scatterlist::SGTable;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY:
> +        // - The NonNull is guaranteed to be valid via our type invariants.
> +        // - The sgt field is guaranteed to be initialized and valid via our type invariants.
> +        unsafe { scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) }
> +    }
> +}
> +
> +impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> {
> +    fn drop(&mut self) {
> +        // SAFETY: `obj` is always valid via our type invariants
> +        let obj = unsafe { self.obj.as_ref() };
> +        let _lock = DmaResvGuard::new(obj);
> +
> +        // SAFETY: We acquired the lock needed for calling this function above
> +        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
> +    }
> +}
> +
> +impl<T: DriverObject, C: DeviceContext> SGTableMap<T, C> {
> +    fn new(obj: &Object<T, C>) -> impl Init<Self, Error> {
> +        // INVARIANT:
> +        // - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it

s/drm_gem_shmem_get_pages_sgt_locked/drm_gem_shmem_get_pages_sgt.