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); } } mutex_unlock(&cli->lock); -- 2.54.0
