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

[Daniel Almeida <dwlsalmeida@gmail.com>]

> On 3 Jun 2026, at 14:14, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> 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.
> 

This design does not solve it, because one can quite trivially get around this
restriction using Option<T> as I said. If your point is “don’t run any drop() here”,
then &mut self doesn’t do it.

>> 
>> 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.

Yeah, but this is more about resource transfer in general, not
this pattern specifically.

I agree that this has issues, and yes, lockdep complained back
then :)

> 
>> }
>> ```
>> 
>> 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.

Can you expand a bit on what you mean here?