Christian König
2025-Apr-11 11:05 UTC
[PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list
Am 11.04.25 um 11:29 schrieb Philipp Stanner:> [SNIP] > It could be, however, that at the same moment nouveau_fence_signal() is > removing that entry, holding the appropriate lock. > > So we have a race. Again.Ah, yes of course. If signaled is called with or without the lock is actually undetermined.> You see, fixing things in Nouveau is difficult :) > It gets more difficult if you want to clean it up "properly", so it > conforms to rules such as those from dma_fence. > > I have now provided two fixes that both work, but you are not satisfied > with from the dma_fence-maintainer's perspective. I understand that, > but please also understand that it's actually not my primary task to > work on Nouveau. I just have to fix this bug to move on with my > scheduler work.Well I'm happy with whatever solution as long as it works, but as far as I can see the approach with the callback simply doesn't. You just can't drop the fence reference for the list from the callback.> So if you have another idea, feel free to share it. But I'd like to > know how we can go on here.Well the fence code actually works, doesn't it? The problem is rather that setting the error throws a warning because it doesn't expect signaled fences on the pending list. Maybe we should fix that instead.> I'm running out of ideas. What I'm wondering if we couldn't just remove > performance hacky fastpath functions such as > nouveau_fence_is_signaled() completely. It seems redundant to me.That would work for me as well.> > Or we might add locking to it, but IDK what was achieved with RCU here. > In any case it's definitely bad that Nouveau has so many redundant and > half-redundant mechanisms.Yeah, agree messing with the locks even more won't help us here. Regards, Christian.> > > P. > >> >> P. >> >>> Regards, >>> Christian. >>> >>>> P. >>>> >>>> >>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> Replace the call to dma_fence_is_signaled() with >>>>>> nouveau_fence_base_is_signaled(). >>>>>> >>>>>> Cc: <stable at vger.kernel.org> # 4.10+, precise commit not to >>>>>> be >>>>>> determined >>>>>> Signed-off-by: Philipp Stanner <phasta at kernel.org> >>>>>> --- >>>>>> ?drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>>>>> ?1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> index 7cc84472cece..33535987d8ed 100644 >>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>>>>> @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence >>>>>> *fence) >>>>>> ? nvif_event_block(&fctx->event); >>>>>> ? spin_unlock_irqrestore(&fctx->lock, flags); >>>>>> ? } >>>>>> - return dma_fence_is_signaled(&fence->base); >>>>>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence- >>>>>>> base.flags); >>>>>> ?} >>>>>> ? >>>>>> ?static long-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20250411/05f4feed/attachment-0001.htm>