[Intel-gfx] [PATCH 1/5] drm/i915: Be wary of data races when reading the active execlists

[Intel-gfx] [PATCH 1/5] drm/i915: Be wary of data races when reading the active execlists

[Chris Wilson]

[ 1413.563200] BUG: KCSAN: data-race in __await_execution+0x217/0x370 [i915]
[ 1413.563221]
[ 1413.563236] race at unknown origin, with read to 0xffff88885bb6c478 of 8 bytes by task 9654 on cpu 1:
[ 1413.563548]  __await_execution+0x217/0x370 [i915]
[ 1413.563891]  i915_request_await_dma_fence+0x4eb/0x6a0 [i915]
[ 1413.564235]  i915_request_await_object+0x421/0x490 [i915]
[ 1413.564577]  i915_gem_do_execbuffer+0x29b7/0x3c40 [i915]
[ 1413.564967]  i915_gem_execbuffer2_ioctl+0x22f/0x5c0 [i915]
[ 1413.564998]  drm_ioctl_kernel+0x156/0x1b0
[ 1413.565022]  drm_ioctl+0x2ff/0x480
[ 1413.565046]  __x64_sys_ioctl+0x87/0xd0
[ 1413.565069]  do_syscall_64+0x4d/0x80
[ 1413.565094]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

To complicate matters, we have to both avoid the read tearing of *active
and avoid any write tearing as perform the pending[] -> inflight[]
promotion of the execlists.

v2: When in doubt, write the same comment again.

Fixes: b55230e5e800 ("drm/i915: Check for awaits on still currently executing requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 15 +++++++++++----
 drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e0280a672f1d..29c0fde8b4df 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2060,6 +2060,14 @@ static inline void clear_ports(struct i915_request **ports, int count)
 	memset_p((void **)ports, NULL, count);
 }
 
+static inline void
+copy_ports(struct i915_request **dst, struct i915_request **src, int count)
+{
+	/* A memcpy_p() would be very useful here! */
+	while (count--)
+		WRITE_ONCE(*dst++, *src++); /* avoid write tearing */
+}
+
 static void execlists_dequeue(struct intel_engine_cs *engine)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -2648,10 +2656,9 @@ static void process_csb(struct intel_engine_cs *engine)
 
 			/* switch pending to inflight */
 			GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
-			memcpy(execlists->inflight,
-			       execlists->pending,
-			       execlists_num_ports(execlists) *
-			       sizeof(*execlists->pending));
+			copy_ports(execlists->inflight,
+				   execlists->pending,
+				   execlists_num_ports(execlists));
 			smp_wmb(); /* complete the seqlock */
 			WRITE_ONCE(execlists->active, execlists->inflight);
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 0b2fe55e6194..781a6783affe 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -388,17 +388,38 @@ static bool __request_in_flight(const struct i915_request *signal)
 	 * As we know that there are always preemption points between
 	 * requests, we know that only the currently executing request
 	 * may be still active even though we have cleared the flag.
-	 * However, we can't rely on our tracking of ELSP[0] to known
+	 * However, we can't rely on our tracking of ELSP[0] to know
 	 * which request is currently active and so maybe stuck, as
 	 * the tracking maybe an event behind. Instead assume that
 	 * if the context is still inflight, then it is still active
 	 * even if the active flag has been cleared.
+	 *
+	 * To further complicate matters, if there a pending promotion, the HW
+	 * may either perform a context switch to the second inflight execlists,
+	 * or it may switch to the pending set of execlists. In the case of the
+	 * latter, it may send the ACK and we process the event copying the
+	 * pending[] over top of inflight[], _overwriting_ our *active. Since
+	 * this implies the HW is arbitrating and not struck in *active, we do
+	 * not worry about complete accuracy, but we do require no read/write
+	 * tearing of the pointer [the read of the pointer must be valid, even
+	 * as the array is being overwritten, for which we require the writes
+	 * to avoid tearing.]
+	 *
+	 * Note that the read of *execlists->active may race with the promotion
+	 * of execlists->pending[] to execlists->inflight[], overwritting
+	 * the value at *execlists->active. This is fine. The promotion implies
+	 * that we received an ACK from the HW, and so the context is not
+	 * stuck -- if we do not see ourselves in *active, the inflight status
+	 * is valid. If instead we see ourselves being copied into *active,
+	 * we are inflight and may signal the callback.
 	 */
 	if (!intel_context_inflight(signal->context))
 		return false;
 
 	rcu_read_lock();
-	for (port = __engine_active(signal->engine); (rq = *port); port++) {
+	for (port = __engine_active(signal->engine);
+	     (rq = READ_ONCE(*port)); /* may race with promotion of pending[] */
+	     port++) {
 		if (rq->context == signal->context) {
 			inflight = i915_seqno_passed(rq->fence.seqno,
 						     signal->fence.seqno);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/5] drm/i915: Remove i915_request.lock requirement for execution callbacks

[Chris Wilson]

We are using the i915_request.lock to serialise adding an execution
callback with __i915_request_submit. However, if we use an atomic
llist_add to serialise multiple waiters and then check to see if the
request is already executing, we can remove the irq-spinlock.

v2: Avoid using the irq_work when outside of the irq-spinlocks, where we
can execute the callbacks immediately.
v3: Pay close attention to the order of setting ACTIVE on retirement, we
need to ensure the request is signaled and breadcrumbs detached before
we finish removing the request from the engine.

Fixes: 1d9221e9d395 ("drm/i915: Skip signaling a signaled request")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 121 ++++++++++++++++------------
 1 file changed, 70 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 781a6783affe..2ef17b11ca4b 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -186,30 +186,34 @@ static void irq_execute_cb_hook(struct irq_work *wrk)
 	irq_execute_cb(wrk);
 }
 
-static void __notify_execute_cb(struct i915_request *rq)
+static __always_inline void
+__notify_execute_cb(struct i915_request *rq, bool (*fn)(struct irq_work *wrk))
 {
 	struct execute_cb *cb, *cn;
 
-	lockdep_assert_held(&rq->lock);
-
-	GEM_BUG_ON(!i915_request_is_active(rq));
 	if (llist_empty(&rq->execute_cb))
 		return;
 
-	llist_for_each_entry_safe(cb, cn, rq->execute_cb.first, work.llnode)
-		irq_work_queue(&cb->work);
+	llist_for_each_entry_safe(cb, cn,
+				  llist_del_all(&rq->execute_cb),
+				  work.llnode)
+		fn(&cb->work);
+}
 
-	/*
-	 * XXX Rollback on __i915_request_unsubmit()
-	 *
-	 * In the future, perhaps when we have an active time-slicing scheduler,
-	 * it will be interesting to unsubmit parallel execution and remove
-	 * busywaits from the GPU until their master is restarted. This is
-	 * quite hairy, we have to carefully rollback the fence and do a
-	 * preempt-to-idle cycle on the target engine, all the while the
-	 * master execute_cb may refire.
-	 */
-	init_llist_head(&rq->execute_cb);
+static void __notify_execute_cb_irq(struct i915_request *rq)
+{
+	__notify_execute_cb(rq, irq_work_queue);
+}
+
+static bool irq_work_imm(struct irq_work *wrk)
+{
+	wrk->func(wrk);
+	return false;
+}
+
+static void __notify_execute_cb_imm(struct i915_request *rq)
+{
+	__notify_execute_cb(rq, irq_work_imm);
 }
 
 static inline void
@@ -274,9 +278,14 @@ static void remove_from_engine(struct i915_request *rq)
 		locked = engine;
 	}
 	list_del_init(&rq->sched.link);
+	spin_unlock_irq(&locked->active.lock);
+
 	clear_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
 	clear_bit(I915_FENCE_FLAG_HOLD, &rq->fence.flags);
-	spin_unlock_irq(&locked->active.lock);
+
+	/* Prevent further __await_execution() registering a cb, then flush */
+	set_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags);
+	__notify_execute_cb_imm(rq);
 }
 
 bool i915_request_retire(struct i915_request *rq)
@@ -288,6 +297,7 @@ bool i915_request_retire(struct i915_request *rq)
 
 	GEM_BUG_ON(!i915_sw_fence_signaled(&rq->submit));
 	trace_i915_request_retire(rq);
+	i915_request_mark_complete(rq);
 
 	/*
 	 * We know the GPU must have read the request to have
@@ -305,16 +315,7 @@ bool i915_request_retire(struct i915_request *rq)
 		__i915_request_fill(rq, POISON_FREE);
 	rq->ring->head = rq->postfix;
 
-	/*
-	 * We only loosely track inflight requests across preemption,
-	 * and so we may find ourselves attempting to retire a _completed_
-	 * request that we have removed from the HW and put back on a run
-	 * queue.
-	 */
-	remove_from_engine(rq);
-
 	spin_lock_irq(&rq->lock);
-	i915_request_mark_complete(rq);
 	if (!i915_request_signaled(rq))
 		dma_fence_signal_locked(&rq->fence);
 	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
@@ -323,13 +324,21 @@ bool i915_request_retire(struct i915_request *rq)
 		GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
 		atomic_dec(&rq->engine->gt->rps.num_waiters);
 	}
-	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags)) {
-		set_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags);
-		__notify_execute_cb(rq);
-	}
-	GEM_BUG_ON(!llist_empty(&rq->execute_cb));
 	spin_unlock_irq(&rq->lock);
 
+	/*
+	 * We only loosely track inflight requests across preemption,
+	 * and so we may find ourselves attempting to retire a _completed_
+	 * request that we have removed from the HW and put back on a run
+	 * queue.
+	 *
+	 * As we set I915_FENCE_FLAG_ACTIVE on the request, this should be
+	 * after removing the breadcrumb and signaling it, so that we do not
+	 * inadvertently attach the breadcrumb to a completed request.
+	 */
+	remove_from_engine(rq);
+	GEM_BUG_ON(!llist_empty(&rq->execute_cb));
+
 	remove_from_client(rq);
 	__list_del_entry(&rq->link); /* poison neither prev/next (RCU walks) */
 
@@ -357,12 +366,6 @@ void i915_request_retire_upto(struct i915_request *rq)
 	} while (i915_request_retire(tmp) && tmp != rq);
 }
 
-static void __llist_add(struct llist_node *node, struct llist_head *head)
-{
-	node->next = head->first;
-	head->first = node;
-}
-
 static struct i915_request * const *
 __engine_active(struct intel_engine_cs *engine)
 {
@@ -460,18 +463,24 @@ __await_execution(struct i915_request *rq,
 		cb->work.func = irq_execute_cb_hook;
 	}
 
-	spin_lock_irq(&signal->lock);
-	if (i915_request_is_active(signal) || __request_in_flight(signal)) {
-		if (hook) {
-			hook(rq, &signal->fence);
-			i915_request_put(signal);
-		}
-		i915_sw_fence_complete(cb->fence);
-		kmem_cache_free(global.slab_execute_cbs, cb);
-	} else {
-		__llist_add(&cb->work.llnode, &signal->execute_cb);
+	/*
+	 * Register the callback first, then see if the signaler is already
+	 * active. This ensures that if we race with the
+	 * __notify_execute_cb from i915_request_submit() and we are not
+	 * included in that list, we get a second bite of the cherry and
+	 * execute it ourselves. After this point, a future
+	 * i915_request_submit() will notify us.
+	 *
+	 * In i915_request_retire() we set the ACTIVE bit on a completed
+	 * request (then flush the execute_cb). So by registering the
+	 * callback first, then checking the ACTIVE bit, we serialise with
+	 * the completed/retired request.
+	 */
+	if (llist_add(&cb->work.llnode, &signal->execute_cb)) {
+		if (i915_request_is_active(signal) ||
+		    __request_in_flight(signal))
+			__notify_execute_cb_imm(signal);
 	}
-	spin_unlock_irq(&signal->lock);
 
 	return 0;
 }
@@ -587,18 +596,28 @@ bool __i915_request_submit(struct i915_request *request)
 		clear_bit(I915_FENCE_FLAG_PQUEUE, &request->fence.flags);
 	}
 
+	/*
+	 * XXX Rollback bonded-execution on __i915_request_unsubmit()?
+	 *
+	 * In the future, perhaps when we have an active time-slicing scheduler,
+	 * it will be interesting to unsubmit parallel execution and remove
+	 * busywaits from the GPU until their master is restarted. This is
+	 * quite hairy, we have to carefully rollback the fence and do a
+	 * preempt-to-idle cycle on the target engine, all the while the
+	 * master execute_cb may refire.
+	 */
+	__notify_execute_cb_irq(request);
+
 	/* We may be recursing from the signal callback of another i915 fence */
 	if (!i915_request_signaled(request)) {
 		spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
 
-		__notify_execute_cb(request);
 		if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 			     &request->fence.flags) &&
 		    !i915_request_enable_breadcrumb(request))
 			intel_engine_signal_breadcrumbs(engine);
 
 		spin_unlock(&request->lock);
-		GEM_BUG_ON(!llist_empty(&request->execute_cb));
 	}
 
 	return result;
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/5] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs

[Chris Wilson]

Since the breadcrumb enabling/cancelling itself is serialised by the
breadcrumbs.irq_lock, with a bit of care we can remove the outer
serialisation with i915_request.lock for concurrent
dma_fence_enable_signaling(). This has the important side-effect of
eliminating the nested i915_request.lock within request submission.

The challenge in serialisation is around the unsubmission where we take
an active request that wants a breadcrumb on the signaling engine and
put it to sleep. We do not want a concurrent
dma_fence_enable_signaling() to attach a breadcrumb as we unsubmit, so
we must mark the request as no longer active before serialising with the
concurrent enable-signaling.

On retire, we serialise with the concurrent enable-signaling, but
instead of clearing ACTIVE, we mark it as SIGNALED.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 130 +++++++++++++-------
 drivers/gpu/drm/i915/gt/intel_lrc.c         |  14 ---
 drivers/gpu/drm/i915/i915_request.c         |  39 +++---
 3 files changed, 100 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 91786310c114..a0f52417238c 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -220,17 +220,17 @@ static void signal_irq_work(struct irq_work *work)
 	}
 }
 
-static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
+static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
 {
 	struct intel_engine_cs *engine =
 		container_of(b, struct intel_engine_cs, breadcrumbs);
 
 	lockdep_assert_held(&b->irq_lock);
 	if (b->irq_armed)
-		return true;
+		return;
 
 	if (!intel_gt_pm_get_if_awake(engine->gt))
-		return false;
+		return;
 
 	/*
 	 * The breadcrumb irq will be disarmed on the interrupt after the
@@ -250,8 +250,6 @@ static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
 
 	if (!b->irq_enabled++)
 		irq_enable(engine);
-
-	return true;
 }
 
 void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
@@ -310,57 +308,99 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 {
 }
 
-bool i915_request_enable_breadcrumb(struct i915_request *rq)
+static void insert_breadcrumb(struct i915_request *rq,
+			      struct intel_breadcrumbs *b)
 {
-	lockdep_assert_held(&rq->lock);
+	struct intel_context *ce = rq->context;
+	struct list_head *pos;
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
-		return true;
+	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
+		return;
 
-	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags)) {
-		struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
-		struct intel_context *ce = rq->context;
-		struct list_head *pos;
+	__intel_breadcrumbs_arm_irq(b);
 
-		spin_lock(&b->irq_lock);
+	/*
+	 * We keep the seqno in retirement order, so we can break
+	 * inside intel_engine_signal_breadcrumbs as soon as we've
+	 * passed the last completed request (or seen a request that
+	 * hasn't event started). We could walk the timeline->requests,
+	 * but keeping a separate signalers_list has the advantage of
+	 * hopefully being much smaller than the full list and so
+	 * provides faster iteration and detection when there are no
+	 * more interrupts required for this context.
+	 *
+	 * We typically expect to add new signalers in order, so we
+	 * start looking for our insertion point from the tail of
+	 * the list.
+	 */
+	list_for_each_prev(pos, &ce->signals) {
+		struct i915_request *it =
+			list_entry(pos, typeof(*it), signal_link);
 
-		if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
-			goto unlock;
+		if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
+			break;
+	}
+	list_add(&rq->signal_link, pos);
+	if (pos == &ce->signals) /* catch transitions from empty list */
+		list_move_tail(&ce->signal_link, &b->signalers);
+	GEM_BUG_ON(!check_signal_order(ce, rq));
 
-		if (!__intel_breadcrumbs_arm_irq(b))
-			goto unlock;
+	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+}
 
-		/*
-		 * We keep the seqno in retirement order, so we can break
-		 * inside intel_engine_signal_breadcrumbs as soon as we've
-		 * passed the last completed request (or seen a request that
-		 * hasn't event started). We could walk the timeline->requests,
-		 * but keeping a separate signalers_list has the advantage of
-		 * hopefully being much smaller than the full list and so
-		 * provides faster iteration and detection when there are no
-		 * more interrupts required for this context.
-		 *
-		 * We typically expect to add new signalers in order, so we
-		 * start looking for our insertion point from the tail of
-		 * the list.
-		 */
-		list_for_each_prev(pos, &ce->signals) {
-			struct i915_request *it =
-				list_entry(pos, typeof(*it), signal_link);
+bool i915_request_enable_breadcrumb(struct i915_request *rq)
+{
+	struct intel_breadcrumbs *b;
 
-			if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
-				break;
-		}
-		list_add(&rq->signal_link, pos);
-		if (pos == &ce->signals) /* catch transitions from empty list */
-			list_move_tail(&ce->signal_link, &b->signalers);
-		GEM_BUG_ON(!check_signal_order(ce, rq));
+	/* Seralises with i915_request_retire() using rq->lock */
+	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
+		return true;
 
-		set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
-unlock:
+	/*
+	 * Peek at i915_request_submit()/i915_request_unsubmit() status.
+	 *
+	 * If the request is not yet active (and not signaled), we will
+	 * attach the breadcrumb later.
+	 */
+	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
+		return true;
+
+	/*
+	 * rq->engine is locked by rq->engine->active.lock. That however
+	 * is not known until after rq->engine has been dereferenced and
+	 * the lock acquired. Hence we acquire the lock and then validate
+	 * that rq->engine still matches the lock we hold for it.
+	 *
+	 * Here, we are using the breadcrumb lock as a proxy for the
+	 * rq->engine->active.lock, and we know that since the breadcrumb
+	 * will be serialised within i915_request_submit/i915_request_unsubmit,
+	 * the engine cannot change while active as long as we hold the
+	 * breadcrumb lock on that engine.
+	 *
+	 * From the dma_fence_enable_signaling() path, we are outside of the
+	 * request submit/unsubmit path, and so we must be more careful to
+	 * acquire the right lock.
+	 */
+	b = &READ_ONCE(rq->engine)->breadcrumbs;
+	spin_lock(&b->irq_lock);
+	while (unlikely(b != &READ_ONCE(rq->engine)->breadcrumbs)) {
 		spin_unlock(&b->irq_lock);
+		b = &READ_ONCE(rq->engine)->breadcrumbs;
+		spin_lock(&b->irq_lock);
 	}
 
+	/*
+	 * Now that we are finally serialised with request submit/unsubmit,
+	 * [with b->irq_lock] and with i915_request_reitre() [via checking
+	 * SIGNALED with rq->lock] confirm the request is indeed active. If
+	 * it is no longer active, the breadcrumb will be attached upon
+	 * i915_request_submit().
+	 */
+	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
+		insert_breadcrumb(rq, b);
+
+	spin_unlock(&b->irq_lock);
+
 	return !__request_completed(rq);
 }
 
@@ -368,8 +408,6 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
 {
 	struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
 
-	lockdep_assert_held(&rq->lock);
-
 	/*
 	 * We must wait for b->irq_lock so that we know the interrupt handler
 	 * has released its reference to the intel_context and has completed
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 29c0fde8b4df..21c16e31c4fe 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1148,20 +1148,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
 		} else {
 			struct intel_engine_cs *owner = rq->context->engine;
 
-			/*
-			 * Decouple the virtual breadcrumb before moving it
-			 * back to the virtual engine -- we don't want the
-			 * request to complete in the background and try
-			 * and cancel the breadcrumb on the virtual engine
-			 * (instead of the old engine where it is linked)!
-			 */
-			if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
-				     &rq->fence.flags)) {
-				spin_lock_nested(&rq->lock,
-						 SINGLE_DEPTH_NESTING);
-				i915_request_cancel_breadcrumb(rq);
-				spin_unlock(&rq->lock);
-			}
 			WRITE_ONCE(rq->engine, owner);
 			owner->submit_request(rq);
 			active = NULL;
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 2ef17b11ca4b..8c345ead04a6 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -320,11 +320,12 @@ bool i915_request_retire(struct i915_request *rq)
 		dma_fence_signal_locked(&rq->fence);
 	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
 		i915_request_cancel_breadcrumb(rq);
+	spin_unlock_irq(&rq->lock);
+
 	if (i915_request_has_waitboost(rq)) {
 		GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
 		atomic_dec(&rq->engine->gt->rps.num_waiters);
 	}
-	spin_unlock_irq(&rq->lock);
 
 	/*
 	 * We only loosely track inflight requests across preemption,
@@ -608,17 +609,9 @@ bool __i915_request_submit(struct i915_request *request)
 	 */
 	__notify_execute_cb_irq(request);
 
-	/* We may be recursing from the signal callback of another i915 fence */
-	if (!i915_request_signaled(request)) {
-		spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
-
-		if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
-			     &request->fence.flags) &&
-		    !i915_request_enable_breadcrumb(request))
-			intel_engine_signal_breadcrumbs(engine);
-
-		spin_unlock(&request->lock);
-	}
+	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
+	    !i915_request_enable_breadcrumb(request))
+		intel_engine_signal_breadcrumbs(engine);
 
 	return result;
 }
@@ -640,27 +633,27 @@ void __i915_request_unsubmit(struct i915_request *request)
 {
 	struct intel_engine_cs *engine = request->engine;
 
+	/*
+	 * Only unwind in reverse order, required so that the per-context list
+	 * is kept in seqno/ring order.
+	 */
 	RQ_TRACE(request, "\n");
 
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->active.lock);
 
 	/*
-	 * Only unwind in reverse order, required so that the per-context list
-	 * is kept in seqno/ring order.
+	 * Before we remove this breadcrumb from the signal list, we have
+	 * to ensure that a concurrent dma_fence_enable_signaling() does not
+	 * attach itself. We first mark the request as no longer active and
+	 * make sure that is visible to other cores, and then remove the
+	 * breadcrumb if attached.
 	 */
-
-	/* We may be recursing from the signal callback of another i915 fence */
-	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
-
+	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
+	clear_bit_unlock(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
 	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
 		i915_request_cancel_breadcrumb(request);
 
-	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
-	clear_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
-
-	spin_unlock(&request->lock);
-
 	/* We've already spun, don't charge on resubmitting. */
 	if (request->sched.semaphores && i915_request_started(request))
 		request->sched.semaphores = 0;
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/5] drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs

[Chris Wilson]

Now that we have serialised the request retirement's decoupling of the
breadcrumb from the engine with the request signaling, we know that
there should never be a stale breadcrumb attached to an unbound virtual
engine. We can now remove the fixup code that had to migrate the
breadcrumbs along with the virtual engine, from one sibling to the next.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c  | 29 --------------------
 drivers/gpu/drm/i915/gt/intel_engine.h       |  3 --
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  2 --
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 15 ----------
 4 files changed, 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index a0f52417238c..164662ae130b 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -167,8 +167,6 @@ static void signal_irq_work(struct irq_work *work)
 	if (b->irq_armed && list_empty(&b->signalers))
 		__intel_breadcrumbs_disarm_irq(b);
 
-	list_splice_init(&b->signaled_requests, &signal);
-
 	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
 		GEM_BUG_ON(list_empty(&ce->signals));
 
@@ -258,7 +256,6 @@ void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
 
 	spin_lock_init(&b->irq_lock);
 	INIT_LIST_HEAD(&b->signalers);
-	INIT_LIST_HEAD(&b->signaled_requests);
 
 	init_irq_work(&b->irq_work, signal_irq_work);
 }
@@ -278,32 +275,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&b->irq_lock, flags);
 }
 
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
-					     struct intel_context *ce)
-{
-	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	unsigned long flags;
-
-	spin_lock_irqsave(&b->irq_lock, flags);
-	if (!list_empty(&ce->signals)) {
-		struct i915_request *rq, *next;
-
-		/* Queue for executing the signal callbacks in the irq_work */
-		list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
-			GEM_BUG_ON(rq->engine != engine);
-			GEM_BUG_ON(!__request_completed(rq));
-
-			__signal_request(rq, &b->signaled_requests);
-		}
-
-		INIT_LIST_HEAD(&ce->signals);
-		list_del_init(&ce->signal_link);
-
-		irq_work_queue(&b->irq_work);
-	}
-	spin_unlock_irqrestore(&b->irq_lock, flags);
-}
-
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 {
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index a9249a23903a..faf00a353e25 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
 void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
 
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
-					     struct intel_context *ce);
-
 void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
 				    struct drm_printer *p);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 8de92fd7d392..e0a2ceac729f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -393,8 +393,6 @@ struct intel_engine_cs {
 		spinlock_t irq_lock;
 		struct list_head signalers;
 
-		struct list_head signaled_requests;
-
 		struct irq_work irq_work; /* for use from inside irq_lock */
 
 		unsigned int irq_enabled;
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 21c16e31c4fe..88a5c155154d 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
 	return true;
 }
 
-static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
-{
-	/*
-	 * All the outstanding signals on ve->siblings[0] must have
-	 * been completed, just pending the interrupt handler. As those
-	 * signals still refer to the old sibling (via rq->engine), we must
-	 * transfer those to the old irq_worker to keep our locking
-	 * consistent.
-	 */
-	intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
-}
-
 #define for_each_waiter(p__, rq__) \
 	list_for_each_entry_lockless(p__, \
 				     &(rq__)->sched.waiters_list, \
@@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 					virtual_update_register_offsets(regs,
 									engine);
 
-				if (!list_empty(&ve->context.signals))
-					virtual_xfer_breadcrumbs(ve);
-
 				/*
 				 * Move the bound engine to the top of the list
 				 * for future execution. We then kick this
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/5] drm/i915/gt: Only transfer the virtual context to the new engine if active

[Chris Wilson]

One more complication of preempt-to-busy with respect to the virtual
engine is that we may have retired the last request along the virtual
engine at the same time as preparing to submit the completed request to
a new engine. That submit will be shortcircuited, but not before we have
updated the context with the new register offsets and marked the virtual
engine as bound to the new engine (by calling swap on ve->siblings[]).
As we may have just retired the completed request, we may also be in the
middle of calling intel_context_exit() to turn off the power management
associated with the virtual engine, and that in turn walks the
ve->siblings[]. If we happen to call swap() on the array as we walk, we
will call intel_engine_pm_put() twice on the same engine.

In this patch, we prevent this by only updating the bound engine after a
successful submission which weeds out the already completed requests.

Alternatively, we could walk a non-volatile array for the pm, such as
using the engine->mask. The small advantage to performing the update
after the submit is that we then only have to do a swap for active
requests.

Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Nayana, Venkata Ramana" <venkata.ramana.nayana@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 65 ++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 88a5c155154d..5e8278e8ac79 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1805,6 +1805,33 @@ static bool virtual_matches(const struct virtual_engine *ve,
 	return true;
 }
 
+static void virtual_xfer_context(struct virtual_engine *ve,
+				 struct intel_engine_cs *engine)
+{
+	unsigned int n;
+
+	if (likely(engine == ve->siblings[0]))
+		return;
+
+	GEM_BUG_ON(READ_ONCE(ve->context.inflight));
+	if (!intel_engine_has_relative_mmio(engine))
+		virtual_update_register_offsets(ve->context.lrc_reg_state,
+						engine);
+
+	/*
+	 * Move the bound engine to the top of the list for
+	 * future execution. We then kick this tasklet first
+	 * before checking others, so that we preferentially
+	 * reuse this set of bound registers.
+	 */
+	for (n = 1; n < ve->num_siblings; n++) {
+		if (ve->siblings[n] == engine) {
+			swap(ve->siblings[n], ve->siblings[0]);
+			break;
+		}
+	}
+}
+
 #define for_each_waiter(p__, rq__) \
 	list_for_each_entry_lockless(p__, \
 				     &(rq__)->sched.waiters_list, \
@@ -2253,35 +2280,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			GEM_BUG_ON(!(rq->execution_mask & engine->mask));
 			WRITE_ONCE(rq->engine, engine);
 
-			if (engine != ve->siblings[0]) {
-				u32 *regs = ve->context.lrc_reg_state;
-				unsigned int n;
-
-				GEM_BUG_ON(READ_ONCE(ve->context.inflight));
-
-				if (!intel_engine_has_relative_mmio(engine))
-					virtual_update_register_offsets(regs,
-									engine);
-
+			if (__i915_request_submit(rq)) {
 				/*
-				 * Move the bound engine to the top of the list
-				 * for future execution. We then kick this
-				 * tasklet first before checking others, so that
-				 * we preferentially reuse this set of bound
-				 * registers.
+				 * Only after we confirm that we will submit
+				 * this request (i.e. it has not already
+				 * completed), do we want to update the context.
+				 *
+				 * This serves two purposes. It avoids
+				 * unnecessary work if we are resubmitting an
+				 * already completed request after timeslicing.
+				 * But more importantly, it prevents us altering
+				 * ve->siblings[] on an idle context, where
+				 * we may be using ve->siblings[] in
+				 * virtual_context_enter / virtual_context_exit.
 				 */
-				for (n = 1; n < ve->num_siblings; n++) {
-					if (ve->siblings[n] == engine) {
-						swap(ve->siblings[n],
-						     ve->siblings[0]);
-						break;
-					}
-				}
-
+				virtual_xfer_context(ve, engine);
 				GEM_BUG_ON(ve->siblings[0] != engine);
-			}
 
-			if (__i915_request_submit(rq)) {
 				submit = true;
 				last = rq;
 			}
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/5] drm/i915: Remove i915_request.lock requirement for execution callbacks

[Tvrtko Ursulin]

On 16/07/2020 12:33, Chris Wilson wrote:
> We are using the i915_request.lock to serialise adding an execution
> callback with __i915_request_submit. However, if we use an atomic
> llist_add to serialise multiple waiters and then check to see if the
> request is already executing, we can remove the irq-spinlock.
> 
> v2: Avoid using the irq_work when outside of the irq-spinlocks, where we
> can execute the callbacks immediately.
> v3: Pay close attention to the order of setting ACTIVE on retirement, we
> need to ensure the request is signaled and breadcrumbs detached before
> we finish removing the request from the engine.
> 
> Fixes: 1d9221e9d395 ("drm/i915: Skip signaling a signaled request")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 121 ++++++++++++++++------------
>   1 file changed, 70 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 781a6783affe..2ef17b11ca4b 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -186,30 +186,34 @@ static void irq_execute_cb_hook(struct irq_work *wrk)
>   	irq_execute_cb(wrk);
>   }
>   
> -static void __notify_execute_cb(struct i915_request *rq)
> +static __always_inline void
> +__notify_execute_cb(struct i915_request *rq, bool (*fn)(struct irq_work *wrk))
>   {
>   	struct execute_cb *cb, *cn;
>   
> -	lockdep_assert_held(&rq->lock);
> -
> -	GEM_BUG_ON(!i915_request_is_active(rq));
>   	if (llist_empty(&rq->execute_cb))
>   		return;
>   
> -	llist_for_each_entry_safe(cb, cn, rq->execute_cb.first, work.llnode)
> -		irq_work_queue(&cb->work);
> +	llist_for_each_entry_safe(cb, cn,
> +				  llist_del_all(&rq->execute_cb),
> +				  work.llnode)
> +		fn(&cb->work);
> +}
>   
> -	/*
> -	 * XXX Rollback on __i915_request_unsubmit()
> -	 *
> -	 * In the future, perhaps when we have an active time-slicing scheduler,
> -	 * it will be interesting to unsubmit parallel execution and remove
> -	 * busywaits from the GPU until their master is restarted. This is
> -	 * quite hairy, we have to carefully rollback the fence and do a
> -	 * preempt-to-idle cycle on the target engine, all the while the
> -	 * master execute_cb may refire.
> -	 */
> -	init_llist_head(&rq->execute_cb);
> +static void __notify_execute_cb_irq(struct i915_request *rq)
> +{
> +	__notify_execute_cb(rq, irq_work_queue);
> +}
> +
> +static bool irq_work_imm(struct irq_work *wrk)
> +{
> +	wrk->func(wrk);
> +	return false;
> +}
> +
> +static void __notify_execute_cb_imm(struct i915_request *rq)
> +{
> +	__notify_execute_cb(rq, irq_work_imm);
>   }
>   
>   static inline void
> @@ -274,9 +278,14 @@ static void remove_from_engine(struct i915_request *rq)
>   		locked = engine;
>   	}
>   	list_del_init(&rq->sched.link);
> +	spin_unlock_irq(&locked->active.lock);
> +
>   	clear_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
>   	clear_bit(I915_FENCE_FLAG_HOLD, &rq->fence.flags);
> -	spin_unlock_irq(&locked->active.lock);
> +
> +	/* Prevent further __await_execution() registering a cb, then flush */
> +	set_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags);
> +	__notify_execute_cb_imm(rq);
>   }
>   
>   bool i915_request_retire(struct i915_request *rq)
> @@ -288,6 +297,7 @@ bool i915_request_retire(struct i915_request *rq)
>   
>   	GEM_BUG_ON(!i915_sw_fence_signaled(&rq->submit));
>   	trace_i915_request_retire(rq);
> +	i915_request_mark_complete(rq);
>   
>   	/*
>   	 * We know the GPU must have read the request to have
> @@ -305,16 +315,7 @@ bool i915_request_retire(struct i915_request *rq)
>   		__i915_request_fill(rq, POISON_FREE);
>   	rq->ring->head = rq->postfix;
>   
> -	/*
> -	 * We only loosely track inflight requests across preemption,
> -	 * and so we may find ourselves attempting to retire a _completed_
> -	 * request that we have removed from the HW and put back on a run
> -	 * queue.
> -	 */
> -	remove_from_engine(rq);
> -
>   	spin_lock_irq(&rq->lock);
> -	i915_request_mark_complete(rq);
>   	if (!i915_request_signaled(rq))
>   		dma_fence_signal_locked(&rq->fence);
>   	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
> @@ -323,13 +324,21 @@ bool i915_request_retire(struct i915_request *rq)
>   		GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
>   		atomic_dec(&rq->engine->gt->rps.num_waiters);
>   	}
> -	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags)) {
> -		set_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags);
> -		__notify_execute_cb(rq);
> -	}
> -	GEM_BUG_ON(!llist_empty(&rq->execute_cb));
>   	spin_unlock_irq(&rq->lock);
>   
> +	/*
> +	 * We only loosely track inflight requests across preemption,
> +	 * and so we may find ourselves attempting to retire a _completed_
> +	 * request that we have removed from the HW and put back on a run
> +	 * queue.
> +	 *
> +	 * As we set I915_FENCE_FLAG_ACTIVE on the request, this should be
> +	 * after removing the breadcrumb and signaling it, so that we do not
> +	 * inadvertently attach the breadcrumb to a completed request.
> +	 */
> +	remove_from_engine(rq);
> +	GEM_BUG_ON(!llist_empty(&rq->execute_cb));
> +
>   	remove_from_client(rq);
>   	__list_del_entry(&rq->link); /* poison neither prev/next (RCU walks) */
>   
> @@ -357,12 +366,6 @@ void i915_request_retire_upto(struct i915_request *rq)
>   	} while (i915_request_retire(tmp) && tmp != rq);
>   }
>   
> -static void __llist_add(struct llist_node *node, struct llist_head *head)
> -{
> -	node->next = head->first;
> -	head->first = node;
> -}
> -
>   static struct i915_request * const *
>   __engine_active(struct intel_engine_cs *engine)
>   {
> @@ -460,18 +463,24 @@ __await_execution(struct i915_request *rq,
>   		cb->work.func = irq_execute_cb_hook;
>   	}
>   
> -	spin_lock_irq(&signal->lock);
> -	if (i915_request_is_active(signal) || __request_in_flight(signal)) {
> -		if (hook) {
> -			hook(rq, &signal->fence);
> -			i915_request_put(signal);
> -		}
> -		i915_sw_fence_complete(cb->fence);
> -		kmem_cache_free(global.slab_execute_cbs, cb);
> -	} else {
> -		__llist_add(&cb->work.llnode, &signal->execute_cb);
> +	/*
> +	 * Register the callback first, then see if the signaler is already
> +	 * active. This ensures that if we race with the
> +	 * __notify_execute_cb from i915_request_submit() and we are not
> +	 * included in that list, we get a second bite of the cherry and
> +	 * execute it ourselves. After this point, a future
> +	 * i915_request_submit() will notify us.
> +	 *
> +	 * In i915_request_retire() we set the ACTIVE bit on a completed
> +	 * request (then flush the execute_cb). So by registering the
> +	 * callback first, then checking the ACTIVE bit, we serialise with
> +	 * the completed/retired request.
> +	 */
> +	if (llist_add(&cb->work.llnode, &signal->execute_cb)) {
> +		if (i915_request_is_active(signal) ||
> +		    __request_in_flight(signal))
> +			__notify_execute_cb_imm(signal);
>   	}
> -	spin_unlock_irq(&signal->lock);
>   
>   	return 0;
>   }
> @@ -587,18 +596,28 @@ bool __i915_request_submit(struct i915_request *request)
>   		clear_bit(I915_FENCE_FLAG_PQUEUE, &request->fence.flags);
>   	}
>   
> +	/*
> +	 * XXX Rollback bonded-execution on __i915_request_unsubmit()?
> +	 *
> +	 * In the future, perhaps when we have an active time-slicing scheduler,
> +	 * it will be interesting to unsubmit parallel execution and remove
> +	 * busywaits from the GPU until their master is restarted. This is
> +	 * quite hairy, we have to carefully rollback the fence and do a
> +	 * preempt-to-idle cycle on the target engine, all the while the
> +	 * master execute_cb may refire.
> +	 */
> +	__notify_execute_cb_irq(request);
> +
>   	/* We may be recursing from the signal callback of another i915 fence */
>   	if (!i915_request_signaled(request)) {
>   		spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
>   
> -		__notify_execute_cb(request);
>   		if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>   			     &request->fence.flags) &&
>   		    !i915_request_enable_breadcrumb(request))
>   			intel_engine_signal_breadcrumbs(engine);
>   
>   		spin_unlock(&request->lock);
> -		GEM_BUG_ON(!llist_empty(&request->execute_cb));
>   	}
>   
>   	return result;
> 

Looks okay.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists
URL   : https://patchwork.freedesktop.org/series/79551/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
83628de655db drm/i915: Be wary of data races when reading the active execlists
-:92: WARNING:TYPO_SPELLING: 'overwritting' may be misspelled - perhaps 'overwriting'?
#92: FILE: drivers/gpu/drm/i915/i915_request.c:409:
+	 * of execlists->pending[] to execlists->inflight[], overwritting

total: 0 errors, 1 warnings, 0 checks, 67 lines checked
fa1d73d35a4f drm/i915: Remove i915_request.lock requirement for execution callbacks
70395ae3921e drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
101a6a825a8f drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs
78c9cd63de6d drm/i915/gt: Only transfer the virtual context to the new engine if active
-:28: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#28: 
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"

-:28: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 6d06779e8672 ("drm/i915: Load balancing across a virtual engine")'
#28: 
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"

total: 1 errors, 1 warnings, 0 checks, 81 lines checked


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists
URL   : https://patchwork.freedesktop.org/series/79551/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 5505 bytes --]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists
URL   : https://patchwork.freedesktop.org/series/79551/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8754 -> Patchwork_18189
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/index.html

Known issues
------------

  Here are the changes found in Patchwork_18189 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic:
    - fi-tgl-y:           [PASS][1] -> [DMESG-WARN][2] ([i915#402])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-tgl-y/igt@gem_mmap@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-tgl-y/igt@gem_mmap@basic.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-byt-j1900/igt@i915_pm_rpm@basic-pci-d3-state.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-byt-j1900/igt@i915_pm_rpm@basic-pci-d3-state.html

  
#### Possible fixes ####

  * igt@gem_flink_basic@basic:
    - fi-tgl-y:           [DMESG-WARN][5] ([i915#402]) -> [PASS][6] +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-tgl-y/igt@gem_flink_basic@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-tgl-y/igt@gem_flink_basic@basic.html

  * igt@i915_module_load@reload:
    - fi-tgl-u2:          [DMESG-WARN][7] ([i915#402]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-tgl-u2/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-tgl-u2/igt@i915_module_load@reload.html

  * igt@i915_selftest@live@gem_contexts:
    - fi-tgl-u2:          [INCOMPLETE][9] ([i915#2045]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-tgl-u2/igt@i915_selftest@live@gem_contexts.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-tgl-u2/igt@i915_selftest@live@gem_contexts.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-bsw-n3050:       [DMESG-WARN][11] ([i915#1982]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
    - fi-bsw-kefka:       [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  
#### Warnings ####

  * igt@kms_pipe_crc_basic@read-crc-pipe-a:
    - fi-kbl-x1275:       [DMESG-WARN][15] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][16] ([i915#62] / [i915#92])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-kbl-x1275/igt@kms_pipe_crc_basic@read-crc-pipe-a.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-kbl-x1275/igt@kms_pipe_crc_basic@read-crc-pipe-a.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-x1275:       [DMESG-WARN][17] ([i915#62] / [i915#92]) -> [DMESG-WARN][18] ([i915#62] / [i915#92] / [i915#95]) +4 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/fi-kbl-x1275/igt@prime_vgem@basic-fence-flip.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/fi-kbl-x1275/igt@prime_vgem@basic-fence-flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2045]: https://gitlab.freedesktop.org/drm/intel/issues/2045
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (45 -> 40)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper 


Build changes
-------------

  * Linux: CI_DRM_8754 -> Patchwork_18189

  CI-20190529: 20190529
  CI_DRM_8754: 5e2a3a9c4ca7fe59db74a1fffe29e6a2012e2225 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5738: bc8b56fe177af34fbde7b96f1f66614a0014c6ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18189: 78c9cd63de6d4849d6270082f7f4f344ef1af272 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

78c9cd63de6d drm/i915/gt: Only transfer the virtual context to the new engine if active
101a6a825a8f drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs
70395ae3921e drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
fa1d73d35a4f drm/i915: Remove i915_request.lock requirement for execution callbacks
83628de655db drm/i915: Be wary of data races when reading the active execlists

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/index.html

[-- Attachment #1.2: Type: text/html, Size: 7075 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs

[Tvrtko Ursulin]

On 16/07/2020 12:33, Chris Wilson wrote:
> Since the breadcrumb enabling/cancelling itself is serialised by the
> breadcrumbs.irq_lock, with a bit of care we can remove the outer
> serialisation with i915_request.lock for concurrent
> dma_fence_enable_signaling(). This has the important side-effect of
> eliminating the nested i915_request.lock within request submission.
> 
> The challenge in serialisation is around the unsubmission where we take
> an active request that wants a breadcrumb on the signaling engine and
> put it to sleep. We do not want a concurrent
> dma_fence_enable_signaling() to attach a breadcrumb as we unsubmit, so
> we must mark the request as no longer active before serialising with the
> concurrent enable-signaling.
> 
> On retire, we serialise with the concurrent enable-signaling, but
> instead of clearing ACTIVE, we mark it as SIGNALED.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 130 +++++++++++++-------
>   drivers/gpu/drm/i915/gt/intel_lrc.c         |  14 ---
>   drivers/gpu/drm/i915/i915_request.c         |  39 +++---
>   3 files changed, 100 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 91786310c114..a0f52417238c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -220,17 +220,17 @@ static void signal_irq_work(struct irq_work *work)
>   	}
>   }
>   
> -static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
> +static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>   {
>   	struct intel_engine_cs *engine =
>   		container_of(b, struct intel_engine_cs, breadcrumbs);
>   
>   	lockdep_assert_held(&b->irq_lock);
>   	if (b->irq_armed)
> -		return true;
> +		return;
>   
>   	if (!intel_gt_pm_get_if_awake(engine->gt))
> -		return false;
> +		return;
>   
>   	/*
>   	 * The breadcrumb irq will be disarmed on the interrupt after the
> @@ -250,8 +250,6 @@ static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>   
>   	if (!b->irq_enabled++)
>   		irq_enable(engine);
> -
> -	return true;
>   }
>   
>   void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> @@ -310,57 +308,99 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>   {
>   }
>   
> -bool i915_request_enable_breadcrumb(struct i915_request *rq)
> +static void insert_breadcrumb(struct i915_request *rq,
> +			      struct intel_breadcrumbs *b)
>   {
> -	lockdep_assert_held(&rq->lock);
> +	struct intel_context *ce = rq->context;
> +	struct list_head *pos;
>   
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
> -		return true;
> +	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> +		return;
>   
> -	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags)) {
> -		struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
> -		struct intel_context *ce = rq->context;
> -		struct list_head *pos;
> +	__intel_breadcrumbs_arm_irq(b);
>   
> -		spin_lock(&b->irq_lock);
> +	/*
> +	 * We keep the seqno in retirement order, so we can break
> +	 * inside intel_engine_signal_breadcrumbs as soon as we've
> +	 * passed the last completed request (or seen a request that
> +	 * hasn't event started). We could walk the timeline->requests,
> +	 * but keeping a separate signalers_list has the advantage of
> +	 * hopefully being much smaller than the full list and so
> +	 * provides faster iteration and detection when there are no
> +	 * more interrupts required for this context.
> +	 *
> +	 * We typically expect to add new signalers in order, so we
> +	 * start looking for our insertion point from the tail of
> +	 * the list.
> +	 */
> +	list_for_each_prev(pos, &ce->signals) {
> +		struct i915_request *it =
> +			list_entry(pos, typeof(*it), signal_link);
>   
> -		if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> -			goto unlock;
> +		if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
> +			break;
> +	}
> +	list_add(&rq->signal_link, pos);
> +	if (pos == &ce->signals) /* catch transitions from empty list */
> +		list_move_tail(&ce->signal_link, &b->signalers);
> +	GEM_BUG_ON(!check_signal_order(ce, rq));
>   
> -		if (!__intel_breadcrumbs_arm_irq(b))
> -			goto unlock;
> +	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> +}
>   
> -		/*
> -		 * We keep the seqno in retirement order, so we can break
> -		 * inside intel_engine_signal_breadcrumbs as soon as we've
> -		 * passed the last completed request (or seen a request that
> -		 * hasn't event started). We could walk the timeline->requests,
> -		 * but keeping a separate signalers_list has the advantage of
> -		 * hopefully being much smaller than the full list and so
> -		 * provides faster iteration and detection when there are no
> -		 * more interrupts required for this context.
> -		 *
> -		 * We typically expect to add new signalers in order, so we
> -		 * start looking for our insertion point from the tail of
> -		 * the list.
> -		 */
> -		list_for_each_prev(pos, &ce->signals) {
> -			struct i915_request *it =
> -				list_entry(pos, typeof(*it), signal_link);
> +bool i915_request_enable_breadcrumb(struct i915_request *rq)
> +{
> +	struct intel_breadcrumbs *b;
>   
> -			if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
> -				break;
> -		}
> -		list_add(&rq->signal_link, pos);
> -		if (pos == &ce->signals) /* catch transitions from empty list */
> -			list_move_tail(&ce->signal_link, &b->signalers);
> -		GEM_BUG_ON(!check_signal_order(ce, rq));
> +	/* Seralises with i915_request_retire() using rq->lock */

Serialises

> +	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
> +		return true;
>   
> -		set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> -unlock:
> +	/*
> +	 * Peek at i915_request_submit()/i915_request_unsubmit() status.
> +	 *
> +	 * If the request is not yet active (and not signaled), we will
> +	 * attach the breadcrumb later.
> +	 */
> +	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> +		return true;
> +
> +	/*
> +	 * rq->engine is locked by rq->engine->active.lock. That however
> +	 * is not known until after rq->engine has been dereferenced and
> +	 * the lock acquired. Hence we acquire the lock and then validate
> +	 * that rq->engine still matches the lock we hold for it.
> +	 *
> +	 * Here, we are using the breadcrumb lock as a proxy for the
> +	 * rq->engine->active.lock, and we know that since the breadcrumb
> +	 * will be serialised within i915_request_submit/i915_request_unsubmit,
> +	 * the engine cannot change while active as long as we hold the
> +	 * breadcrumb lock on that engine.
> +	 *
> +	 * From the dma_fence_enable_signaling() path, we are outside of the
> +	 * request submit/unsubmit path, and so we must be more careful to
> +	 * acquire the right lock.
> +	 */
> +	b = &READ_ONCE(rq->engine)->breadcrumbs;
> +	spin_lock(&b->irq_lock);
> +	while (unlikely(b != &READ_ONCE(rq->engine)->breadcrumbs)) {
>   		spin_unlock(&b->irq_lock);
> +		b = &READ_ONCE(rq->engine)->breadcrumbs;
> +		spin_lock(&b->irq_lock);
>   	}
>   
> +	/*
> +	 * Now that we are finally serialised with request submit/unsubmit,
> +	 * [with b->irq_lock] and with i915_request_reitre() [via checking

retire

> +	 * SIGNALED with rq->lock] confirm the request is indeed active. If
> +	 * it is no longer active, the breadcrumb will be attached upon
> +	 * i915_request_submit().
> +	 */
> +	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> +		insert_breadcrumb(rq, b);
> +
> +	spin_unlock(&b->irq_lock);
> +
>   	return !__request_completed(rq);
>   }
>   
> @@ -368,8 +408,6 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
>   {
>   	struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
>   
> -	lockdep_assert_held(&rq->lock);
> -
>   	/*
>   	 * We must wait for b->irq_lock so that we know the interrupt handler
>   	 * has released its reference to the intel_context and has completed
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 29c0fde8b4df..21c16e31c4fe 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1148,20 +1148,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>   		} else {
>   			struct intel_engine_cs *owner = rq->context->engine;
>   
> -			/*
> -			 * Decouple the virtual breadcrumb before moving it
> -			 * back to the virtual engine -- we don't want the
> -			 * request to complete in the background and try
> -			 * and cancel the breadcrumb on the virtual engine
> -			 * (instead of the old engine where it is linked)!
> -			 */
> -			if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> -				     &rq->fence.flags)) {
> -				spin_lock_nested(&rq->lock,
> -						 SINGLE_DEPTH_NESTING);
> -				i915_request_cancel_breadcrumb(rq);
> -				spin_unlock(&rq->lock);
> -			}
>   			WRITE_ONCE(rq->engine, owner);
>   			owner->submit_request(rq);
>   			active = NULL;
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 2ef17b11ca4b..8c345ead04a6 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -320,11 +320,12 @@ bool i915_request_retire(struct i915_request *rq)
>   		dma_fence_signal_locked(&rq->fence);
>   	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
>   		i915_request_cancel_breadcrumb(rq);
> +	spin_unlock_irq(&rq->lock);
> +
>   	if (i915_request_has_waitboost(rq)) {
>   		GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
>   		atomic_dec(&rq->engine->gt->rps.num_waiters);
>   	}
> -	spin_unlock_irq(&rq->lock);
>   
>   	/*
>   	 * We only loosely track inflight requests across preemption,
> @@ -608,17 +609,9 @@ bool __i915_request_submit(struct i915_request *request)
>   	 */
>   	__notify_execute_cb_irq(request);
>   
> -	/* We may be recursing from the signal callback of another i915 fence */
> -	if (!i915_request_signaled(request)) {
> -		spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> -
> -		if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> -			     &request->fence.flags) &&
> -		    !i915_request_enable_breadcrumb(request))
> -			intel_engine_signal_breadcrumbs(engine);
> -
> -		spin_unlock(&request->lock);
> -	}
> +	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
> +	    !i915_request_enable_breadcrumb(request))
> +		intel_engine_signal_breadcrumbs(engine);
>   
>   	return result;
>   }
> @@ -640,27 +633,27 @@ void __i915_request_unsubmit(struct i915_request *request)
>   {
>   	struct intel_engine_cs *engine = request->engine;
>   
> +	/*
> +	 * Only unwind in reverse order, required so that the per-context list
> +	 * is kept in seqno/ring order.
> +	 */
>   	RQ_TRACE(request, "\n");
>   
>   	GEM_BUG_ON(!irqs_disabled());
>   	lockdep_assert_held(&engine->active.lock);
>   
>   	/*
> -	 * Only unwind in reverse order, required so that the per-context list
> -	 * is kept in seqno/ring order.
> +	 * Before we remove this breadcrumb from the signal list, we have
> +	 * to ensure that a concurrent dma_fence_enable_signaling() does not
> +	 * attach itself. We first mark the request as no longer active and
> +	 * make sure that is visible to other cores, and then remove the
> +	 * breadcrumb if attached.
>   	 */
> -
> -	/* We may be recursing from the signal callback of another i915 fence */
> -	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> -
> +	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
> +	clear_bit_unlock(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
>   	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
>   		i915_request_cancel_breadcrumb(request);
>   
> -	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
> -	clear_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
> -
> -	spin_unlock(&request->lock);
> -
>   	/* We've already spun, don't charge on resubmitting. */
>   	if (request->sched.semaphores && i915_request_started(request))
>   		request->sched.semaphores = 0;
> 

I did not find a hole (race) after quite a bit of straining the grey 
matter so.. lets see..

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs

[Tvrtko Ursulin]

On 16/07/2020 12:33, Chris Wilson wrote:
> Now that we have serialised the request retirement's decoupling of the
> breadcrumb from the engine with the request signaling, we know that
> there should never be a stale breadcrumb attached to an unbound virtual
> engine. We can now remove the fixup code that had to migrate the
> breadcrumbs along with the virtual engine, from one sibling to the next.

What do you mean by "unbound virtual engine"? Previous siblings[0]? We 
do know that has been completed, at the point the next one is getting 
dequeued, and by the virtue of breadcrumbs doing the signaling it will 
have been removed from the list. But that was true before. Which leaves 
me confused as to why the transfer was needed.. Was it just because 
explicit wait used to be a potential signaler and that's no longer the case?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c  | 29 --------------------
>   drivers/gpu/drm/i915/gt/intel_engine.h       |  3 --
>   drivers/gpu/drm/i915/gt/intel_engine_types.h |  2 --
>   drivers/gpu/drm/i915/gt/intel_lrc.c          | 15 ----------
>   4 files changed, 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index a0f52417238c..164662ae130b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -167,8 +167,6 @@ static void signal_irq_work(struct irq_work *work)
>   	if (b->irq_armed && list_empty(&b->signalers))
>   		__intel_breadcrumbs_disarm_irq(b);
>   
> -	list_splice_init(&b->signaled_requests, &signal);
> -
>   	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
>   		GEM_BUG_ON(list_empty(&ce->signals));
>   
> @@ -258,7 +256,6 @@ void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>   
>   	spin_lock_init(&b->irq_lock);
>   	INIT_LIST_HEAD(&b->signalers);
> -	INIT_LIST_HEAD(&b->signaled_requests);
>   
>   	init_irq_work(&b->irq_work, signal_irq_work);
>   }
> @@ -278,32 +275,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>   	spin_unlock_irqrestore(&b->irq_lock, flags);
>   }
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce)
> -{
> -	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&b->irq_lock, flags);
> -	if (!list_empty(&ce->signals)) {
> -		struct i915_request *rq, *next;
> -
> -		/* Queue for executing the signal callbacks in the irq_work */
> -		list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
> -			GEM_BUG_ON(rq->engine != engine);
> -			GEM_BUG_ON(!__request_completed(rq));
> -
> -			__signal_request(rq, &b->signaled_requests);
> -		}
> -
> -		INIT_LIST_HEAD(&ce->signals);
> -		list_del_init(&ce->signal_link);
> -
> -		irq_work_queue(&b->irq_work);
> -	}
> -	spin_unlock_irqrestore(&b->irq_lock, flags);
> -}
> -
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>   {
>   }
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index a9249a23903a..faf00a353e25 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
>   void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce);
> -
>   void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
>   				    struct drm_printer *p);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 8de92fd7d392..e0a2ceac729f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -393,8 +393,6 @@ struct intel_engine_cs {
>   		spinlock_t irq_lock;
>   		struct list_head signalers;
>   
> -		struct list_head signaled_requests;
> -
>   		struct irq_work irq_work; /* for use from inside irq_lock */
>   
>   		unsigned int irq_enabled;
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 21c16e31c4fe..88a5c155154d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
>   	return true;
>   }
>   
> -static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
> -{
> -	/*
> -	 * All the outstanding signals on ve->siblings[0] must have
> -	 * been completed, just pending the interrupt handler. As those
> -	 * signals still refer to the old sibling (via rq->engine), we must
> -	 * transfer those to the old irq_worker to keep our locking
> -	 * consistent.
> -	 */
> -	intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
> -}
> -
>   #define for_each_waiter(p__, rq__) \
>   	list_for_each_entry_lockless(p__, \
>   				     &(rq__)->sched.waiters_list, \
> @@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   					virtual_update_register_offsets(regs,
>   									engine);
>   
> -				if (!list_empty(&ve->context.signals))
> -					virtual_xfer_breadcrumbs(ve);
> -
>   				/*
>   				 * Move the bound engine to the top of the list
>   				 * for future execution. We then kick this
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915/gt: Only transfer the virtual context to the new engine if active

[Tvrtko Ursulin]

On 16/07/2020 12:33, Chris Wilson wrote:
> One more complication of preempt-to-busy with respect to the virtual
> engine is that we may have retired the last request along the virtual
> engine at the same time as preparing to submit the completed request to
> a new engine. That submit will be shortcircuited, but not before we have
> updated the context with the new register offsets and marked the virtual
> engine as bound to the new engine (by calling swap on ve->siblings[]).
> As we may have just retired the completed request, we may also be in the
> middle of calling intel_context_exit() to turn off the power management

virtual_context_exit

> associated with the virtual engine, and that in turn walks the
> ve->siblings[]. If we happen to call swap() on the array as we walk, we
> will call intel_engine_pm_put() twice on the same engine.
> 
> In this patch, we prevent this by only updating the bound engine after a
> successful submission which weeds out the already completed requests.
> 
> Alternatively, we could walk a non-volatile array for the pm, such as
> using the engine->mask. The small advantage to performing the update
> after the submit is that we then only have to do a swap for active
> requests.
> 
> Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: "Nayana, Venkata Ramana" <venkata.ramana.nayana@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 65 ++++++++++++++++++-----------
>   1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 88a5c155154d..5e8278e8ac79 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1805,6 +1805,33 @@ static bool virtual_matches(const struct virtual_engine *ve,
>   	return true;
>   }
>   
> +static void virtual_xfer_context(struct virtual_engine *ve,
> +				 struct intel_engine_cs *engine)
> +{
> +	unsigned int n;
> +
> +	if (likely(engine == ve->siblings[0]))
> +		return;
> +
> +	GEM_BUG_ON(READ_ONCE(ve->context.inflight));
> +	if (!intel_engine_has_relative_mmio(engine))
> +		virtual_update_register_offsets(ve->context.lrc_reg_state,
> +						engine);
> +
> +	/*
> +	 * Move the bound engine to the top of the list for
> +	 * future execution. We then kick this tasklet first
> +	 * before checking others, so that we preferentially
> +	 * reuse this set of bound registers.
> +	 */
> +	for (n = 1; n < ve->num_siblings; n++) {
> +		if (ve->siblings[n] == engine) {
> +			swap(ve->siblings[n], ve->siblings[0]);
> +			break;
> +		}
> +	}
> +}
> +
>   #define for_each_waiter(p__, rq__) \
>   	list_for_each_entry_lockless(p__, \
>   				     &(rq__)->sched.waiters_list, \
> @@ -2253,35 +2280,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   			GEM_BUG_ON(!(rq->execution_mask & engine->mask));
>   			WRITE_ONCE(rq->engine, engine);
>   
> -			if (engine != ve->siblings[0]) {
> -				u32 *regs = ve->context.lrc_reg_state;
> -				unsigned int n;
> -
> -				GEM_BUG_ON(READ_ONCE(ve->context.inflight));
> -
> -				if (!intel_engine_has_relative_mmio(engine))
> -					virtual_update_register_offsets(regs,
> -									engine);
> -
> +			if (__i915_request_submit(rq)) {
>   				/*
> -				 * Move the bound engine to the top of the list
> -				 * for future execution. We then kick this
> -				 * tasklet first before checking others, so that
> -				 * we preferentially reuse this set of bound
> -				 * registers.
> +				 * Only after we confirm that we will submit
> +				 * this request (i.e. it has not already
> +				 * completed), do we want to update the context.
> +				 *
> +				 * This serves two purposes. It avoids
> +				 * unnecessary work if we are resubmitting an
> +				 * already completed request after timeslicing.
> +				 * But more importantly, it prevents us altering
> +				 * ve->siblings[] on an idle context, where
> +				 * we may be using ve->siblings[] in
> +				 * virtual_context_enter / virtual_context_exit.
>   				 */
> -				for (n = 1; n < ve->num_siblings; n++) {
> -					if (ve->siblings[n] == engine) {
> -						swap(ve->siblings[n],
> -						     ve->siblings[0]);
> -						break;
> -					}
> -				}
> -
> +				virtual_xfer_context(ve, engine);
>   				GEM_BUG_ON(ve->siblings[0] != engine);
> -			}
>   
> -			if (__i915_request_submit(rq)) {
>   				submit = true;
>   				last = rq;
>   			}
> 

This was nasty indeed. How did you manage to find this?

I think it is okay to do the update once we know submit is doing ahead, 
but in the light of this "gotcha" I think it would also be good to start 
using for_each_engine_masked in virtual_context_enter/exit.

Regardless:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/i915/gt: Drop intel_engine_transfer_stale_breadcrumbs

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-07-16 16:29:37)
> 
> On 16/07/2020 12:33, Chris Wilson wrote:
> > Now that we have serialised the request retirement's decoupling of the
> > breadcrumb from the engine with the request signaling, we know that
> > there should never be a stale breadcrumb attached to an unbound virtual
> > engine. We can now remove the fixup code that had to migrate the
> > breadcrumbs along with the virtual engine, from one sibling to the next.
> 
> What do you mean by "unbound virtual engine"?

I think of ve->context.inflight == NULL as being unbound.

> Previous siblings[0]? We 
> do know that has been completed, at the point the next one is getting 
> dequeued, and by the virtue of breadcrumbs doing the signaling it will 
> have been removed from the list. But that was true before. Which leaves 
> me confused as to why the transfer was needed.. Was it just because 
> explicit wait used to be a potential signaler and that's no longer the case?

Evidently we did get requests finding their way onto
ve->engine[0].breadcumbs after the unsubmit. I thought I had a good
explanation with a window between ACTIVE and SIGNALED, but going back to
tip, those transitions are all underneath the rq->lock.

However, if we submit a completed request, that is put onto the
rq->engine->breadcrumb, but we do not schedule-in the context. That
would cause us to have breadcrumbs on ve->engine[0] while
ve->context.flight was NULL and on the next virtual request submission
could switch to a new engine with stale requests.

Ok, the issue of stale breadcrumbs is not completely solved yet.
But this time, this time for sure!, I think I know the cause of the
stale breadcrumbs!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915/gt: Only transfer the virtual context to the new engine if active

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-07-16 16:37:25)
> 
> On 16/07/2020 12:33, Chris Wilson wrote:
> > One more complication of preempt-to-busy with respect to the virtual
> > engine is that we may have retired the last request along the virtual
> > engine at the same time as preparing to submit the completed request to
> > a new engine. That submit will be shortcircuited, but not before we have
> > updated the context with the new register offsets and marked the virtual
> > engine as bound to the new engine (by calling swap on ve->siblings[]).
> > As we may have just retired the completed request, we may also be in the
> > middle of calling intel_context_exit() to turn off the power management
> 
> virtual_context_exit
> 
> > associated with the virtual engine, and that in turn walks the
> > ve->siblings[]. If we happen to call swap() on the array as we walk, we
> > will call intel_engine_pm_put() twice on the same engine.
> > 
> > In this patch, we prevent this by only updating the bound engine after a
> > successful submission which weeds out the already completed requests.
> > 
> > Alternatively, we could walk a non-volatile array for the pm, such as
> > using the engine->mask. The small advantage to performing the update
> > after the submit is that we then only have to do a swap for active
> > requests.
> > 
> > Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> > References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: "Nayana, Venkata Ramana" <venkata.ramana.nayana@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 65 ++++++++++++++++++-----------
> >   1 file changed, 40 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 88a5c155154d..5e8278e8ac79 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1805,6 +1805,33 @@ static bool virtual_matches(const struct virtual_engine *ve,
> >       return true;
> >   }
> >   
> > +static void virtual_xfer_context(struct virtual_engine *ve,
> > +                              struct intel_engine_cs *engine)
> > +{
> > +     unsigned int n;
> > +
> > +     if (likely(engine == ve->siblings[0]))
> > +             return;
> > +
> > +     GEM_BUG_ON(READ_ONCE(ve->context.inflight));
> > +     if (!intel_engine_has_relative_mmio(engine))
> > +             virtual_update_register_offsets(ve->context.lrc_reg_state,
> > +                                             engine);
> > +
> > +     /*
> > +      * Move the bound engine to the top of the list for
> > +      * future execution. We then kick this tasklet first
> > +      * before checking others, so that we preferentially
> > +      * reuse this set of bound registers.
> > +      */
> > +     for (n = 1; n < ve->num_siblings; n++) {
> > +             if (ve->siblings[n] == engine) {
> > +                     swap(ve->siblings[n], ve->siblings[0]);
> > +                     break;
> > +             }
> > +     }
> > +}
> > +
> >   #define for_each_waiter(p__, rq__) \
> >       list_for_each_entry_lockless(p__, \
> >                                    &(rq__)->sched.waiters_list, \
> > @@ -2253,35 +2280,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                       GEM_BUG_ON(!(rq->execution_mask & engine->mask));
> >                       WRITE_ONCE(rq->engine, engine);
> >   
> > -                     if (engine != ve->siblings[0]) {
> > -                             u32 *regs = ve->context.lrc_reg_state;
> > -                             unsigned int n;
> > -
> > -                             GEM_BUG_ON(READ_ONCE(ve->context.inflight));
> > -
> > -                             if (!intel_engine_has_relative_mmio(engine))
> > -                                     virtual_update_register_offsets(regs,
> > -                                                                     engine);
> > -
> > +                     if (__i915_request_submit(rq)) {
> >                               /*
> > -                              * Move the bound engine to the top of the list
> > -                              * for future execution. We then kick this
> > -                              * tasklet first before checking others, so that
> > -                              * we preferentially reuse this set of bound
> > -                              * registers.
> > +                              * Only after we confirm that we will submit
> > +                              * this request (i.e. it has not already
> > +                              * completed), do we want to update the context.
> > +                              *
> > +                              * This serves two purposes. It avoids
> > +                              * unnecessary work if we are resubmitting an
> > +                              * already completed request after timeslicing.
> > +                              * But more importantly, it prevents us altering
> > +                              * ve->siblings[] on an idle context, where
> > +                              * we may be using ve->siblings[] in
> > +                              * virtual_context_enter / virtual_context_exit.
> >                                */
> > -                             for (n = 1; n < ve->num_siblings; n++) {
> > -                                     if (ve->siblings[n] == engine) {
> > -                                             swap(ve->siblings[n],
> > -                                                  ve->siblings[0]);
> > -                                             break;
> > -                                     }
> > -                             }
> > -
> > +                             virtual_xfer_context(ve, engine);
> >                               GEM_BUG_ON(ve->siblings[0] != engine);
> > -                     }
> >   
> > -                     if (__i915_request_submit(rq)) {
> >                               submit = true;
> >                               last = rq;
> >                       }
> > 
> 
> This was nasty indeed. How did you manage to find this?

Staring the traces, playing games of what-if. The clue was a bunch of
retirement and parking (too early due to the bad wakeref count) around
the very occasional bouncing virtual engine.

> I think it is okay to do the update once we know submit is doing ahead, 
> but in the light of this "gotcha" I think it would also be good to start 
> using for_each_engine_masked in virtual_context_enter/exit.

I am tempted, but I want to avoid further reliance on ve->base.gt. That
may be a moot point already, but I hope not.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs

[Chris Wilson]

After staring at the breadcrumb enabling/cancellation and coming to the
conclusion that the cause of the mysterious stale breadcrumbs must the
act of submitting a completed requests, we can then redirect those
completed requests onto a dedicated signaled_list at the time of
construction and so eliminate intel_engine_transfer_stale_breadcrumbs().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 44 +++++++--------------
 drivers/gpu/drm/i915/gt/intel_engine.h      |  3 --
 drivers/gpu/drm/i915/gt/intel_lrc.c         | 15 -------
 drivers/gpu/drm/i915/i915_request.c         |  5 +--
 4 files changed, 17 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index a0f52417238c..11660bef1545 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -144,7 +144,6 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
 
 static void __signal_request(struct i915_request *rq, struct list_head *signals)
 {
-	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
 	clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 
 	if (!__dma_fence_signal(&rq->fence))
@@ -278,32 +277,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&b->irq_lock, flags);
 }
 
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
-					     struct intel_context *ce)
-{
-	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	unsigned long flags;
-
-	spin_lock_irqsave(&b->irq_lock, flags);
-	if (!list_empty(&ce->signals)) {
-		struct i915_request *rq, *next;
-
-		/* Queue for executing the signal callbacks in the irq_work */
-		list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
-			GEM_BUG_ON(rq->engine != engine);
-			GEM_BUG_ON(!__request_completed(rq));
-
-			__signal_request(rq, &b->signaled_requests);
-		}
-
-		INIT_LIST_HEAD(&ce->signals);
-		list_del_init(&ce->signal_link);
-
-		irq_work_queue(&b->irq_work);
-	}
-	spin_unlock_irqrestore(&b->irq_lock, flags);
-}
-
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 {
 }
@@ -317,6 +290,17 @@ static void insert_breadcrumb(struct i915_request *rq,
 	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
 		return;
 
+	/*
+	 * If the request is already completed, we can transfer it
+	 * straight onto a signaled list, and queue the irq worker for
+	 * its signal completion.
+	 */
+	if (__request_completed(rq)) {
+		__signal_request(rq, &b->signaled_requests);
+		irq_work_queue(&b->irq_work);
+		return;
+	}
+
 	__intel_breadcrumbs_arm_irq(b);
 
 	/*
@@ -341,8 +325,10 @@ static void insert_breadcrumb(struct i915_request *rq,
 			break;
 	}
 	list_add(&rq->signal_link, pos);
-	if (pos == &ce->signals) /* catch transitions from empty list */
+	if (pos == &ce->signals) { /* catch transitions from empty list */
 		list_move_tail(&ce->signal_link, &b->signalers);
+		irq_work_queue(&b->irq_work); /* check after enabling irq */
+	}
 	GEM_BUG_ON(!check_signal_order(ce, rq));
 
 	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
@@ -401,7 +387,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
 
 	spin_unlock(&b->irq_lock);
 
-	return !__request_completed(rq);
+	return true;
 }
 
 void i915_request_cancel_breadcrumb(struct i915_request *rq)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index a9249a23903a..faf00a353e25 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
 void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
 
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
-					     struct intel_context *ce);
-
 void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
 				    struct drm_printer *p);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 21c16e31c4fe..88a5c155154d 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
 	return true;
 }
 
-static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
-{
-	/*
-	 * All the outstanding signals on ve->siblings[0] must have
-	 * been completed, just pending the interrupt handler. As those
-	 * signals still refer to the old sibling (via rq->engine), we must
-	 * transfer those to the old irq_worker to keep our locking
-	 * consistent.
-	 */
-	intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
-}
-
 #define for_each_waiter(p__, rq__) \
 	list_for_each_entry_lockless(p__, \
 				     &(rq__)->sched.waiters_list, \
@@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 					virtual_update_register_offsets(regs,
 									engine);
 
-				if (!list_empty(&ve->context.signals))
-					virtual_xfer_breadcrumbs(ve);
-
 				/*
 				 * Move the bound engine to the top of the list
 				 * for future execution. We then kick this
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index fc25382e1201..d88f046ccbdd 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -609,9 +609,8 @@ bool __i915_request_submit(struct i915_request *request)
 	 */
 	__notify_execute_cb_irq(request);
 
-	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
-	    !i915_request_enable_breadcrumb(request))
-		intel_engine_signal_breadcrumbs(engine);
+	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
+		i915_request_enable_breadcrumb(request);
 
 	return result;
 }
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 18549 bytes --]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists
URL   : https://patchwork.freedesktop.org/series/79551/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8754_full -> Patchwork_18189_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18189_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18189_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18189_full:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_backlight@fade_with_suspend:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] +3 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl5/igt@i915_pm_backlight@fade_with_suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl2/igt@i915_pm_backlight@fade_with_suspend.html

  
Known issues
------------

  Here are the changes found in Patchwork_18189_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_reloc@basic-concurrent0:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#1930])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk7/igt@gem_exec_reloc@basic-concurrent0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk6/igt@gem_exec_reloc@basic-concurrent0.html

  * igt@gem_exec_whisper@basic-queues-all:
    - shard-glk:          [PASS][5] -> [DMESG-WARN][6] ([i915#118] / [i915#95])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk1/igt@gem_exec_whisper@basic-queues-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk7/igt@gem_exec_whisper@basic-queues-all.html

  * igt@kms_big_fb@y-tiled-64bpp-rotate-180:
    - shard-glk:          [PASS][7] -> [DMESG-FAIL][8] ([i915#118] / [i915#95])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk4/igt@kms_big_fb@y-tiled-64bpp-rotate-180.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk8/igt@kms_big_fb@y-tiled-64bpp-rotate-180.html

  * igt@kms_big_fb@yf-tiled-32bpp-rotate-270:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([i915#1635] / [i915#1982])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-apl8/igt@kms_big_fb@yf-tiled-32bpp-rotate-270.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-apl1/igt@kms_big_fb@yf-tiled-32bpp-rotate-270.html

  * igt@kms_color@pipe-b-ctm-negative:
    - shard-skl:          [PASS][11] -> [DMESG-WARN][12] ([i915#1982]) +14 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl6/igt@kms_color@pipe-b-ctm-negative.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl5/igt@kms_color@pipe-b-ctm-negative.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [PASS][13] -> [DMESG-WARN][14] ([i915#180]) +7 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([i915#177] / [i915#52] / [i915#54])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl3/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl2/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html

  * igt@kms_flip@flip-vs-expired-vblank@a-edp1:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([i915#79]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl2/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl9/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html

  * igt@kms_flip@plain-flip-fb-recreate@c-edp1:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([i915#2122])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl4/igt@kms_flip@plain-flip-fb-recreate@c-edp1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl4/igt@kms_flip@plain-flip-fb-recreate@c-edp1.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          [PASS][21] -> [DMESG-WARN][22] ([i915#180] / [i915#1982])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt:
    - shard-tglb:         [PASS][23] -> [SKIP][24] ([i915#668]) +7 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt.html

  * igt@kms_hdr@bpc-switch-dpms:
    - shard-skl:          [PASS][25] -> [FAIL][26] ([i915#1188])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl3/igt@kms_hdr@bpc-switch-dpms.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl2/igt@kms_hdr@bpc-switch-dpms.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [PASS][27] -> [FAIL][28] ([fdo#108145] / [i915#265])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl5/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [PASS][29] -> [SKIP][30] ([fdo#109441])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-iclb2/igt@kms_psr@psr2_dpms.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-iclb1/igt@kms_psr@psr2_dpms.html

  
#### Possible fixes ####

  * igt@gem_ctx_persistence@engines-mixed-process@vecs0:
    - shard-glk:          [FAIL][31] ([i915#1528]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk5/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk7/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html

  * igt@gem_exec_parallel@fds@rcs0:
    - shard-iclb:         [INCOMPLETE][33] -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-iclb6/igt@gem_exec_parallel@fds@rcs0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-iclb4/igt@gem_exec_parallel@fds@rcs0.html

  * igt@gem_exec_whisper@basic-contexts-forked-all:
    - shard-glk:          [DMESG-WARN][35] ([i915#118] / [i915#95]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk4/igt@gem_exec_whisper@basic-contexts-forked-all.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk9/igt@gem_exec_whisper@basic-contexts-forked-all.html

  * igt@i915_selftest@mock@requests:
    - shard-iclb:         [INCOMPLETE][37] ([i915#2110]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-iclb2/igt@i915_selftest@mock@requests.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-iclb7/igt@i915_selftest@mock@requests.html

  * igt@kms_color@pipe-a-ctm-0-75:
    - shard-skl:          [DMESG-WARN][39] ([i915#1982]) -> [PASS][40] +4 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl4/igt@kms_color@pipe-a-ctm-0-75.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl8/igt@kms_color@pipe-a-ctm-0-75.html

  * igt@kms_cursor_edge_walk@pipe-a-64x64-left-edge:
    - shard-glk:          [DMESG-FAIL][41] ([i915#118] / [i915#70] / [i915#95]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-glk1/igt@kms_cursor_edge_walk@pipe-a-64x64-left-edge.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-glk4/igt@kms_cursor_edge_walk@pipe-a-64x64-left-edge.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-snb:          [TIMEOUT][43] ([i915#1958] / [i915#2119]) -> [PASS][44] +2 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-snb1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-snb1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_flip@flip-vs-suspend@c-dp1:
    - shard-kbl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +5 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-kbl3/igt@kms_flip@flip-vs-suspend@c-dp1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-kbl3/igt@kms_flip@flip-vs-suspend@c-dp1.html

  * igt@kms_flip@plain-flip-ts-check-interruptible@c-edp1:
    - shard-skl:          [FAIL][47] ([i915#2122]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl10/igt@kms_flip@plain-flip-ts-check-interruptible@c-edp1.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl3/igt@kms_flip@plain-flip-ts-check-interruptible@c-edp1.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-tglb:         [DMESG-WARN][49] ([i915#1982]) -> [PASS][50] +1 similar issue
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb7/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-tglb2/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][51] ([fdo#108145] / [i915#265]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [SKIP][53] ([fdo#109441]) -> [PASS][54] +2 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-iclb4/igt@kms_psr@psr2_primary_page_flip.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_vblank@pipe-a-query-forked-busy:
    - shard-tglb:         [DMESG-WARN][55] ([i915#402]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb7/igt@kms_vblank@pipe-a-query-forked-busy.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-tglb7/igt@kms_vblank@pipe-a-query-forked-busy.html

  * igt@kms_vblank@pipe-c-wait-busy:
    - shard-kbl:          [DMESG-WARN][57] ([i915#1982]) -> [PASS][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-kbl7/igt@kms_vblank@pipe-c-wait-busy.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-kbl7/igt@kms_vblank@pipe-c-wait-busy.html

  * igt@perf_pmu@busy-idle@rcs0:
    - shard-snb:          [FAIL][59] -> [PASS][60] +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-snb1/igt@perf_pmu@busy-idle@rcs0.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-snb1/igt@perf_pmu@busy-idle@rcs0.html

  * igt@perf_pmu@busy-idle@vcs0:
    - shard-snb:          [INCOMPLETE][61] ([i915#2119] / [i915#82]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-snb1/igt@perf_pmu@busy-idle@vcs0.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-snb1/igt@perf_pmu@busy-idle@vcs0.html

  * igt@perf_pmu@semaphore-busy@rcs0:
    - shard-kbl:          [FAIL][63] ([i915#1820]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-kbl7/igt@perf_pmu@semaphore-busy@rcs0.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-kbl1/igt@perf_pmu@semaphore-busy@rcs0.html

  
#### Warnings ####

  * igt@gem_exec_reloc@basic-concurrent16:
    - shard-snb:          [TIMEOUT][65] ([i915#1958] / [i915#2119]) -> [FAIL][66] ([i915#1930])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-snb1/igt@gem_exec_reloc@basic-concurrent16.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-snb1/igt@gem_exec_reloc@basic-concurrent16.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         [WARN][67] ([i915#1515]) -> [FAIL][68] ([i915#1515])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-iclb7/igt@i915_pm_rc6_residency@rc6-idle.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-iclb7/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@kms_ccs@pipe-c-ccs-on-another-bo:
    - shard-snb:          [TIMEOUT][69] ([i915#1958] / [i915#2119]) -> [SKIP][70] ([fdo#109271]) +1 similar issue
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-snb1/igt@kms_ccs@pipe-c-ccs-on-another-bo.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-snb1/igt@kms_ccs@pipe-c-ccs-on-another-bo.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          [FAIL][71] ([fdo#108145] / [i915#265]) -> [DMESG-FAIL][72] ([fdo#108145] / [i915#1982])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html
    - shard-apl:          [DMESG-FAIL][73] ([fdo#108145] / [i915#1635] / [i915#1982]) -> [FAIL][74] ([fdo#108145] / [i915#1635] / [i915#265])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-apl7/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-apl8/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@runner@aborted:
    - shard-apl:          ([FAIL][75], [FAIL][76]) ([i915#1610] / [i915#1635] / [i915#2110]) -> [FAIL][77] ([i915#1635] / [i915#2110])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-apl3/igt@runner@aborted.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-apl7/igt@runner@aborted.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-apl6/igt@runner@aborted.html
    - shard-tglb:         ([FAIL][78], [FAIL][79], [FAIL][80]) ([i915#1764] / [i915#2110]) -> [FAIL][81] ([i915#2110])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb3/igt@runner@aborted.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb7/igt@runner@aborted.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8754/shard-tglb2/igt@runner@aborted.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/shard-tglb5/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1515]: https://gitlab.freedesktop.org/drm/intel/issues/1515
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1610]: https://gitlab.freedesktop.org/drm/intel/issues/1610
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#1764]: https://gitlab.freedesktop.org/drm/intel/issues/1764
  [i915#177]: https://gitlab.freedesktop.org/drm/intel/issues/177
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1820]: https://gitlab.freedesktop.org/drm/intel/issues/1820
  [i915#1930]: https://gitlab.freedesktop.org/drm/intel/issues/1930
  [i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2110]: https://gitlab.freedesktop.org/drm/intel/issues/2110
  [i915#2119]: https://gitlab.freedesktop.org/drm/intel/issues/2119
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
  [i915#70]: https://gitlab.freedesktop.org/drm/intel/issues/70
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_8754 -> Patchwork_18189

  CI-20190529: 20190529
  CI_DRM_8754: 5e2a3a9c4ca7fe59db74a1fffe29e6a2012e2225 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5738: bc8b56fe177af34fbde7b96f1f66614a0014c6ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18189: 78c9cd63de6d4849d6270082f7f4f344ef1af272 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18189/index.html

[-- Attachment #1.2: Type: text/html, Size: 22677 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)
URL   : https://patchwork.freedesktop.org/series/79551/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
848b1076da85 drm/i915: Be wary of data races when reading the active execlists
-:92: WARNING:TYPO_SPELLING: 'overwritting' may be misspelled - perhaps 'overwriting'?
#92: FILE: drivers/gpu/drm/i915/i915_request.c:409:
+	 * of execlists->pending[] to execlists->inflight[], overwritting

total: 0 errors, 1 warnings, 0 checks, 67 lines checked
4f7fe277264c drm/i915: Remove i915_request.lock requirement for execution callbacks
3ef9648cd846 drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
5e545c7291af drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
cd44d243792a drm/i915/gt: Only transfer the virtual context to the new engine if active
-:28: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#28: 
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"

-:28: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 6d06779e8672 ("drm/i915: Load balancing across a virtual engine")'
#28: 
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"

total: 1 errors, 1 warnings, 0 checks, 81 lines checked


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)
URL   : https://patchwork.freedesktop.org/series/79551/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 7416 bytes --]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)
URL   : https://patchwork.freedesktop.org/series/79551/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8757 -> Patchwork_18193
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/index.html

Known issues
------------

  Here are the changes found in Patchwork_18193 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-tgl-y:           [PASS][1] -> [DMESG-WARN][2] ([i915#402]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-y/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-y/igt@i915_module_load@reload.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1:
    - fi-icl-u2:          [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-y/igt@kms_frontbuffer_tracking@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-y/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - fi-tgl-u2:          [PASS][7] -> [DMESG-WARN][8] ([i915#402])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-tgl-u2:          [FAIL][9] ([i915#1888]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html

  * igt@i915_module_load@reload:
    - fi-tgl-u2:          [DMESG-WARN][11] ([i915#402]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-u2/igt@i915_module_load@reload.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-u2/igt@i915_module_load@reload.html
    - fi-icl-y:           [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-icl-y/igt@i915_module_load@reload.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-icl-y/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       [DMESG-WARN][15] ([i915#1982]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@module-reload:
    - fi-cml-s:           [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-cml-s/igt@i915_pm_rpm@module-reload.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-cml-s/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-tgl-y:           [DMESG-WARN][19] ([i915#1982]) -> [PASS][20] +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
    - fi-icl-u2:          [DMESG-WARN][21] ([i915#1982]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html

  * igt@prime_self_import@basic-with_two_bos:
    - fi-tgl-y:           [DMESG-WARN][23] ([i915#402]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html

  
#### Warnings ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-kbl-x1275:       [DMESG-WARN][25] ([i915#62] / [i915#92]) -> [DMESG-WARN][26] ([i915#62] / [i915#92] / [i915#95]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-kbl-x1275:       [DMESG-WARN][27] ([i915#1982] / [i915#62] / [i915#92]) -> [DMESG-WARN][28] ([i915#62] / [i915#92])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (45 -> 40)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper 


Build changes
-------------

  * Linux: CI_DRM_8757 -> Patchwork_18193

  CI-20190529: 20190529
  CI_DRM_8757: 6802049b80a49f5f45c2bc2dd3e6d189204dc2bb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5738: bc8b56fe177af34fbde7b96f1f66614a0014c6ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18193: cd44d243792a10e88f0e66df51a82ba47bb4a998 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

cd44d243792a drm/i915/gt: Only transfer the virtual context to the new engine if active
5e545c7291af drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
3ef9648cd846 drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
4f7fe277264c drm/i915: Remove i915_request.lock requirement for execution callbacks
848b1076da85 drm/i915: Be wary of data races when reading the active execlists

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/index.html

[-- Attachment #1.2: Type: text/html, Size: 9452 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 13957 bytes --]

== Series Details ==

Series: series starting with [1/5] drm/i915: Be wary of data races when reading the active execlists (rev2)
URL   : https://patchwork.freedesktop.org/series/79551/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8757_full -> Patchwork_18193_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18193_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18193_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18193_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl9/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl3/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html

  
Known issues
------------

  Here are the changes found in Patchwork_18193_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@kms:
    - shard-skl:          [PASS][3] -> [DMESG-WARN][4] ([i915#1982]) +12 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl1/igt@gem_eio@kms.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl5/igt@gem_eio@kms.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][5] -> [FAIL][6] ([i915#79]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1:
    - shard-apl:          [PASS][7] -> [FAIL][8] ([i915#1635] / [i915#79])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-apl2/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-apl1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1.html

  * igt@kms_flip@flip-vs-suspend@b-dp1:
    - shard-kbl:          [PASS][9] -> [DMESG-WARN][10] ([i915#180]) +5 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-kbl7/igt@kms_flip@flip-vs-suspend@b-dp1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-kbl4/igt@kms_flip@flip-vs-suspend@b-dp1.html

  * igt@kms_frontbuffer_tracking@psr-slowdraw:
    - shard-tglb:         [PASS][11] -> [DMESG-WARN][12] ([i915#1982]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-tglb8/igt@kms_frontbuffer_tracking@psr-slowdraw.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-tglb3/igt@kms_frontbuffer_tracking@psr-slowdraw.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([i915#53])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl3/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl4/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145] / [i915#265]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#109642] / [fdo#111068])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-iclb4/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_primary_blt:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-iclb4/igt@kms_psr@psr2_primary_blt.html

  * igt@perf_pmu@semaphore-busy@rcs0:
    - shard-kbl:          [PASS][21] -> [FAIL][22] ([i915#1820])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-kbl7/igt@perf_pmu@semaphore-busy@rcs0.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-kbl3/igt@perf_pmu@semaphore-busy@rcs0.html

  
#### Possible fixes ####

  * igt@gem_ctx_persistence@processes:
    - shard-skl:          [FAIL][23] ([i915#1528]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl1/igt@gem_ctx_persistence@processes.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl4/igt@gem_ctx_persistence@processes.html

  * igt@gem_userptr_blits@invalid-mmap-offset-unsync@gtt:
    - shard-tglb:         [INCOMPLETE][25] ([i915#2119] / [i915#2149]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-tglb7/igt@gem_userptr_blits@invalid-mmap-offset-unsync@gtt.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-tglb6/igt@gem_userptr_blits@invalid-mmap-offset-unsync@gtt.html

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-skl:          [INCOMPLETE][27] -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl10/igt@gem_workarounds@suspend-resume-fd.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl1/igt@gem_workarounds@suspend-resume-fd.html

  * igt@i915_module_load@reload:
    - shard-tglb:         [DMESG-WARN][29] ([i915#402]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-tglb6/igt@i915_module_load@reload.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-tglb3/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@i2c:
    - shard-skl:          [DMESG-WARN][31] ([i915#1982]) -> [PASS][32] +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl6/igt@i915_pm_rpm@i2c.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl10/igt@i915_pm_rpm@i2c.html

  * igt@i915_selftest@mock@requests:
    - shard-apl:          [INCOMPLETE][33] ([i915#1635] / [i915#2110]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-apl1/igt@i915_selftest@mock@requests.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-apl4/igt@i915_selftest@mock@requests.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen:
    - shard-skl:          [FAIL][35] ([i915#54]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl5/igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen.html

  * igt@kms_flip@flip-vs-expired-vblank@a-edp1:
    - shard-skl:          [FAIL][37] ([i915#79]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl8/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl6/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@a-dp1:
    - shard-kbl:          [DMESG-WARN][39] ([i915#180]) -> [PASS][40] +7 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-kbl6/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-kbl1/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@c-edp1:
    - shard-skl:          [INCOMPLETE][41] ([i915#198]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl9/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl3/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-tglb:         [DMESG-WARN][43] ([i915#1982]) -> [PASS][44] +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-tglb2/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-pwrite.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-tglb1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-pwrite.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [FAIL][45] ([fdo#108145] / [i915#265]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_psr@psr2_primary_render:
    - shard-iclb:         [SKIP][47] ([fdo#109441]) -> [PASS][48] +1 similar issue
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-iclb7/igt@kms_psr@psr2_primary_render.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-iclb2/igt@kms_psr@psr2_primary_render.html

  * igt@kms_vblank@pipe-c-wait-busy-hang:
    - shard-apl:          [DMESG-WARN][49] ([i915#1635] / [i915#1982]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-apl1/igt@kms_vblank@pipe-c-wait-busy-hang.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-apl1/igt@kms_vblank@pipe-c-wait-busy-hang.html

  
#### Warnings ####

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         [WARN][51] ([i915#1515]) -> [FAIL][52] ([i915#1515])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-iclb4/igt@i915_pm_rc6_residency@rc6-idle.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-iclb8/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
    - shard-skl:          [FAIL][53] ([i915#79]) -> [FAIL][54] ([i915#2122])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-apl:          [DMESG-FAIL][55] ([fdo#108145] / [i915#1635] / [i915#1982]) -> [FAIL][56] ([fdo#108145] / [i915#1635] / [i915#265])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-apl6/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-apl8/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@runner@aborted:
    - shard-tglb:         [FAIL][57] ([i915#2110]) -> [FAIL][58] ([i915#1764] / [i915#2110])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8757/shard-tglb8/igt@runner@aborted.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/shard-tglb3/igt@runner@aborted.html

  
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#1515]: https://gitlab.freedesktop.org/drm/intel/issues/1515
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#1764]: https://gitlab.freedesktop.org/drm/intel/issues/1764
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1820]: https://gitlab.freedesktop.org/drm/intel/issues/1820
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2110]: https://gitlab.freedesktop.org/drm/intel/issues/2110
  [i915#2119]: https://gitlab.freedesktop.org/drm/intel/issues/2119
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2149]: https://gitlab.freedesktop.org/drm/intel/issues/2149
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#53]: https://gitlab.freedesktop.org/drm/intel/issues/53
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_8757 -> Patchwork_18193

  CI-20190529: 20190529
  CI_DRM_8757: 6802049b80a49f5f45c2bc2dd3e6d189204dc2bb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5738: bc8b56fe177af34fbde7b96f1f66614a0014c6ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18193: cd44d243792a10e88f0e66df51a82ba47bb4a998 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18193/index.html

[-- Attachment #1.2: Type: text/html, Size: 16727 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs

[Tvrtko Ursulin]

On 16/07/2020 18:28, Chris Wilson wrote:
> After staring at the breadcrumb enabling/cancellation and coming to the
> conclusion that the cause of the mysterious stale breadcrumbs must the
> act of submitting a completed requests, we can then redirect those
> completed requests onto a dedicated signaled_list at the time of
> construction and so eliminate intel_engine_transfer_stale_breadcrumbs().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 44 +++++++--------------
>   drivers/gpu/drm/i915/gt/intel_engine.h      |  3 --
>   drivers/gpu/drm/i915/gt/intel_lrc.c         | 15 -------
>   drivers/gpu/drm/i915/i915_request.c         |  5 +--
>   4 files changed, 17 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index a0f52417238c..11660bef1545 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -144,7 +144,6 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
>   
>   static void __signal_request(struct i915_request *rq, struct list_head *signals)
>   {
> -	GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
>   	clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>   
>   	if (!__dma_fence_signal(&rq->fence))
> @@ -278,32 +277,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>   	spin_unlock_irqrestore(&b->irq_lock, flags);
>   }
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce)
> -{
> -	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&b->irq_lock, flags);
> -	if (!list_empty(&ce->signals)) {
> -		struct i915_request *rq, *next;
> -
> -		/* Queue for executing the signal callbacks in the irq_work */
> -		list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
> -			GEM_BUG_ON(rq->engine != engine);
> -			GEM_BUG_ON(!__request_completed(rq));
> -
> -			__signal_request(rq, &b->signaled_requests);
> -		}
> -
> -		INIT_LIST_HEAD(&ce->signals);
> -		list_del_init(&ce->signal_link);
> -
> -		irq_work_queue(&b->irq_work);
> -	}
> -	spin_unlock_irqrestore(&b->irq_lock, flags);
> -}
> -
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>   {
>   }
> @@ -317,6 +290,17 @@ static void insert_breadcrumb(struct i915_request *rq,
>   	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
>   		return;
>   
> +	/*
> +	 * If the request is already completed, we can transfer it
> +	 * straight onto a signaled list, and queue the irq worker for
> +	 * its signal completion.
> +	 */
> +	if (__request_completed(rq)) {
> +		__signal_request(rq, &b->signaled_requests);
> +		irq_work_queue(&b->irq_work);
> +		return;
> +	}
> +
>   	__intel_breadcrumbs_arm_irq(b);
>   
>   	/*
> @@ -341,8 +325,10 @@ static void insert_breadcrumb(struct i915_request *rq,
>   			break;
>   	}
>   	list_add(&rq->signal_link, pos);
> -	if (pos == &ce->signals) /* catch transitions from empty list */
> +	if (pos == &ce->signals) { /* catch transitions from empty list */
>   		list_move_tail(&ce->signal_link, &b->signalers);
> +		irq_work_queue(&b->irq_work); /* check after enabling irq */
> +	}
>   	GEM_BUG_ON(!check_signal_order(ce, rq));
>   
>   	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> @@ -401,7 +387,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   
>   	spin_unlock(&b->irq_lock);
>   
> -	return !__request_completed(rq);
> +	return true;

Maybe my in head diff apply is failing me, but I think there isn't a 
"return false" path left so could be made a return void function.

Regards,

Tvrtko

>   }
>   
>   void i915_request_cancel_breadcrumb(struct i915_request *rq)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index a9249a23903a..faf00a353e25 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
>   void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>   void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>   
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> -					     struct intel_context *ce);
> -
>   void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
>   				    struct drm_printer *p);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 21c16e31c4fe..88a5c155154d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
>   	return true;
>   }
>   
> -static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
> -{
> -	/*
> -	 * All the outstanding signals on ve->siblings[0] must have
> -	 * been completed, just pending the interrupt handler. As those
> -	 * signals still refer to the old sibling (via rq->engine), we must
> -	 * transfer those to the old irq_worker to keep our locking
> -	 * consistent.
> -	 */
> -	intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
> -}
> -
>   #define for_each_waiter(p__, rq__) \
>   	list_for_each_entry_lockless(p__, \
>   				     &(rq__)->sched.waiters_list, \
> @@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   					virtual_update_register_offsets(regs,
>   									engine);
>   
> -				if (!list_empty(&ve->context.signals))
> -					virtual_xfer_breadcrumbs(ve);
> -
>   				/*
>   				 * Move the bound engine to the top of the list
>   				 * for future execution. We then kick this
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index fc25382e1201..d88f046ccbdd 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -609,9 +609,8 @@ bool __i915_request_submit(struct i915_request *request)
>   	 */
>   	__notify_execute_cb_irq(request);
>   
> -	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
> -	    !i915_request_enable_breadcrumb(request))
> -		intel_engine_signal_breadcrumbs(engine);
> +	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
> +		i915_request_enable_breadcrumb(request);
>   
>   	return result;
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-07-17 09:13:21)
> 
> On 16/07/2020 18:28, Chris Wilson wrote:
> > @@ -341,8 +325,10 @@ static void insert_breadcrumb(struct i915_request *rq,
> >                       break;
> >       }
> >       list_add(&rq->signal_link, pos);
> > -     if (pos == &ce->signals) /* catch transitions from empty list */
> > +     if (pos == &ce->signals) { /* catch transitions from empty list */
> >               list_move_tail(&ce->signal_link, &b->signalers);
> > +             irq_work_queue(&b->irq_work); /* check after enabling irq */
> > +     }
> >       GEM_BUG_ON(!check_signal_order(ce, rq));
> >   
> >       set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > @@ -401,7 +387,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> >   
> >       spin_unlock(&b->irq_lock);
> >   
> > -     return !__request_completed(rq);
> > +     return true;
> 
> Maybe my in head diff apply is failing me, but I think there isn't a 
> "return false" path left so could be made a return void function.

There is no return false path anymore (since we always queue the worker
which should run immediately after dma_fence_enable_signaling if
necessary, that seemed to be more sensible than conditionally using the
worker, I also looked at splitting enable_breadcrumb and
activate_breadcrumb, but the two paths are more similar than not), I
kept it bool so that it matched i915_fence_enable_signaling.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs

[Tvrtko Ursulin]

On 17/07/2020 09:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-07-17 09:13:21)
>>
>> On 16/07/2020 18:28, Chris Wilson wrote:
>>> @@ -341,8 +325,10 @@ static void insert_breadcrumb(struct i915_request *rq,
>>>                        break;
>>>        }
>>>        list_add(&rq->signal_link, pos);
>>> -     if (pos == &ce->signals) /* catch transitions from empty list */
>>> +     if (pos == &ce->signals) { /* catch transitions from empty list */
>>>                list_move_tail(&ce->signal_link, &b->signalers);
>>> +             irq_work_queue(&b->irq_work); /* check after enabling irq */
>>> +     }
>>>        GEM_BUG_ON(!check_signal_order(ce, rq));
>>>    
>>>        set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>>> @@ -401,7 +387,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>>>    
>>>        spin_unlock(&b->irq_lock);
>>>    
>>> -     return !__request_completed(rq);
>>> +     return true;
>>
>> Maybe my in head diff apply is failing me, but I think there isn't a
>> "return false" path left so could be made a return void function.
> 
> There is no return false path anymore (since we always queue the worker
> which should run immediately after dma_fence_enable_signaling if
> necessary, that seemed to be more sensible than conditionally using the
> worker, I also looked at splitting enable_breadcrumb and
> activate_breadcrumb, but the two paths are more similar than not), I
> kept it bool so that it matched i915_fence_enable_signaling.

It's a bit questionable, in this case it would probably be better to 
have explicit "return true" in i915_fence_enable_signaling. But it is a 
minor point anyway and bugfix trumps it.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx