[PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

[Christian König]

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>