On Fri, Oct 15, 2021 at 02:56:59PM +0200, Christian König wrote:
Am 15.10.21 um 14:52 schrieb Maarten Lankhorst:
Op 15-10-2021 om 14:07 schreef Christian König:
Am 15.10.21 um 13:57 schrieb Maarten Lankhorst:
Commit 7fa828cb9265 ("dma-buf: use new iterator in dma_resv_test_signaled") accidentally forgot to test whether the dma-buf is actually signaled, breaking pretty much everything depending on it.
NAK, the dma_resv_for_each_fence_unlocked() returns only unsignaled fences. So the code is correct as it is.
That seems like it might cause some unexpected behavior when that function is called with one of the fence locks held, if it calls dma_fence_signal().
Could it be changed to only test the signaled bit, in which case this patch would still be useful?
That's exactly what I suggested as well, but Daniel was against that because of concerns around barriers.
I don't want open-coded bitmask tests, because the current code we have in dma-fence.c is missing barriers, and that doesn't get better if we spread that all around. But if you want this then wrap it in some static inline in dma-fence.h or so, that's fine. Just not open-coded outside of these files, like i915-gem code does a lot (which imo is just plain a disaster).
Or at least add some lockdep annotations, that fence->lock might be taken. So any hangs would at least be easy to spot with lockdep.
That should be trivial doable.
might_lock is trivial to add, but it's more complicated. The spinlock is provided by the fence code, which means there's lots of different lockdep classes. A might_lock on fence->lock is better than nothing, but maybe not good enough.
What we might need are a few more pieces: - a fake dma-fence spinlock lockdep key, maybe call it dma_fence_lock_key or so. - in dma_fence_init we lock dma_fence_lock_key, and then might_lock the actual spinlock passed as an argument. This establishes dependencies from that fake lock to all real fence spinlocks - anywhere we need a might lock we take dma_fence_lock_key instead
The potential issue here is that this might result in lockdep splats in cases where fences somehow naturally nest (maybe drm/sched job fence vs hw fence). So perhaps too much. -Daniel