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/4aabdbcf20b11626c253f203745b1d55c37...).
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-074badd1...
Haha that's pretty close to what I wrote to test the code. Do you have plans to send it soon?