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: Opaquebindings::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 objectunsafe { 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 itret = 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`.