rust: drm: gem: Introduce shmem::Object::sg_table()

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@redhat.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Danilo Krummrich <dakr@kernel.org>
Link: https://patch.msgid.link/20260612194436.585385-5-lyude@redhat.com
This commit is contained in:
Lyude Paul 2026-06-12 15:43:36 -04:00
parent 52aab89ad3
commit 616c229ab0

View File

@ -11,6 +11,11 @@
use crate::{
container_of,
device::{
self,
Bound, //
},
devres::*,
drm::{
driver,
gem,
@ -19,14 +24,23 @@
DeviceContext,
Registered, //
},
error::to_result,
error::{
from_err_ptr,
to_result, //
},
io::{
Io,
IoCapable,
IoKnownSize, //
},
prelude::*,
sync::aref::ARef,
scatterlist,
sync::{
aref::ARef,
new_mutex,
Mutex,
SetOnce, //
},
types::{
NotThreadSafe,
Opaque, //
@ -35,7 +49,10 @@
use core::{
ffi::c_void,
marker::PhantomData,
mem::MaybeUninit, //
mem::{
ManuallyDrop,
MaybeUninit, //
},
ops::{
Deref,
DerefMut, //
@ -90,6 +107,11 @@ pub struct Object<T: DriverObject, C: DeviceContext = Registered> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T, C>>>,
/// Devres object for unmapping any SGTable on driver-unbind.
sgt_res: ManuallyDrop<SetOnce<Devres<SGTableMap<T, C>>>>,
#[pin]
/// Lock for protecting initialization of `sgt_res`.
sgt_lock: Mutex<()>,
#[pin]
inner: T,
_ctx: PhantomData<C>,
@ -148,6 +170,8 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
sgt_res: ManuallyDrop::new(SetOnce::new()),
sgt_lock <- new_mutex!(()),
inner <- T::new(dev, size, args),
_ctx: PhantomData::<C>,
}),
@ -192,18 +216,26 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We're in free_callback - so this function is safe to call.
// - We won't be using the gem resources on `this` after this call.
unsafe { bindings::drm_gem_shmem_release(this) };
let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T, C>`
let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
// We need to drop `sgt_res` first, since doing so requires that the GEM object is still
// alive.
// SAFETY:
// - We verified above that `this` is valid.
// - We are in free_callback, guaranteeing we have exclusive access to `this` and that
// `sgt_res` will not be used after dropping it here.
unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) };
// SAFETY:
// - We're in free_callback - so this function is safe to call.
// - We won't be using the gem resources on `this` after this call.
unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
@ -281,6 +313,46 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, SIZE>> {
self.make_vmap()
}
/// 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. `populate()` cannot return false, as
// `sgt_res` must be unpopulated, and we must hold `sgt_lock` to reach this point.
self.sgt_res
.populate(Devres::new(dev, SGTableMap::new(self))?);
// 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> {
@ -499,6 +571,64 @@ unsafe fn io_write(&self, value: $ty, address: usize) {
#[cfg(CONFIG_64BIT)]
impl_vmap_io_capable!(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_res` has an initialized and valid pointer to an
/// [`SGTable`].
///
/// [`SGTable`]: scatterlist::SGTable
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 below and check whether or not it succeeds,
// fulfilling the invariant of SGTableMap that the object's `sgt` field is initialized.
// SAFETY:
// - `obj` is fully initialized, making this function safe to call.
from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
Ok(Self { obj: obj.into() })
}
}
// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
// it points to is guaranteed to be thread-safe.
unsafe impl<T: DriverObject, C: DeviceContext> Send for SGTableMap<T, C> {}
// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
// it points to is guaranteed to be thread-safe.
unsafe impl<T: DriverObject, C: DeviceContext> Sync for SGTableMap<T, C> {}
#[kunit_tests(rust_drm_gem_shmem)]
mod tests {
use super::*;
@ -614,4 +744,28 @@ fn vmap_io() -> Result {
Ok(())
}
// TODO: I would love to actually test the success paths of sg_table(), but that would require
// also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
// that for someone else.
// Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
// skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
// bound to it.
#[test]
fn fail_sg_table_on_wrong_dev() -> Result {
let (_dev, drm) = create_drm_dev()?;
let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
// If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
// TODO: Someday, we should test that creating an sg_table here still succeeds.
Ok(())
}
}