Message-ID: <4283351a-5650-4b48-8958-e76a54118045@amd.com>
Date: Thu, 10 Apr 2025 10:37:18 +0200
From: Christian König <christian.koenig@....com>
To: 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()

Am 09.04.25 um 17:04 schrieb Philipp Stanner:
> On Wed, 2025-04-09 at 16:10 +0200, Christian König wrote:
>>> 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.

What do you mean? I only skimmed over the use cases, but as far as I can see those are all valid.

You can test the flag if you know what the fence means to you, that is not a problem at all.

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

That sounds like I didn't used the right wording.

It *must* not matter to the consumer. See the purpose of the DMA-fence framework is to make it irrelevant for the consumer how the provider has implemented it's fences.

This means that things like if polling or interrupt driven signaling is used, 32bit vs 64bit seq numbers, etc... should all be hidden by the framework from the consumer of the fences.


BTW I'm actually not sure if nouveau has a bug here. As far as I can see nouveau_fence_signal() will be called later eventually and do the necessary cleanup.

But on the other hand it wouldn't surprise me if nouveau has a bug with that. The driver has been basically only barely maintained for quite a while.

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

Well this is not my preferred solution, it's just the technical correct solution as far as I can see.

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

As far as I can see that is clearly the wrong place to document that stuff.

Regards,
Christian.

>
> P.
Content of type "text/html" skipped

View attachment "0001-drm-nouveau-fix-and-cleanup-fence-handling.patch" of type "text/x-patch" (2670 bytes)

Powered by blists - more mailing lists