Message-ID: <5873bc43-1d60-419a-9c5b-e623fc5e9c47@amd.com>
Date: Sun, 28 Sep 2025 16:34:23 +0200
From: Christian König <christian.koenig@....com>
To: phasta@...nel.org, Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Gary Guo <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
 Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Asahi Lina <lina+kernel@...hilina.net>,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Tamir Duberstein <tamird@...il.com>,
 Wedson Almeida Filho <wedsonaf@...il.com>,
 FUJITA Tomonori <fujita.tomonori@...il.com>,
 Krishna Ketan Rai <prafulrai522@...il.com>, Lyude Paul <lyude@...hat.com>,
 Mitchell Levy <levymitchell0@...il.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, llvm@...ts.linux.dev,
 dri-devel@...ts.freedesktop.org
Subject: Re: [RFC PATCH] rust: sync: Add dma_fence abstractions

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@...nel.org>
>>> ---
>>> 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.

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

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

Powered by blists - more mailing lists