On Mon, 2026-06-15 at 10:26 +0100, Tvrtko Ursulin wrote: > Lets simplify and make clearer the fence lock cycling workaround in > nouveau_cli_work(). > > The reason for the workaround is that the worker processes the list of > pending work items (and so fence callbacks) opportunisticaly, while > dma_fence_is_signaled() returns true as soon as a the fence is signaled. > It is therefore not allowed for the opportunistic processing to free the > work item, since the fence callback processing can still be in the process > of dereferencing work->cb. > > This wasn't very clear in the code so clarify it with a nice comment. > > In the process we replace the somewhat interleaved and not very readable > nouveau_cli_work_ready() helper with a direct dma_fence_is_signaled, which > is even faster since it removes the lock cycling from the unsignaled fence > path. > > Signed-off-by: Tvrtko Ursulin <[email protected]> > Cc: Philipp Stanner <[email protected]> > Cc: Lyude Paul <[email protected]> > Cc: Danilo Krummrich <[email protected]> > Cc: [email protected] > --- > v2: > * Don't dereference work after work->func(work) frees it. > > v3: > * Put back and explain lock cycling. > --- > drivers/gpu/drm/nouveau/nouveau_drm.c | 34 +++++++++++++-------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c > b/drivers/gpu/drm/nouveau/nouveau_drm.c > index 42a81166f3a9..7ea95ab960a0 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c > @@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev) > return nouveau_platform_name(to_platform_device(dev->dev)); > } > > -static inline bool > -nouveau_cli_work_ready(struct dma_fence *fence) > -{ > - unsigned long flags; > - bool ret = true; > - > - dma_fence_lock_irqsave(fence, flags); > - if (!dma_fence_is_signaled_locked(fence)) > - ret = false; > - dma_fence_unlock_irqrestore(fence, flags); > - > - if (ret == true) > - dma_fence_put(fence); > - return ret; > -} > - > static void > nouveau_cli_work(struct work_struct *w) > { > @@ -176,9 +160,25 @@ nouveau_cli_work(struct work_struct *w) > struct nouveau_cli_work *work, *wtmp; > mutex_lock(&cli->lock); > list_for_each_entry_safe(work, wtmp, &cli->worker, head) { > - if (!work->fence || nouveau_cli_work_ready(work->fence)) { > + struct dma_fence *fence = work->fence; > + > + if (!fence || dma_fence_is_signaled(fence)) { > + if (fence) { > + unsigned long flags; > + > + /* > + * Because fence can still be in the process of > + * processing the callback list, and the > + * callback references the work we are about to > + * free, we need to sync with the callback > + * processing before freeing the work. > + */ > + dma_fence_lock_irqsave(fence, flags); > + dma_fence_unlock_irqrestore(fence, flags); > + } > list_del(&work->head); > work->func(work); > + dma_fence_put(fence);
This is semantically exactly the same fix that was introduced by Dave in c8a5d5ea3ba6a and which you are reverting here again, basically. It's no improvement IMO, just a differently looking work around the fact that the dma_fence framework is not properly synchronized. I suggest you participate here [1] and here [2] and share your opinion. At least *this* problem would disappear of only dma_fence_is_signaled() were locking properly. P. [1] https://lore.kernel.org/dri-devel/[email protected]/ [2] https://lore.kernel.org/dri-devel/[email protected]/ > } > } > mutex_unlock(&cli->lock);
