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.
