Re: [RFC PATCH] rust: sync: Add dma_fence abstractions

[Philipp Stanner]

On Sun, 2025-09-28 at 16:34 +0200, Christian König wrote:
> On 27.09.25 11:01, Philipp Stanner wrote:
> > On Fri, 2025-09-26 at 09:10 -0700, Boqun Feng wrote:
> > > On Thu, Sep 18, 2025 at 02:30:59PM +0200, Philipp Stanner wrote:
> > > > dma_fence is a synchronization mechanism which is needed by virtually
> > > > all GPU drivers.
> > > > 
> > > > A dma_fence offers many features, among which the most important ones
> > > > are registering callbacks (for example to kick off a work item) which
> > > > get executed once a fence gets signalled.
> > > > 
> > > > dma_fence has a number of callbacks. Only the two most basic ones
> > > > (get_driver_name(), get_timeline_name() are abstracted since they are
> > > > enough to enable the basic functionality.
> > > > 
> > > > Callbacks in Rust are registered by passing driver data which implements
> > > > the Rust callback trait, whose function will be called by the C backend.
> > > > 
> > > > dma_fence's are always refcounted, so implement AlwaysRefcounted for
> > > > them. Once a reference drops to zero, the C backend calls a release
> > > > function, where we implement drop_in_place() to conveniently marry that
> > > > C-cleanup mechanism with Rust's ownership concepts.
> > > > 
> > > > This patch provides basic functionality, but is still missing:
> > > >   - An implementation of PinInit<T, Error> for all driver data.
> > > >   - A clever implementation for working dma_fence_begin_signalling()
> > > >     guards. See the corresponding TODO in the code.
> > > >   - Additional useful helper functions such as dma_fence_is_signaled().
> > > >     These _should_ be relatively trivial to implement, though.
> > > > 
> > > > Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
> > > > ---
> > > > So. ¡Hola!
> > > > 
> > > > This is a highly WIP RFC. It's obviously at many places not yet
> > > > conforming very well to Rust's standards.
> > > > 
> > > > Nevertheless, it has progressed enough that I want to request comments
> > > > from the community.
> > > > 
> > > > There are a number of TODOs in the code to which I need input.
> > > > 
> > > > Notably, it seems (half-)illegal to use a shared static reference to an
> > > > Atomic, which I currently use for the dma_fence unit test / docstring
> > > > test. I'm willing to rework that if someone suggests how.
> > > > (Still, shouldn't changing a global Atomic always be legal? It can race,
> > > > of course. But that's kind of the point of an atomic)
> > > > 
> > > > What I want comments on the most is the design of the callbacks. I think
> > > > it's a great opportunity to provide Rust drivers with rust-only
> > > > callbacks, so that they don't have to bother about the C functions.
> > > > 
> > > > dma_fence wise, only the most basic callbacks currently get implemented.
> > > > For Nova, AFAICS, we don't need much more than signalling fences and
> > > > registering callbacks.
> > > > 
> > > > 
> > > > Another, solvable, issue I'm having is designing the
> > > > dma_fence_begin_signallin() abstractions. There are TODOs about that in
> > > > the code. That should ideally be robust and not racy. So we might want
> > > > some sort of synchronized (locked?) way for using that abstraction.
> > > > 
> > > > 
> > > > Regarding the manually created spinlock of mine: I so far never need
> > > > that spinlock anywhere in Rust and wasn't sure what's then the best way
> > > > to pass a "raw" spinlock to C.
> > > > 
> > > > 
> > > > So much from my side. Hope to hear from you.
> > > > 
> > > > (I've compiled and tested this with the unit test on the current -rc3)
> > > > 
> > > > Philipp
> > > > ---
> > > >  rust/bindings/bindings_helper.h |   1 +
> > > >  rust/helpers/dma_fence.c        |  23 ++
> > > >  rust/helpers/helpers.c          |   1 +
> > > >  rust/helpers/spinlock.c         |   5 +
> > > >  rust/kernel/sync.rs             |   2 +
> > > >  rust/kernel/sync/dma_fence.rs   | 388 ++++++++++++++++++++++++++++++++
> > > 
> > > I missed this part, and I don't think kernel::sync is where dma_fence
> > > should be, as kernel::sync is mostly for the basic synchronization
> > > between threads/irqs. dma_fence is probably better to be grouped with
> > > dma-buf and other dma related primitives. Maybe in kernel::dma? Like:
> > > 
> > > rust/kernel/dma.rs
> > > rust/kernel/dma/dma_buf.rs
> > > rust/kernel/dma/dma_fence.rs
> > > 
> > > Thoughts? Miguel, Greg, Danilo and Lyude, any idea or suggestion?
> > 
> > @Christian König's opinion would be valuable, too.
> 
> Oh yes, please don't mix dma_fences into SW synchronization, it's a HW synchronization primitive.

So do you agree that it should be regarded as a part of DMA_BUF?

> 
> > I'm not super convinced of that because dma_fence has not really much
> > to do with DMA. They're not very different from completions and are a
> > mechanism to synchronize consumers and producers.
> 
> That we mixed up fences and completion events caused a lot of trouble in the past.
> 
> It's astonishing how often Sima and I had to reject peoples ideas to use the dma_fence as SW sync.
> 
> > Actually, before f54d1867005c3 they were just called "fence" and then
> > renamed to "dma_fence" because someone wanted that name.
> 
> That was also partially done to make sure that people understand that this is not for inter SW sync.
> 
> On the other hand the rename didn't helped much :/

gpu_fence might have been appropriate?


Grüße
P.


> 
> Regards,
> Christian.
> 
> > 
> > 
> > Anyways, I don't have strong objections and mostly care about having
> > them available somewhere.
> > 
> > P.
> > 
> > > 
> > > Regards,
> > > Boqun
> > > 
> > > >  6 files changed, 420 insertions(+)
> > > >  create mode 100644 rust/helpers/dma_fence.c
> > > >  create mode 100644 rust/kernel/sync/dma_fence.rs
> > > > 
> > > [...]
> > 
>