Hi Lyude,
> On 29 Aug 2025, at 19:35, Lyude Paul <[email protected]> wrote: > > Currently we expose the ability to retrieve an SGTable for an shmem gem > object using gem::shmem::Object::<T>::sg_table(). However, this only gives us > a > borrowed reference. This being said - retrieving an SGTable is a fallible > operation, and as such it's reasonable that a driver may want to hold > onto an SGTable for longer then a reference would allow in order to avoid > having to deal with fallibility every time they want to access the SGTable. > One such driver with this usecase is the Asahi driver. > > So to support this, let's introduce SGTableRef - which both holds a > pointer to the SGTable and a reference to its respective GEM object in > order to keep the GEM object alive for as long as the SGTableRef. The > type can be used identically to a normal SGTable. > > Signed-off-by: Lyude Paul <[email protected]> > > --- > V3: > * Rename OwnedSGTable to SGTableRef. Since the current version of the > SGTable abstractions now has a `Owned` and `Borrowed` variant, I think > renaming this to SGTableRef makes things less confusing. > We do however, keep the name of owned_sg_table() as-is. > > Signed-off-by: Lyude Paul <[email protected]> > --- > rust/kernel/drm/gem/shmem.rs | 50 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 6a8a392c3691b..1437cda27a22c 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs > @@ -178,6 +178,22 @@ pub fn sg_table(&self) -> Result<&scatterlist::SGTable> { > Ok(unsafe { scatterlist::SGTable::from_raw(sgt) }) > } > > + /// Creates (if necessary) and returns an owned reference to a > scatter-gather table of DMA pages > + /// for this object. > + /// > + /// This is the same as [`sg_table`](Self::sg_table), except that it > instead returns a > + /// [`OwnedSGTable`] which holds a reference to the associated gem > object. This was forgotten ^ > + /// > + /// This will pin the object in memory. > + pub fn owned_sg_table(&self) -> Result<SGTableRef<T>> { owned_sg_table() returns SGTableRef. I do think this is confusing. > + Ok(SGTableRef { Let’s call this shmem::SGTable, perhaps? > + sgt: self.sg_table()?.into(), > + // INVARIANT: We take an owned refcount to `self` here, ensuring > that `sgt` remains > + // valid for as long as this `OwnedSGTable`. > + _owner: self.into(), > + }) > + } > + > /// Creates and returns a virtual kernel memory mapping for this object. > pub fn vmap(&self) -> Result<VMap<T>> { > let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit(); > @@ -309,3 +325,37 @@ fn drop(&mut self) { > unsafe impl<T: DriverObject> Send for VMap<T> {} > /// SAFETY: `iosys_map` objects are safe to send across threads. > unsafe impl<T: DriverObject> Sync for VMap<T> {} > + > +/// An owned reference to a scatter-gather table of DMA address spans for a > GEM shmem object. > +/// > +/// This object holds an owned reference to the underlying GEM shmem object, > ensuring that the > +/// [`SGTable`] referenced by `SGTableRef` remains valid for the lifetime of > this object. > +/// > +/// # Invariants > +/// > +/// - `sgt` is kept alive by `_owner`, ensuring it remains valid for as long > as `Self`. > +/// - `sgt` corresponds to the owned object in `_owner`. > +/// - This object is only exposed in situations where we know the underlying > `SGTable` will not be > +/// modified for the lifetime of this object. > +/// > +/// [`SGTable`]: scatterlist::SGTable > +pub struct SGTableRef<T: DriverObject> { > + sgt: NonNull<scatterlist::SGTable>, Didn’t Danilo & Abdiel introduce an owned SGTable variant? > + _owner: ARef<Object<T>>, > +} > + > +// SAFETY: This object is only exposed in situations where we know the > underlying `SGTable` will not > +// be modified for the lifetime of this object. We should perhaps say why is it valid to send SGTable to another thread here. > +unsafe impl<T: DriverObject> Send for SGTableRef<T> {} > +// SAFETY: This object is only exposed in situations where we know the > underlying `SGTable` will not > +// be modified for the lifetime of this object. > +unsafe impl<T: DriverObject> Sync for SGTableRef<T> {} > + > +impl<T: DriverObject> Deref for SGTableRef<T> { > + type Target = scatterlist::SGTable; > + > + fn deref(&self) -> &Self::Target { > + // SAFETY: Creating an immutable reference to this is safe via our > type invariants. > + unsafe { self.sgt.as_ref() } The as_ref() nomenclature remains in place to convert *mut T to &T? I thought that had changed to from_raw(). > + } > +} > -- > 2.50.0 > >
