Message-ID: <0b2fc70d8fae566c8ca43bafc929e2bd19725924.camel@mailbox.org>
Date: Wed, 09 Apr 2025 17:04:00 +0200
From: Philipp Stanner <phasta@...lbox.org>
To: Christian König <christian.koenig@....com>, 
	phasta@...nel.org, Boris Brezillon <boris.brezillon@...labora.com>
Cc: Sumit Semwal <sumit.semwal@...aro.org>, Gustavo Padovan
 <gustavo@...ovan.org>, Felix Kuehling <Felix.Kuehling@....com>, Alex
 Deucher <alexander.deucher@....com>, Xinhui Pan <Xinhui.Pan@....com>, David
 Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>,  Thomas Zimmermann <tzimmermann@...e.de>, Lucas Stach
 <l.stach@...gutronix.de>, Russell King <linux+etnaviv@...linux.org.uk>,
 Christian Gmeiner <christian.gmeiner@...il.com>,  Jani Nikula
 <jani.nikula@...ux.intel.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
 Tvrtko Ursulin <tursulin@...ulin.net>, Frank Binns
 <frank.binns@...tec.com>, Matt Coster <matt.coster@...tec.com>, Qiang Yu
 <yuq825@...il.com>, Rob Clark <robdclark@...il.com>, Sean Paul
 <sean@...rly.run>,  Konrad Dybcio <konradybcio@...nel.org>, Abhinav Kumar
 <quic_abhinavk@...cinc.com>, Dmitry Baryshkov
 <dmitry.baryshkov@...aro.org>, Marijn Suijten
 <marijn.suijten@...ainline.org>,  Lyude Paul <lyude@...hat.com>, Danilo
 Krummrich <dakr@...nel.org>, Rob Herring <robh@...nel.org>,  Steven Price
 <steven.price@....com>, Dave Airlie <airlied@...hat.com>, Gerd Hoffmann
 <kraxel@...hat.com>,  Matthew Brost <matthew.brost@...el.com>, Huang Rui
 <ray.huang@....com>, Matthew Auld <matthew.auld@...el.com>,  Melissa Wen
 <mwen@...lia.com>, Maíra Canal <mcanal@...lia.com>, Zack
 Rusin <zack.rusin@...adcom.com>, Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Lucas De Marchi
 <lucas.demarchi@...el.com>, Thomas Hellström
 <thomas.hellstrom@...ux.intel.com>, Bas Nieuwenhuizen
 <bas@...nieuwenhuizen.nl>,  Yang Wang <kevinyang.wang@....com>, Jesse Zhang
 <jesse.zhang@....com>, Tim Huang <tim.huang@....com>,  Sathishkumar S
 <sathishkumar.sundararaju@....com>, Saleemkhan Jamadar
 <saleemkhan.jamadar@....com>, Sunil Khatri <sunil.khatri@....com>, Lijo
 Lazar <lijo.lazar@....com>, Hawking Zhang <Hawking.Zhang@....com>, Ma Jun
 <Jun.Ma2@....com>, Yunxiang Li <Yunxiang.Li@....com>, Eric Huang
 <jinhuieric.huang@....com>, Asad Kamal <asad.kamal@....com>, Srinivasan
 Shanmugam <srinivasan.shanmugam@....com>,  Jack Xiao <Jack.Xiao@....com>,
 Friedrich Vock <friedrich.vock@....de>, Michel Dänzer
 <mdaenzer@...hat.com>, Geert Uytterhoeven <geert@...ux-m68k.org>,
 Anna-Maria Behnsen <anna-maria@...utronix.de>, Thomas Gleixner
 <tglx@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, Dan
 Carpenter <dan.carpenter@...aro.org>,  linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,  linaro-mm-sig@...ts.linaro.org,
 linux-kernel@...r.kernel.org,  amd-gfx@...ts.freedesktop.org,
 etnaviv@...ts.freedesktop.org,  intel-gfx@...ts.freedesktop.org,
 lima@...ts.freedesktop.org,  linux-arm-msm@...r.kernel.org,
 freedreno@...ts.freedesktop.org,  nouveau@...ts.freedesktop.org,
 virtualization@...ts.linux.dev,  spice-devel@...ts.freedesktop.org,
 intel-xe@...ts.freedesktop.org
Subject: Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

On Wed, 2025-04-09 at 16:10 +0200, Christian König wrote:
> Am 09.04.25 um 16:01 schrieb Philipp Stanner:
> > On Wed, 2025-04-09 at 15:14 +0200, Christian König wrote:
> > > Am 09.04.25 um 14:56 schrieb Philipp Stanner:
> > > > On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner wrote:
> > > > > On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote:
> > > > > > Hi Philipp,
> > > > > > 
> > > > > > On Wed,  9 Apr 2025 14:06:37 +0200
> > > > > > Philipp Stanner <phasta@...nel.org> wrote:
> > > > > > 
> > > > > > > dma_fence_is_signaled()'s name strongly reads as if this
> > > > > > > function
> > > > > > > were
> > > > > > > intended for checking whether a fence is already
> > > > > > > signaled.
> > > > > > > Also
> > > > > > > the
> > > > > > > boolean it returns hints at that.
> > > > > > > 
> > > > > > > The function's behavior, however, is more complex: it can
> > > > > > > check
> > > > > > > with a
> > > > > > > driver callback whether the hardware's sequence number
> > > > > > > indicates
> > > > > > > that
> > > > > > > the fence can already be treated as signaled, although
> > > > > > > the
> > > > > > > hardware's /
> > > > > > > driver's interrupt handler has not signaled it yet. If
> > > > > > > that's
> > > > > > > the
> > > > > > > case,
> > > > > > > the function also signals the fence.
> > > > > > > 
> > > > > > > (Presumably) this has caused a bug in Nouveau (unknown
> > > > > > > commit),
> > > > > > > where
> > > > > > > nouveau_fence_done() uses the function to check a fence,
> > > > > > > which
> > > > > > > causes a
> > > > > > > race.
> > > > > > > 
> > > > > > > Give the function a more obvious name.
> > > > > > This is just my personal view on this, but I find the new
> > > > > > name
> > > > > > just
> > > > > > as
> > > > > > confusing as the old one. It sounds like something is
> > > > > > checked,
> > > > > > but
> > > > > > it's
> > > > > > clear what, and then the fence is forcibly signaled like it
> > > > > > would
> > > > > > be
> > > > > > if
> > > > > > you call drm_fence_signal(). Of course, this clarified by
> > > > > > the
> > > > > > doc,
> > > > > > but
> > > > > > given the goal was to make the function name clearly
> > > > > > reflect
> > > > > > what
> > > > > > it
> > > > > > does, I'm not convinced it's significantly better.
> > > > > > 
> > > > > > Maybe dma_fence_check_hw_state_and_propagate(), though it
> > > > > > might
> > > > > > be
> > > > > > too long of name. Oh well, feel free to ignore this
> > > > > > comments if
> > > > > > a
> > > > > > majority is fine with the new name.
> > > > > Yoa, the name isn't perfect (the perfect name describing the
> > > > > whole
> > > > > behavior would be
> > > > > dma_fence_check_if_already_signaled_then_check_hardware_state
> > > > > _and
> > > > > _pro
> > > > > pa
> > > > > gate() ^^'
> > > > > 
> > > > > My intention here is to have the reader realize "watch out,
> > > > > the
> > > > > fence
> > > > > might get signaled here!", which is probably the most
> > > > > important
> > > > > event
> > > > > regarding fences, which can race, invoke the callbacks and so
> > > > > on.
> > > > > 
> > > > > For details readers will then check the documentation.
> > > > > 
> > > > > But I'm of course open to see if there's a majority for this
> > > > > or
> > > > > that
> > > > > name.
> > > > how about:
> > > > 
> > > > dma_fence_check_hw_and_signal() ?
> > > I don't think that renaming the function is a good idea in the
> > > first
> > > place.
> > > 
> > > What the function does internally is an implementation detail of
> > > the
> > > framework.
> > > 
> > > For the code using this function it's completely irrelevant if
> > > the
> > > function might also signal the fence, what matters for the caller
> > > is
> > > the returned status of the fence. I think this also counts for
> > > the
> > > dma_fence_is_signaled() documentation.
> > It does obviously matter. As it's currently implemented, a lot of
> > important things happen implicitly.
> 
> Yeah, but that's ok.
> 
> The code who calls this is the consumer of the interface and so
> shouldn't need to know this. That's why we have created the DMA fence
> framework in the first place.
> 
> For the provider side when a driver or similar implements the
> interface the relevant documentation is the dma_fence_ops structure.
> 
> > I only see improvement by making things more obvious.
> > 
> > In any case, how would you call a wrapper that just does
> > test_bit(IS_SIGNALED, …) ?
> 
> Broken, that was very intentionally removed quite shortly after we
> created the framework.
> 
> We have a few cases were implementations do check that for their
> fences, but consumers should never be allowed to touch such
> internals.

There is theory and there is practice. In practice, those internals are
being used by Nouveau, i915, Xe, vmgfx and radeon.

So it seems that we failed quite a bit at communicating clearly how the
interface should be used.

And, to repeat myself, with both name and docu of that function, I
think it is very easy to misunderstand what it's doing. You say that it
shouldn't matter – and maybe that's true, in theory. In practice, it
does matter. In practice, APIs get misused and have side-effects. And
making that harder is desirable.

In any case, I might have to add another such call to Nouveau, because
the solution preferred by you over the callback causes another race.
Certainly one could solve this in a clean way, but someone has to do
the work, and we're talking about more than a few hours here.

In any case, be so kind and look at patch 2 and tell me there if you're
at least OK with making the documentation more detailed.

P.

> 
> Regards,
> Christian.
> 
> > 
> > P.
> > 
> > > What we should improve is the documentation of the dma_fence_ops-
> > > > enable_signaling and dma_fence_ops->signaled callbacks.
> > > Especially see the comment about reference counts on
> > > enable_signaling
> > > which is missing on the signaled callback. That is most likely
> > > the
> > > root cause why nouveau implemented enable_signaling correctly but
> > > not
> > > the other one.
> > > 
> > > But putting that aside I think we should make nails with heads
> > > and
> > > let the framework guarantee that the fences stay alive until they
> > > are
> > > signaled (one way or another). This completely removes the burden
> > > to
> > > keep a reference on unsignaled fences from the drivers /
> > > implementations and make things more over all more defensive.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > P.
> > > > 
> > > > > P.
> > > > > 
> > > > > 
> > > > > > Regards,
> > > > > > 
> > > > > > Boris
> 

Powered by blists - more mailing lists