Re: [PATCH 3/4] rust: Add dma_fence abstractions

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 3 Jun 2026 13:41:02 -0300
Daniel Almeida <dwlsalmeida@gmail.com> wrote:

> > +    /// Called when the fence is signaled.
> > +    ///
> > +    /// This is called from the fence signaling path, which may be in interrupt
> > +    /// context or with locks held, which is why `self` is only borrowed, so that
> > +    /// it cannot drop. Implementations must not sleep or perform
> > +    /// long-running operations.
> > +    ///
> > +    /// An implementation likely wants to inform itself (e.g., through a work item)
> > +    /// within this callback that the associated [`FenceCbRegistration`] can now be
> > +    /// dropped.
> > +    fn called(&mut self);  
> 
> This is a central point. We ideally would want this to consume self, because we
> may want to move things out of the callback.  

This one comes from me. The rationale being that ::called() is called
from an atomic context, and the resources attached to the callback data
might require acquiring other sleeping locks to be released, and
sometimes you don't even notice immediately because said resources are
refcounted, and the lock is only acquired when you happen to be the
last owner. Yes, those can be caught at runtime if the C side is
properly annotated with might_sleep(), but that's not always the case.

If we defer the drop of the data only when the FenceCb is
dropped/recycled, we're at least not constrained by this "runs in
atomic context" thing.

> 
> Consider a fence design where signal() consumes self. Now consider this:
> 
> ```
> impl FenceCb for MyCallback {
>  fn called(&mut self) {
>    // Can't move the fence out, so we have to put an Option<T> just to be able
>    // to move.
>    if let Some(f) = self.some_fence.take() {
>      f.signal();
>    }
> }
> ```
> 
> This used to be the case when our version of the job queue used the "proxy
> fence" design:
> 
> 
> ```
> // Callback on the hw fence
> impl FenceCb for MyCallback {
>  fn called(&mut self) {
>    if let Some(f) = self.submit_fence.take() {
>      f.signal();
>    }

I'm pretty sure lockdep won't like it anyway, because this is nested
locking of the same lock class. For such proxies, we'll need to teach
lockdep about the nesting like has been recently done on
dma_fence_array & co. But I'm digressing.

> }
> ```
> 
> Although this is not the case anymore, since we phased out this design given
> Christian's recent work. Still, we should ideally not require Option<T> here in
> general just to make resource transfer possible.

I see. OTOH, don't we need to make this inner data movable if we want
to cancel the FenceCb before the fence is signaled anyway? And that's
most certainly a case we have in the teardown path.