[PATCH 2/2] drm/i915/active: Reorder nested locking

Janusz Krzysztofik janusz.krzysztofik at linux.intel.com
Tue Jul 18 08:53:39 UTC 2023


Pre-merge results indicate locking dependency issue.  Try with the
order of locks reverted.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
---
 drivers/gpu/drm/i915/i915_active.c | 39 ++++++++++++++++++------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 53c2c669ad003..38333831a360a 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -1059,14 +1059,13 @@ __i915_active_fence_set(struct i915_active_fence *active,
 	 * Both A and B have got a reference to C or NULL, depending on the
 	 * timing of the interrupt handler.  Let's assume that if A has got C
 	 * then it has locked C first (before B).
-	 *
-	 * Note the strong ordering of the timeline also provides consistent
-	 * nesting rules for the fence->lock; the inner lock is always the
-	 * older lock.
 	 */
-	spin_lock_irqsave(fence->lock, flags);
-	if (prev)
-		spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING);
+	if (prev) {
+		spin_lock_irqsave(prev->lock, flags);
+		spin_lock_nested(fence->lock, SINGLE_DEPTH_NESTING);
+	} else {
+		spin_lock_irqsave(fence->lock, flags);
+	}
 
 	/*
 	 * A does the cmpxchg first, and so it sees C or NULL, as before, or
@@ -1080,17 +1079,22 @@ __i915_active_fence_set(struct i915_active_fence *active,
 	 */
 	while (!try_cmpxchg(__active_fence_slot(active), &prev, fence)) {
 		if (prev) {
-			spin_unlock(prev->lock);
+			spin_unlock(fence->lock);
+			spin_unlock_irqrestore(prev->lock, flags);
 			dma_fence_put(prev);
+		} else {
+			spin_unlock_irqrestore(fence->lock, flags);
 		}
-		spin_unlock_irqrestore(fence->lock, flags);
 
 		prev = i915_active_fence_get(active);
 		GEM_BUG_ON(prev == fence);
 
-		spin_lock_irqsave(fence->lock, flags);
-		if (prev)
-			spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING);
+		if (prev) {
+			spin_lock_irqsave(prev->lock, flags);
+			spin_lock_nested(fence->lock, SINGLE_DEPTH_NESTING);
+		} else {
+			spin_lock_irqsave(fence->lock, flags);
+		}
 	}
 
 	/*
@@ -1105,12 +1109,15 @@ __i915_active_fence_set(struct i915_active_fence *active,
 	 * it to complete its transition and takes over the occupancy for
 	 * itself -- remembering that it needs to wait on A before executing.
 	 */
-	if (prev) {
+	if (prev)
 		__list_del_entry(&active->cb.node);
-		spin_unlock(prev->lock); /* serialise with prev->cb_list */
-	}
 	list_add_tail(&active->cb.node, &fence->cb_list);
-	spin_unlock_irqrestore(fence->lock, flags);
+	if (prev) {
+		spin_unlock(fence->lock);
+		spin_unlock_irqrestore(prev->lock, flags);
+	} else {
+		spin_unlock_irqrestore(fence->lock, flags);
+	}
 
 	return prev;
 }
-- 
2.41.0



More information about the Intel-gfx-trybot mailing list