Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Apr 24, 2026 at 1:13 AM JST, Gary Guo wrote:
> On Thu Apr 23, 2026 at 4:27 PM BST, Alexandre Courbot wrote:
>> 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:
>>>>
>>>> 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/4aabdbcf20b11626c253f203745b1d55c37ab2ee).
>>>
>>> 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-074badd18716@linux.dev/
>>
>> Haha that's pretty close to what I wrote to test the code. Do you have
>> plans to send it soon?
>
> I mean.. Alvin has already sent it?
>
> If you (or someone else) want to carry the patch in another series, by all means.

My bad, it was late at night and my focus was a bit down.

It's perfect if Alvin keeps carrying this patch.