Re: [RFC WIP 2/3] rust: sync: Add dma_fence abstractions

[Philipp Stanner <phasta@mailbox.org>]

On Fri, 2025-11-21 at 18:03 -0500, Lyude Paul wrote:
> I haven't gone through this fully yet. I meant to today, but I ended up
> needing way more time to explain some of my review comments w/r/t some
> ww_mutex bindings for rust then I was expecting. But I do already have some
> comments worth reading below:
> 
> On Tue, 2025-11-18 at 14:25 +0100, Philipp Stanner wrote:
> > 
> > +
> > +/// Container for driver data which the driver gets back in its callback once the fence gets
> > +/// signalled.
> > +#[pin_data]
> > +pub struct DmaFenceCb<T: DmaFenceCbFunc> {
> > +    /// C struct needed for the backend.
> > +    #[pin]
> > +    inner: Opaque<bindings::dma_fence_cb>,
> > +    /// Driver data.
> > +    #[pin]
> > +    pub data: T,
> 
> It's entirely possible I've just never seen someone do this before but - is
> are we actually able to make pinned members of structs `pub`? I would have
> thought that wouldn't be allowed (especially if `data` was exposed as just
> `T`, since a user could then move it pretty easily and break the pinning
> guarantee).

> 
> …snip…
> 
> > +    }
> > +
> > +    /// # Safety
> > +    ///
> > +    /// `ptr`must be a valid pointer to a [`DmaFence`].
> > +    unsafe fn dec_ref(ptr: NonNull<Self>) {
> > +        // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is called
> > +        // the fence is by definition still valid.
> > +        let fence = unsafe { (*ptr.as_ptr()).inner.get() };
> > +
> > +        // SAFETY: Valid because `fence` was created validly above.
> > +        unsafe { bindings::dma_fence_put(fence) }
> > +    }
> > +}
> > +
> > +impl<T> DmaFence<T> {
> > +    // TODO: There could be a subtle potential problem here? The LLVM compiler backend can create
> > +    // several versions of this constant. Their content would be identical, but their addresses
> > +    // different.
> > +    const OPS: bindings::dma_fence_ops = Self::ops_create();
> 
> oh no, not you too!!! D:
> 
> I can answer this question - yes, `OPS` definitely won't have a unique memory
> address. Whether that's an issue or not depends on if you actually need to
> check what pointer a `DmaFence` has its `dma_fence_ops` set to and compare it
> against another. If not though, it's probably fine.

In C, there are some use cases where people check the fence_ops addr to
see to whom the fence belongs, AFAIK.

I, so far, can live with there being several ops as long as they all
point to the same functions:

get_driver_name() and get_timeline_name() won't be called by anyone any
time soon (maybe we could even remove them from C, but so far they are
mandatory), and release() receives its data pointer from the C backend,
and since all is pinned we should be good.

However, it's probably wise to at least leave a comment (without the
"TODO") there to make future extenders aware that they cannot identify
a fence by its ops.

> > 
> > 

[…]

> > +
> > +    /// Signal the fence. This will invoke all registered callbacks.
> > +    pub fn signal(&self) -> Result {
> > +        // SAFETY: `self` is refcounted.
> > +        let ret = unsafe { bindings::dma_fence_signal(self.as_raw()) };
> > +        if ret != 0 {
> > +            return Err(Error::from_errno(ret));
> > +        }
> 
> You can just use to_result()

OK.

--


I want to present a new version of DmaFence soonish which takes the
separate spinlocks into account that Christian is working on.


P.