[Linaro-mm-sig] Re: [PATCH v12 4/5] rust: drm: gem: Introduce shmem::SGTable

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/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?

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.