Re: [PATCH v2] drm/nouveau: Prevent signalled fences in pending list

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Apr 03, 2025 at 02:08:06PM +0200, Christian König wrote:
> Am 03.04.25 um 12:13 schrieb Philipp Stanner:
> 
> > @@ -235,6 +227,19 @@ nouveau_fence_emit(struct nouveau_fence *fence)
> >  			       &fctx->lock, fctx->context, ++fctx->sequence);
> >  	kref_get(&fctx->fence_ref);
> >  
> > +	fence->cb.func = nouveau_fence_cleanup_cb;
> > +	/* Adding a callback runs into __dma_fence_enable_signaling(), which will
> > +	 * ultimately run into nouveau_fence_no_signaling(), where a WARN_ON
> > +	 * would fire because the refcount can be dropped there.
> > +	 *
> > +	 * Increment the refcount here temporarily to work around that.
> > +	 */
> > +	dma_fence_get(&fence->base);
> > +	ret = dma_fence_add_callback(&fence->base, &fence->cb, nouveau_fence_cleanup_cb);
> 
> That looks like a really really awkward approach. The driver basically uses a the DMA fence infrastructure as middle layer and callbacks into itself to cleanup it's own structures.
> 
> Additional to that we don't guarantee any callback order for the DMA fence and so it can be that mix cleaning up the callback with other work which is certainly not good when you want to guarantee that the cleanup happens under the same lock.
> 
> Instead the call to dma_fence_signal_locked() should probably be removed from nouveau_fence_signal() and into nouveau_fence_context_kill() and nouveau_fence_update().
> 
> This way nouveau_fence_is_signaled() can call this function as well.

Yes, I think this would work as well. It wouldn't work if only
dma_fence_signal() is called on this fence, but that should be fine.

So, I agree that's probably the better approach.

> BTW: nouveau_fence_no_signaling() looks completely broken as well. It calls nouveau_fence_is_signaled() and then list_del() on the fence head.

It does indeed look broken, as in the fence may not be signaled at all. If at
all, it should call dma_fence_is_signaled() instead.

> As far as I can see that is completely superfluous and should probably be dropped. IIRC I once had a patch to clean that up but it was dropped for some reason.

Agreed, to me it looks unnecessary as well.