job_lock is taken by the V3D interrupt handler, so nesting an IRQ-unsafe
lock under it creates an inversion. drm_syncobj_replace_fence() takes
drm_syncobj::lock, which other paths acquire with interrupts enabled. For
example, a thread interrupted while holding it blocks the handler on
job_lock, and the handler can only make progress once the submit path
releases job_lock, which it cannot do while waiting for drm_syncobj::lock.
This is reported by lockdep:
[ 188.139076] ========================================================
[ 188.139086] WARNING: possible irq lock inversion dependency detected
[ 188.139096] 7.3.0-rc3-00067-gc4f3feeaf60f #67 Tainted: G C
[ 188.139108] --------------------------------------------------------
[ 188.139116] swapper/0/0 just changed the state of lock:
[ 188.139125] ffff0000054f8ff8 (&vc4->job_lock){-...}-{3:3}, at:
vc4_irq+0xa8/0x28c [vc4]
[ 188.139234] but this lock took another, HARDIRQ-unsafe lock in the past:
[ 188.139242] (&syncobj->lock){+.+.}-{3:3}
[ 188.139256] and interrupts could create inverse lock ordering between them.
[ 188.139267] other info that might help us debug this:
[ 188.139275] Possible interrupt unsafe locking scenario:
[ 188.139284] CPU0 CPU1
[ 188.139291] ---- ----
[ 188.139299] lock(&syncobj->lock);
[ 188.139312] local_irq_disable();
[ 188.139320] lock(&vc4->job_lock);
[ 188.139335] lock(&syncobj->lock);
[ 188.139350] <Interrupt>
[ 188.139358] lock(&vc4->job_lock);
[ 188.139372] *** DEADLOCK ***
Hand the fence over after job_lock is dropped. The job can complete and
drop exec->fence in that window, so take a reference first and publish
through the local pointer, as exec may already be gone.
Fixes: e84fcb95e074 ("drm/vc4: Export fence through syncobj")
Signed-off-by: Maíra Canal <[email protected]>
---
drivers/gpu/drm/vc4/vc4_gem.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 6ee4bd77d433..f4be154d4eb0 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -639,8 +639,11 @@ vc4_queue_submit(struct drm_device *dev, struct
vc4_exec_info *exec,
fence->seqno = exec->seqno;
exec->fence = &fence->base;
+ /* The job can complete and drop exec->fence as soon as job_lock is
+ * released, so hold our own reference.
+ */
if (out_sync)
- drm_syncobj_replace_fence(out_sync, exec->fence);
+ dma_fence_get(&fence->base);
vc4_attach_fences(exec);
@@ -662,6 +665,11 @@ vc4_queue_submit(struct drm_device *dev, struct
vc4_exec_info *exec,
spin_unlock_irqrestore(&vc4->job_lock, irqflags);
+ if (out_sync) {
+ drm_syncobj_replace_fence(out_sync, &fence->base);
+ dma_fence_put(&fence->base);
+ }
+
return 0;
}
--
2.55.0
