[PATCH 3/4] drm/i915/active: Serialize use of barriers as fence trackers

[Janusz Krzysztofik]

When adding a request to a composite tracker, we try to use an existing
fence tracker already registered with that composite tracker.  The tracker
we obtain can already track another fence, can be an idle barrier, or an
active barrier.

When we acquire an idle barrier, we don't claim it in any way until
__i915_active_fence_set() we call substitutes its NULL fence pointer with
that of our request's fence.  But another thread looking for an idle
barrier can race with us.  If that thread is collecting barriers for
preallocation, it may update the NULL fence pointer with ERR_PTR(-EAGAIN)
barrier mark, either before or after we manage to replace it with our
request fence.  It can also corrupt our callback list pointers when
reusing them as an engine pointer and a preallocated barriers llist node
link, or we can corrupt their data.

When we acquire a non-idle barrier in turn, we try to delete that barrier
from a list of barrier tasks it belongs to.  If that deletion succeedes
then we convert the barrier to an idle barrier by replacing its barrier
mark with NULL and decermenting active count of its hosting composite
tracker.  But as soon as we do this, we expose that barrier to the above
described idle barrier race.

Claim acquired idle barrier immediately by marking it with
ERR_PTR(-EAGAIN) barrier mark.  Serialize that operations with other
threads trying to claim a barrier and go back for picking up another
tracker if another thread has won the race.

Furthermore, on successful deletion of a non-idle barrier from a barrier
tasks list, don't overwrite the barrier mark with NULL -- that's not
needed at the moment since the barrier, once deleted from its list, can no
longer be acquired by any other thread that respects deletion results.
Also, don't decrease acitve counter of the hosting composite tracker but
skip follow up increase of it instead.

For the above to work correctly, teach __i915_active_fence_set function to
recognize and process non-idle barriers when requested.

Fixes: d8af05ff38ae ("drm/i915: Allow sharing the idle-barrier from other kernel requests")
References: https://gitlab.freedesktop.org/drm/intel/-/issues/6333
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
Cc: stable at vger.kernel.org # v5.4
---
 drivers/gpu/drm/i915/i915_active.c | 65 ++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 84d0a4c16eb06..8eb10af7928f4 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -425,11 +425,17 @@ replace_barrier(struct i915_active *ref, struct i915_active_fence *active)
 	return __active_del_barrier(ref, node_from_active(active));
 }
 
+static inline bool is_idle_barrier(struct active_node *node, u64 idx);
+static struct dma_fence *
+____i915_active_fence_set(struct i915_active_fence *active,
+			  struct dma_fence *fence, bool barrier);
+
 int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
 {
 	u64 idx = i915_request_timeline(rq)->fence_context;
 	struct dma_fence *fence = &rq->fence;
 	struct i915_active_fence *active;
+	bool replaced;
 	int err;
 
 	/* Prevent reaping in case we malloc/wait while building the tree */
@@ -444,13 +450,18 @@ int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
 			goto out;
 		}
 
-		if (replace_barrier(ref, active)) {
-			RCU_INIT_POINTER(active->fence, NULL);
-			atomic_dec(&ref->count);
-		}
-	} while (is_barrier(active));
+		replaced = replace_barrier(ref, active);
+		if (replaced)
+			break;
+
+		if (!cmpxchg(__active_fence_slot(active), NULL,
+			     ERR_PTR(-EAGAIN)))
+			break;
 
-	if (!__i915_active_fence_set(active, fence))
+	} while (IS_ERR_OR_NULL(rcu_access_pointer(active->fence)));
+
+	if (!____i915_active_fence_set(active, fence, is_barrier(active)) &&
+	    !replaced)
 		__i915_active_acquire(ref);
 
 out:
@@ -1021,21 +1032,9 @@ void i915_request_add_active_barriers(struct i915_request *rq)
 	spin_unlock_irqrestore(&rq->lock, flags);
 }
 
-/*
- * __i915_active_fence_set: Update the last active fence along its timeline
- * @active: the active tracker
- * @fence: the new fence (under construction)
- *
- * Records the new @fence as the last active fence along its timeline in
- * this active tracker, moving the tracking callbacks from the previous
- * fence onto this one. Returns the previous fence (if not already completed),
- * which the caller must ensure is executed before the new fence. To ensure
- * that the order of fences within the timeline of the i915_active_fence is
- * understood, it should be locked by the caller.
- */
-struct dma_fence *
-__i915_active_fence_set(struct i915_active_fence *active,
-			struct dma_fence *fence)
+static struct dma_fence *
+____i915_active_fence_set(struct i915_active_fence *active,
+			  struct dma_fence *fence, bool barrier)
 {
 	struct dma_fence *prev;
 	unsigned long flags;
@@ -1067,6 +1066,11 @@ __i915_active_fence_set(struct i915_active_fence *active,
 	 */
 	spin_lock_irqsave(fence->lock, flags);
 	prev = xchg(__active_fence_slot(active), fence);
+	if (barrier) {
+		GEM_BUG_ON(!IS_ERR(prev));
+		prev = NULL;
+	}
+	GEM_BUG_ON(IS_ERR(prev));
 	if (prev) {
 		GEM_BUG_ON(prev == fence);
 		spin_lock_nested(prev->lock, SINGLE_DEPTH_NESTING);
@@ -1079,6 +1083,25 @@ __i915_active_fence_set(struct i915_active_fence *active,
 	return prev;
 }
 
+/*
+ * __i915_active_fence_set: Update the last active fence along its timeline
+ * @active: the active tracker
+ * @fence: the new fence (under construction)
+ *
+ * Records the new @fence as the last active fence along its timeline in
+ * this active tracker, moving the tracking callbacks from the previous
+ * fence onto this one. Returns the previous fence (if not already completed),
+ * which the caller must ensure is executed before the new fence. To ensure
+ * that the order of fences within the timeline of the i915_active_fence is
+ * understood, it should be locked by the caller.
+ */
+struct dma_fence *
+__i915_active_fence_set(struct i915_active_fence *active,
+			struct dma_fence *fence)
+{
+	return ____i915_active_fence_set(active, fence, false);
+}
+
 int i915_active_fence_set(struct i915_active_fence *active,
 			  struct i915_request *rq)
 {
-- 
2.25.1