[PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Chris Wilson]

Pull setting -EIO on the hung requests into its own utility function.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 1a2b71157f08..53e823d36b28 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
 				     struct intel_engine_cs *engine,
 				     struct intel_ring *ring);
 
+static void mark_eio(struct i915_request *rq)
+{
+	if (!i915_request_signaled(rq))
+		dma_fence_set_error(&rq->fence, -EIO);
+	i915_request_mark_complete(rq);
+}
+
 static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
 {
 	return (i915_ggtt_offset(engine->status_page.vma) +
@@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 	__execlists_reset(engine, true);
 
 	/* Mark all executing requests as skipped. */
-	list_for_each_entry(rq, &engine->active.requests, sched.link) {
-		if (!i915_request_signaled(rq))
-			dma_fence_set_error(&rq->fence, -EIO);
-
-		i915_request_mark_complete(rq);
-	}
+	list_for_each_entry(rq, &engine->active.requests, sched.link)
+		mark_eio(rq);
 
 	/* Flush the queued requests to the timeline list (for retiring). */
 	while ((rb = rb_first_cached(&execlists->queue))) {
@@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			list_del_init(&rq->sched.link);
+			mark_eio(rq);
 			__i915_request_submit(rq);
-			dma_fence_set_error(&rq->fence, -EIO);
-			i915_request_mark_complete(rq);
 		}
 
 		rb_erase_cached(&p->node, &execlists->queue);
@@ -2533,13 +2534,14 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 		RB_CLEAR_NODE(rb);
 
 		spin_lock(&ve->base.active.lock);
-		if (ve->request) {
-			ve->request->engine = engine;
-			__i915_request_submit(ve->request);
-			dma_fence_set_error(&ve->request->fence, -EIO);
-			i915_request_mark_complete(ve->request);
+		rq = fetch_and_zero(&ve->request);
+		if (rq) {
+			mark_eio(rq);
+
+			rq->engine = engine;
+			__i915_request_submit(rq);
+
 			ve->base.execlists.queue_priority_hint = INT_MIN;
-			ve->request = NULL;
 		}
 		spin_unlock(&ve->base.active.lock);
 	}
-- 
2.23.0

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

[PATCH 2/5] drm/i915: Only enqueue already completed requests

[Chris Wilson]

If we are asked to submit a completed request, just move it onto the
active-list without modifying it's payload. If we try to emit the
modified payload of a completed request, we risk racing with the
ring->head update during retirement which may advance the head past our
breadcrumb and so we generate a warning for the emission being behind
the RING_HEAD.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 45 +++++++++++++----------------
 drivers/gpu/drm/i915/i915_request.c | 28 ++++++++++--------
 drivers/gpu/drm/i915/i915_request.h |  2 +-
 3 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 53e823d36b28..53bc4308793c 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -806,6 +806,9 @@ static bool can_merge_rq(const struct i915_request *prev,
 	GEM_BUG_ON(prev == next);
 	GEM_BUG_ON(!assert_priority_queue(prev, next));
 
+	if (i915_request_completed(next))
+		return true;
+
 	if (!can_merge_ctx(prev->hw_context, next->hw_context))
 		return false;
 
@@ -1188,21 +1191,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				continue;
 			}
 
-			if (i915_request_completed(rq)) {
-				ve->request = NULL;
-				ve->base.execlists.queue_priority_hint = INT_MIN;
-				rb_erase_cached(rb, &execlists->virtual);
-				RB_CLEAR_NODE(rb);
-
-				rq->engine = engine;
-				__i915_request_submit(rq);
-
-				spin_unlock(&ve->base.active.lock);
-
-				rb = rb_first_cached(&execlists->virtual);
-				continue;
-			}
-
 			if (last && !can_merge_rq(last, rq)) {
 				spin_unlock(&ve->base.active.lock);
 				return; /* leave this for another */
@@ -1256,11 +1244,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				GEM_BUG_ON(ve->siblings[0] != engine);
 			}
 
-			__i915_request_submit(rq);
-			if (!i915_request_completed(rq)) {
+			if (__i915_request_submit(rq)) {
 				submit = true;
 				last = rq;
 			}
+
+			if (!submit) {
+				spin_unlock(&ve->base.active.lock);
+				rb = rb_first_cached(&execlists->virtual);
+				continue;
+			}
 		}
 
 		spin_unlock(&ve->base.active.lock);
@@ -1273,8 +1266,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			if (i915_request_completed(rq))
-				goto skip;
+			bool merge = true;
 
 			/*
 			 * Can we combine this request with the current port?
@@ -1315,14 +1307,17 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				    ctx_single_port_submission(rq->hw_context))
 					goto done;
 
-				*port = execlists_schedule_in(last, port - execlists->pending);
-				port++;
+				merge = false;
 			}
 
-			last = rq;
-			submit = true;
-skip:
-			__i915_request_submit(rq);
+			if (__i915_request_submit(rq)) {
+				if (!merge) {
+					*port = execlists_schedule_in(last, port - execlists->pending);
+					port++;
+				}
+				submit = true;
+				last = rq;
+			}
 		}
 
 		rb_erase_cached(&p->node, &execlists->queue);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 9bd8538b1907..db1a0048a753 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
 	return 0;
 }
 
-void __i915_request_submit(struct i915_request *request)
+bool __i915_request_submit(struct i915_request *request)
 {
 	struct intel_engine_cs *engine = request->engine;
+	bool result = false;
 
 	GEM_TRACE("%s fence %llx:%lld, current %d\n",
 		  engine->name,
@@ -389,6 +390,9 @@ void __i915_request_submit(struct i915_request *request)
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->active.lock);
 
+	if (i915_request_completed(request))
+		goto xfer;
+
 	if (i915_gem_context_is_banned(request->gem_context))
 		i915_request_skip(request, -EIO);
 
@@ -412,13 +416,18 @@ void __i915_request_submit(struct i915_request *request)
 	    i915_sw_fence_signaled(&request->semaphore))
 		engine->saturated |= request->sched.semaphores;
 
-	/* We may be recursing from the signal callback of another i915 fence */
-	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
+	engine->emit_fini_breadcrumb(request,
+				     request->ring->vaddr + request->postfix);
 
-	list_move_tail(&request->sched.link, &engine->active.requests);
+	trace_i915_request_execute(request);
+	engine->serial++;
+	result = true;
 
-	GEM_BUG_ON(test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
-	set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
+xfer:	/* We may be recursing from the signal callback of another i915 fence */
+	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
+
+	if (!test_and_set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags))
+		list_move_tail(&request->sched.link, &engine->active.requests);
 
 	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
 	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags) &&
@@ -429,12 +438,7 @@ void __i915_request_submit(struct i915_request *request)
 
 	spin_unlock(&request->lock);
 
-	engine->emit_fini_breadcrumb(request,
-				     request->ring->vaddr + request->postfix);
-
-	engine->serial++;
-
-	trace_i915_request_execute(request);
+	return result;
 }
 
 void i915_request_submit(struct i915_request *request)
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index b18f49528ded..ec5bb4c2e5ae 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -292,7 +292,7 @@ int i915_request_await_execution(struct i915_request *rq,
 
 void i915_request_add(struct i915_request *rq);
 
-void __i915_request_submit(struct i915_request *request);
+bool __i915_request_submit(struct i915_request *request);
 void i915_request_submit(struct i915_request *request);
 
 void i915_request_skip(struct i915_request *request, int error);
-- 
2.23.0

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

[PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Chris Wilson]

As preempt-to-busy leaves the request on the HW as the resubmission is
processed, that request may complete in the background and even cause a
second virtual request to enter queue. This second virtual request
breaks our "single request in the virtual pipeline" assumptions.
Furthermore, as the virtual request may be completed and retired, we
lose the reference the virtual engine assumes is held. Normally, just
removing the request from the scheduler queue removes it from the
engine, but the virtual engine keeps track of its singleton request via
its ve->request. This pointer needs protecting with a reference.

Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 34 ++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 53bc4308793c..1b2bacc60300 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -529,7 +529,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
 				i915_request_cancel_breadcrumb(rq);
 				spin_unlock(&rq->lock);
 			}
-			rq->engine = owner;
 			owner->submit_request(rq);
 			active = NULL;
 		}
@@ -1248,6 +1247,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				submit = true;
 				last = rq;
 			}
+			i915_request_put(rq);
 
 			if (!submit) {
 				spin_unlock(&ve->base.active.lock);
@@ -2535,6 +2535,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 
 			rq->engine = engine;
 			__i915_request_submit(rq);
+			i915_request_put(rq);
 
 			ve->base.execlists.queue_priority_hint = INT_MIN;
 		}
@@ -3787,7 +3788,9 @@ static void virtual_submission_tasklet(unsigned long data)
 
 static void virtual_submit_request(struct i915_request *rq)
 {
-	struct virtual_engine *ve = to_virtual_engine(rq->engine);
+	struct virtual_engine *ve = to_virtual_engine(rq->hw_context->engine);
+	struct i915_request *stale;
+	unsigned long flags;
 
 	GEM_TRACE("%s: rq=%llx:%lld\n",
 		  ve->base.name,
@@ -3796,15 +3799,30 @@ static void virtual_submit_request(struct i915_request *rq)
 
 	GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
 
-	GEM_BUG_ON(ve->request);
-	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
+	spin_lock_irqsave(&ve->base.active.lock, flags);
+
+	stale = ve->request;
+	if (stale) {
+		GEM_BUG_ON(!i915_request_completed(stale));
+		__i915_request_submit(stale);
+		i915_request_put(stale);
+	}
+
+	if (i915_request_completed(rq)) {
+		__i915_request_submit(rq);
+		ve->request = NULL;
+	} else {
+		ve->base.execlists.queue_priority_hint = rq_prio(rq);
+		ve->request = i915_request_get(rq);
+		rq->engine = &ve->base; /* fixup from unwind */
 
-	ve->base.execlists.queue_priority_hint = rq_prio(rq);
-	WRITE_ONCE(ve->request, rq);
+		GEM_BUG_ON(!list_empty(virtual_queue(ve)));
+		list_move_tail(&rq->sched.link, virtual_queue(ve));
 
-	list_move_tail(&rq->sched.link, virtual_queue(ve));
+		tasklet_schedule(&ve->base.execlists.tasklet);
+	}
 
-	tasklet_schedule(&ve->base.execlists.tasklet);
+	spin_unlock_irqrestore(&ve->base.active.lock, flags);
 }
 
 static struct ve_bond *
-- 
2.23.0

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

[PATCH 4/5] drm/i915: Fixup preempt-to-busy vs reset of a virtual request

[Chris Wilson]

Due to the nature of preempt-to-busy the execlists active tracking and
the schedule queue may become temporarily desync'ed (between resubmission
to HW and its ack from HW). This means that we may have unwound a
request and passed it back to the virtual engine, but it is still
inflight on the HW and may even result in a GPU hang. If we detect that
GPU hang and try to reset, the hanging request->engine will no longer
match the current engine, which means that the request is not on the
execlists active list and we should not try to find an older incomplete
request. Given that we have deduced this must be a request on a virtual
engine, it is the single active request in the context and so must be
guilty (as the context is still inflight, it is prevented from being
executed on another engine as we process the reset).

Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c   | 7 +++++--
 drivers/gpu/drm/i915/gt/intel_reset.c | 4 +---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 1b2bacc60300..3eadd294bcd7 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2326,11 +2326,14 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
 
 static struct i915_request *active_request(struct i915_request *rq)
 {
-	const struct list_head * const list =
-		&i915_request_active_timeline(rq)->requests;
 	const struct intel_context * const ce = rq->hw_context;
 	struct i915_request *active = NULL;
+	struct list_head *list;
 
+	if (!i915_request_is_active(rq)) /* unwound, but incomplete! */
+		return rq;
+
+	list = &i915_request_active_timeline(rq)->requests;
 	list_for_each_entry_from_reverse(rq, list, link) {
 		if (i915_request_completed(rq))
 			break;
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 89048ca8924c..ae68c3786f63 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -42,11 +42,10 @@ static void engine_skip_context(struct i915_request *rq)
 	struct intel_engine_cs *engine = rq->engine;
 	struct i915_gem_context *hung_ctx = rq->gem_context;
 
-	lockdep_assert_held(&engine->active.lock);
-
 	if (!i915_request_is_active(rq))
 		return;
 
+	lockdep_assert_held(&engine->active.lock);
 	list_for_each_entry_continue(rq, &engine->active.requests, sched.link)
 		if (rq->gem_context == hung_ctx)
 			i915_request_skip(rq, -EIO);
@@ -123,7 +122,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
 		  rq->fence.seqno,
 		  yesno(guilty));
 
-	lockdep_assert_held(&rq->engine->active.lock);
 	GEM_BUG_ON(i915_request_completed(rq));
 
 	if (guilty) {
-- 
2.23.0

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

[PATCH 5/5] drm/i915: Prevent bonded requests from overtaking each other on preemption

[Chris Wilson]

Force bonded requests to run on distinct engines so that they cannot be
shuffled onto the same engine where timeslicing will reverse the order.
A bonded request will often wait on a semaphore signaled by its master,
creating an implicit dependency -- if we ignore that implicit dependency
and allow the bonded request to run on the same engine and before its
master, we will cause a GPU hang. [Whether it will hang the GPU is
debatable, we should keep on timeslicing and each timeslice should be
"accidentally" counted as forward progress, in which case it should run
but at one-half to one-third speed.]

We can prevent this inversion by restricting which engines we allow
ourselves to jump to upon preemption, i.e. baking in the arrangement
established at first execution. (We should also consider capturing the
implicit dependency using i915_sched_add_dependency(), but first we need
to think about the constraints that requires on the execution/retirement
ordering.)

Fixes: 8ee36e048c98 ("drm/i915/execlists: Minimalistic timeslicing")
References: ee1136908e9b ("drm/i915/execlists: Virtual engine bonding")
Testcase: igt/gem_exec_balancer/bonded-slice
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 3eadd294bcd7..9872bb4c99fc 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -3846,18 +3846,22 @@ static void
 virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
 {
 	struct virtual_engine *ve = to_virtual_engine(rq->engine);
+	intel_engine_mask_t allowed, exec;
 	struct ve_bond *bond;
 
+	allowed = ~to_request(signal)->engine->mask;
+
 	bond = virtual_find_bond(ve, to_request(signal)->engine);
-	if (bond) {
-		intel_engine_mask_t old, new, cmp;
+	if (bond)
+		allowed &= bond->sibling_mask;
 
-		cmp = READ_ONCE(rq->execution_mask);
-		do {
-			old = cmp;
-			new = cmp & bond->sibling_mask;
-		} while ((cmp = cmpxchg(&rq->execution_mask, old, new)) != old);
-	}
+	/* Restrict the bonded request to run on only the available engines */
+	exec = READ_ONCE(rq->execution_mask);
+	while (!try_cmpxchg(&rq->execution_mask, &exec, exec & allowed))
+		;
+
+	/* Prevent the master from being re-run on the bonded engines */
+	to_request(signal)->execution_mask &= ~allowed;
 }
 
 struct intel_context *
-- 
2.23.0

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests
URL   : https://patchwork.freedesktop.org/series/67033/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3baa7cddeeb6 drm/i915/execlists: Refactor -EIO markup of hung requests
24a30775d591 drm/i915: Only enqueue already completed requests
-:95: WARNING:LONG_LINE: line over 100 characters
#95: FILE: drivers/gpu/drm/i915/gt/intel_lrc.c:1315:
+					*port = execlists_schedule_in(last, port - execlists->pending);

total: 0 errors, 1 warnings, 0 checks, 144 lines checked
7194038919bb drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request
370e4dd01c7f drm/i915: Fixup preempt-to-busy vs reset of a virtual request
47afa3fba94d drm/i915: Prevent bonded requests from overtaking each other on preemption
-:25: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit ee1136908e9b ("drm/i915/execlists: Virtual engine bonding")'
#25: 
References: ee1136908e9b ("drm/i915/execlists: Virtual engine bonding")

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

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

✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests
URL   : https://patchwork.freedesktop.org/series/67033/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6934 -> Patchwork_14487
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@basic-small-bo-tiledy:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-icl-u3/igt@gem_mmap_gtt@basic-small-bo-tiledy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-icl-u3/igt@gem_mmap_gtt@basic-small-bo-tiledy.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [INCOMPLETE][3] ([fdo#107718]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_mmap_gtt@basic-write:
    - fi-icl-u3:          [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6] +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-icl-u3/igt@gem_mmap_gtt@basic-write.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-icl-u3/igt@gem_mmap_gtt@basic-write.html

  * igt@kms_busy@basic-flip-c:
    - fi-skl-6770hq:      [SKIP][7] ([fdo#109271] / [fdo#109278]) -> [PASS][8] +2 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-skl-6770hq/igt@kms_busy@basic-flip-c.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-skl-6770hq/igt@kms_busy@basic-flip-c.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [FAIL][9] ([fdo#109483]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6770hq:      [SKIP][11] ([fdo#109271]) -> [PASS][12] +23 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-skl-6770hq/igt@kms_flip@basic-flip-vs-dpms.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-skl-6770hq/igt@kms_flip@basic-flip-vs-dpms.html

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-guc}:       [FAIL][13] ([fdo#103167]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html

  
#### Warnings ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][15] ([fdo#111045] / [fdo#111096]) -> [FAIL][16] ([fdo#111407])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

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

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735


Participating hosts (55 -> 47)
------------------------------

  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-pnv-d510 fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6934 -> Patchwork_14487

  CI-20190529: 20190529
  CI_DRM_6934: dd5bccfa3b5d8de4b5529db8145341c65930a710 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5196: 98783313b8b3097680df69007a9551c6248ab209 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14487: 47afa3fba94d4b5597a679028b3689cb1a0c631a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

47afa3fba94d drm/i915: Prevent bonded requests from overtaking each other on preemption
370e4dd01c7f drm/i915: Fixup preempt-to-busy vs reset of a virtual request
7194038919bb drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request
24a30775d591 drm/i915: Only enqueue already completed requests
3baa7cddeeb6 drm/i915/execlists: Refactor -EIO markup of hung requests

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests
URL   : https://patchwork.freedesktop.org/series/67033/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6934_full -> Patchwork_14487_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14487_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14487_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_14487_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-iclb:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_shared@exec-single-timeline-bsd:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#110841])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb5/igt@gem_ctx_shared@exec-single-timeline-bsd.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@gem_ctx_shared@exec-single-timeline-bsd.html

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#108566])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-kbl3/igt@gem_eio@in-flight-suspend.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-kbl1/igt@gem_eio@in-flight-suspend.html

  * igt@gem_exec_async@concurrent-writes-bsd:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#111325]) +3 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb5/igt@gem_exec_async@concurrent-writes-bsd.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@gem_exec_async@concurrent-writes-bsd.html

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-skl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#104108])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl6/igt@gem_workarounds@suspend-resume-fd.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl2/igt@gem_workarounds@suspend-resume-fd.html

  * igt@kms_flip@2x-modeset-vs-vblank-race:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([fdo#111609])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-glk3/igt@kms_flip@2x-modeset-vs-vblank-race.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-glk4/igt@kms_flip@2x-modeset-vs-vblank-race.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#105363])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
    - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#105363])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-glk4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-glk2/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#103167]) +7 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([fdo#108145] / [fdo#110403])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl5/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109441]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb1/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-apl:          [PASS][23] -> [DMESG-WARN][24] ([fdo#108566]) +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-apl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-apl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [PASS][25] -> [SKIP][26] ([fdo#109276]) +16 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb4/igt@prime_busy@hang-bsd2.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb6/igt@prime_busy@hang-bsd2.html

  
#### Possible fixes ####

  * {igt@gem_eio@kms}:
    - shard-snb:          [INCOMPLETE][27] ([fdo#105411]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-snb2/igt@gem_eio@kms.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-snb1/igt@gem_eio@kms.html

  * igt@gem_eio@unwedge-stress:
    - shard-glk:          [FAIL][29] ([fdo#109661]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-glk1/igt@gem_eio@unwedge-stress.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-glk1/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][31] ([fdo#110854]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb5/igt@gem_exec_balancer@smoke.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_reloc@basic-cpu-read-active:
    - shard-skl:          [DMESG-WARN][33] ([fdo#106107]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl10/igt@gem_exec_reloc@basic-cpu-read-active.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl2/igt@gem_exec_reloc@basic-cpu-read-active.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [SKIP][35] ([fdo#111325]) -> [PASS][36] +6 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb1/igt@gem_exec_schedule@in-order-bsd.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb6/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@independent-bsd2:
    - shard-iclb:         [SKIP][37] ([fdo#109276]) -> [PASS][38] +15 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb6/igt@gem_exec_schedule@independent-bsd2.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb1/igt@gem_exec_schedule@independent-bsd2.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-apl:          [DMESG-WARN][39] ([fdo#108686]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-apl1/igt@gem_tiled_swapping@non-threaded.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-apl3/igt@gem_tiled_swapping@non-threaded.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-apl:          [DMESG-WARN][41] ([fdo#108566]) -> [PASS][42] +8 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-apl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-apl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          [INCOMPLETE][43] ([fdo#109507]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl8/igt@kms_flip@flip-vs-suspend-interruptible.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         [FAIL][45] ([fdo#103167]) -> [PASS][46] +8 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          [DMESG-WARN][47] ([fdo#108566]) -> [PASS][48] +3 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - shard-skl:          [FAIL][49] ([fdo#103191]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl4/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl8/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][51] ([fdo#108145]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][53] ([fdo#103166]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb4/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [SKIP][55] ([fdo#109441]) -> [PASS][56] +3 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb5/igt@kms_psr@psr2_primary_mmap_cpu.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [FAIL][57] ([fdo#111329]) -> [SKIP][58] ([fdo#109276])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb1/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb6/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_mocs_settings@mocs-reset-bsd2:
    - shard-iclb:         [SKIP][59] ([fdo#109276]) -> [FAIL][60] ([fdo#111330]) +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6934/shard-iclb5/igt@gem_mocs_settings@mocs-reset-bsd2.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14487/shard-iclb2/igt@gem_mocs_settings@mocs-reset-bsd2.html

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

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#111609]: https://bugs.freedesktop.org/show_bug.cgi?id=111609


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

  Additional (1): pig-snb-2600 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6934 -> Patchwork_14487

  CI-20190529: 20190529
  CI_DRM_6934: dd5bccfa3b5d8de4b5529db8145341c65930a710 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5196: 98783313b8b3097680df69007a9551c6248ab209 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14487: 47afa3fba94d4b5597a679028b3689cb1a0c631a @ 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_14487/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Pull setting -EIO on the hung requests into its own utility function.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 1a2b71157f08..53e823d36b28 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
>  				     struct intel_engine_cs *engine,
>  				     struct intel_ring *ring);
>  
> +static void mark_eio(struct i915_request *rq)
> +{
> +	if (!i915_request_signaled(rq))
> +		dma_fence_set_error(&rq->fence, -EIO);
> +	i915_request_mark_complete(rq);
> +}
> +
>  static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>  {
>  	return (i915_ggtt_offset(engine->status_page.vma) +
> @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>  	__execlists_reset(engine, true);
>  
>  	/* Mark all executing requests as skipped. */
> -	list_for_each_entry(rq, &engine->active.requests, sched.link) {
> -		if (!i915_request_signaled(rq))
> -			dma_fence_set_error(&rq->fence, -EIO);
> -
> -		i915_request_mark_complete(rq);
> -	}
> +	list_for_each_entry(rq, &engine->active.requests, sched.link)
> +		mark_eio(rq);
>  
>  	/* Flush the queued requests to the timeline list (for retiring). */
>  	while ((rb = rb_first_cached(&execlists->queue))) {
> @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>  		int i;
>  
>  		priolist_for_each_request_consume(rq, rn, p, i) {
> -			list_del_init(&rq->sched.link);
> +			mark_eio(rq);
>  			__i915_request_submit(rq);
> -			dma_fence_set_error(&rq->fence, -EIO);
> -			i915_request_mark_complete(rq);

Now we marek complete before submitting. Does that bring problems?

-Mika


>  		}
>  
>  		rb_erase_cached(&p->node, &execlists->queue);
> @@ -2533,13 +2534,14 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>  		RB_CLEAR_NODE(rb);
>  
>  		spin_lock(&ve->base.active.lock);
> -		if (ve->request) {
> -			ve->request->engine = engine;
> -			__i915_request_submit(ve->request);
> -			dma_fence_set_error(&ve->request->fence, -EIO);
> -			i915_request_mark_complete(ve->request);
> +		rq = fetch_and_zero(&ve->request);
> +		if (rq) {
> +			mark_eio(rq);
> +
> +			rq->engine = engine;
> +			__i915_request_submit(rq);
> +
>  			ve->base.execlists.queue_priority_hint = INT_MIN;
> -			ve->request = NULL;
>  		}
>  		spin_unlock(&ve->base.active.lock);
>  	}
> -- 
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Chris Wilson]

Quoting Mika Kuoppala (2019-09-23 08:47:18)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Pull setting -EIO on the hung requests into its own utility function.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 1a2b71157f08..53e823d36b28 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
> >                                    struct intel_engine_cs *engine,
> >                                    struct intel_ring *ring);
> >  
> > +static void mark_eio(struct i915_request *rq)
> > +{
> > +     if (!i915_request_signaled(rq))
> > +             dma_fence_set_error(&rq->fence, -EIO);
> > +     i915_request_mark_complete(rq);
> > +}
> > +
> >  static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
> >  {
> >       return (i915_ggtt_offset(engine->status_page.vma) +
> > @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >       __execlists_reset(engine, true);
> >  
> >       /* Mark all executing requests as skipped. */
> > -     list_for_each_entry(rq, &engine->active.requests, sched.link) {
> > -             if (!i915_request_signaled(rq))
> > -                     dma_fence_set_error(&rq->fence, -EIO);
> > -
> > -             i915_request_mark_complete(rq);
> > -     }
> > +     list_for_each_entry(rq, &engine->active.requests, sched.link)
> > +             mark_eio(rq);
> >  
> >       /* Flush the queued requests to the timeline list (for retiring). */
> >       while ((rb = rb_first_cached(&execlists->queue))) {
> > @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >               int i;
> >  
> >               priolist_for_each_request_consume(rq, rn, p, i) {
> > -                     list_del_init(&rq->sched.link);
> > +                     mark_eio(rq);
> >                       __i915_request_submit(rq);
> > -                     dma_fence_set_error(&rq->fence, -EIO);
> > -                     i915_request_mark_complete(rq);
> 
> Now we marek complete before submitting. Does that bring problems?

In conjunction with the next patches in the series, it prevents a problem :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Tvrtko Ursulin]

On 21/09/2019 10:55, Chris Wilson wrote:
> Pull setting -EIO on the hung requests into its own utility function.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 1a2b71157f08..53e823d36b28 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
>   				     struct intel_engine_cs *engine,
>   				     struct intel_ring *ring);
>   
> +static void mark_eio(struct i915_request *rq)
> +{
> +	if (!i915_request_signaled(rq))
> +		dma_fence_set_error(&rq->fence, -EIO);
> +	i915_request_mark_complete(rq);
> +}
> +
>   static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>   {
>   	return (i915_ggtt_offset(engine->status_page.vma) +
> @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   	__execlists_reset(engine, true);
>   
>   	/* Mark all executing requests as skipped. */
> -	list_for_each_entry(rq, &engine->active.requests, sched.link) {
> -		if (!i915_request_signaled(rq))
> -			dma_fence_set_error(&rq->fence, -EIO);
> -
> -		i915_request_mark_complete(rq);
> -	}
> +	list_for_each_entry(rq, &engine->active.requests, sched.link)
> +		mark_eio(rq);
>   
>   	/* Flush the queued requests to the timeline list (for retiring). */
>   	while ((rb = rb_first_cached(&execlists->queue))) {
> @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   		int i;
>   
>   		priolist_for_each_request_consume(rq, rn, p, i) {
> -			list_del_init(&rq->sched.link);

list_del_init not needed any more? Should be mentioned in the commit 
message.

> +			mark_eio(rq);
>   			__i915_request_submit(rq);
> -			dma_fence_set_error(&rq->fence, -EIO);
> -			i915_request_mark_complete(rq);

I am also curious about Mika's question - if the change in ordering of 
submit vs mark_complete is important it should be mentioned in the commit.

>   		}
>   
>   		rb_erase_cached(&p->node, &execlists->queue);
> @@ -2533,13 +2534,14 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   		RB_CLEAR_NODE(rb);
>   
>   		spin_lock(&ve->base.active.lock);
> -		if (ve->request) {
> -			ve->request->engine = engine;
> -			__i915_request_submit(ve->request);
> -			dma_fence_set_error(&ve->request->fence, -EIO);
> -			i915_request_mark_complete(ve->request);
> +		rq = fetch_and_zero(&ve->request);
> +		if (rq) {
> +			mark_eio(rq);
> +
> +			rq->engine = engine;
> +			__i915_request_submit(rq);
> +
>   			ve->base.execlists.queue_priority_hint = INT_MIN;
> -			ve->request = NULL;
>   		}
>   		spin_unlock(&ve->base.active.lock);
>   	}
> 

Regards,

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

Re: [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 10:27:01)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > Pull setting -EIO on the hung requests into its own utility function.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
> >   1 file changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 1a2b71157f08..53e823d36b28 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
> >                                    struct intel_engine_cs *engine,
> >                                    struct intel_ring *ring);
> >   
> > +static void mark_eio(struct i915_request *rq)
> > +{
> > +     if (!i915_request_signaled(rq))
> > +             dma_fence_set_error(&rq->fence, -EIO);
> > +     i915_request_mark_complete(rq);
> > +}
> > +
> >   static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
> >   {
> >       return (i915_ggtt_offset(engine->status_page.vma) +
> > @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >       __execlists_reset(engine, true);
> >   
> >       /* Mark all executing requests as skipped. */
> > -     list_for_each_entry(rq, &engine->active.requests, sched.link) {
> > -             if (!i915_request_signaled(rq))
> > -                     dma_fence_set_error(&rq->fence, -EIO);
> > -
> > -             i915_request_mark_complete(rq);
> > -     }
> > +     list_for_each_entry(rq, &engine->active.requests, sched.link)
> > +             mark_eio(rq);
> >   
> >       /* Flush the queued requests to the timeline list (for retiring). */
> >       while ((rb = rb_first_cached(&execlists->queue))) {
> > @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >               int i;
> >   
> >               priolist_for_each_request_consume(rq, rn, p, i) {
> > -                     list_del_init(&rq->sched.link);
> 
> list_del_init not needed any more? Should be mentioned in the commit 
> message.

It's not been needed for a long time; only just noticed it was still
there.
 
> > +                     mark_eio(rq);
> >                       __i915_request_submit(rq);
> > -                     dma_fence_set_error(&rq->fence, -EIO);
> > -                     i915_request_mark_complete(rq);
> 
> I am also curious about Mika's question - if the change in ordering of 
> submit vs mark_complete is important it should be mentioned in the commit.

It's not important as the gpu is wedged, it just ties together with the
next patch where if already completed, we take a short-path through
__i915_request_submit.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Only enqueue already completed requests

[Tvrtko Ursulin]

On 21/09/2019 10:55, Chris Wilson wrote:
> If we are asked to submit a completed request, just move it onto the
> active-list without modifying it's payload. If we try to emit the
> modified payload of a completed request, we risk racing with the
> ring->head update during retirement which may advance the head past our
> breadcrumb and so we generate a warning for the emission being behind
> the RING_HEAD.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 45 +++++++++++++----------------
>   drivers/gpu/drm/i915/i915_request.c | 28 ++++++++++--------
>   drivers/gpu/drm/i915/i915_request.h |  2 +-
>   3 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 53e823d36b28..53bc4308793c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -806,6 +806,9 @@ static bool can_merge_rq(const struct i915_request *prev,
>   	GEM_BUG_ON(prev == next);
>   	GEM_BUG_ON(!assert_priority_queue(prev, next));
>   
> +	if (i915_request_completed(next))
> +		return true;
> +

This reads very un-intuitive. Why would it always be okay to merge 
possibly unrelated requests? From which it follows it must be a hack of 
some kind - from which it follows it needs a comment to explain it.

>   	if (!can_merge_ctx(prev->hw_context, next->hw_context))
>   		return false;
>   
> @@ -1188,21 +1191,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				continue;
>   			}
>   
> -			if (i915_request_completed(rq)) {
> -				ve->request = NULL;
> -				ve->base.execlists.queue_priority_hint = INT_MIN;
> -				rb_erase_cached(rb, &execlists->virtual);
> -				RB_CLEAR_NODE(rb);
> -
> -				rq->engine = engine;
> -				__i915_request_submit(rq);
> -
> -				spin_unlock(&ve->base.active.lock);
> -
> -				rb = rb_first_cached(&execlists->virtual);
> -				continue;
> -			}
> -
>   			if (last && !can_merge_rq(last, rq)) {
>   				spin_unlock(&ve->base.active.lock);
>   				return; /* leave this for another */
> @@ -1256,11 +1244,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				GEM_BUG_ON(ve->siblings[0] != engine);
>   			}
>   
> -			__i915_request_submit(rq);
> -			if (!i915_request_completed(rq)) {
> +			if (__i915_request_submit(rq)) {
>   				submit = true;
>   				last = rq;
>   			}
> +
> +			if (!submit) {
> +				spin_unlock(&ve->base.active.lock);
> +				rb = rb_first_cached(&execlists->virtual);
> +				continue;
> +			}

This block also needs a comment I think. It's about skipping until first 
incomplete request in the queue?

>   		}
>   
>   		spin_unlock(&ve->base.active.lock);
> @@ -1273,8 +1266,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		int i;
>   
>   		priolist_for_each_request_consume(rq, rn, p, i) {
> -			if (i915_request_completed(rq))
> -				goto skip;
> +			bool merge = true;
>   
>   			/*
>   			 * Can we combine this request with the current port?
> @@ -1315,14 +1307,17 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				    ctx_single_port_submission(rq->hw_context))
>   					goto done;
>   
> -				*port = execlists_schedule_in(last, port - execlists->pending);
> -				port++;
> +				merge = false;
>   			}
>   
> -			last = rq;
> -			submit = true;
> -skip:
> -			__i915_request_submit(rq);
> +			if (__i915_request_submit(rq)) {
> +				if (!merge) {
> +					*port = execlists_schedule_in(last, port - execlists->pending);
> +					port++;
> +				}
> +				submit = true;
> +				last = rq;
> +			}
>   		}
>   
>   		rb_erase_cached(&p->node, &execlists->queue);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 9bd8538b1907..db1a0048a753 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
>   	return 0;
>   }
>   
> -void __i915_request_submit(struct i915_request *request)
> +bool __i915_request_submit(struct i915_request *request)
>   {
>   	struct intel_engine_cs *engine = request->engine;
> +	bool result = false;
>   
>   	GEM_TRACE("%s fence %llx:%lld, current %d\n",
>   		  engine->name,
> @@ -389,6 +390,9 @@ void __i915_request_submit(struct i915_request *request)
>   	GEM_BUG_ON(!irqs_disabled());
>   	lockdep_assert_held(&engine->active.lock);
>   
> +	if (i915_request_completed(request))
> +		goto xfer;

A comment here as well I think to explain what happens next with 
completed requests put on the active list. They just get removed during 
retire? Do they need to even be put on that list?

> +
>   	if (i915_gem_context_is_banned(request->gem_context))
>   		i915_request_skip(request, -EIO);
>   
> @@ -412,13 +416,18 @@ void __i915_request_submit(struct i915_request *request)
>   	    i915_sw_fence_signaled(&request->semaphore))
>   		engine->saturated |= request->sched.semaphores;
>   
> -	/* We may be recursing from the signal callback of another i915 fence */
> -	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> +	engine->emit_fini_breadcrumb(request,
> +				     request->ring->vaddr + request->postfix);
>   
> -	list_move_tail(&request->sched.link, &engine->active.requests);
> +	trace_i915_request_execute(request);
> +	engine->serial++;
> +	result = true;
>   
> -	GEM_BUG_ON(test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
> -	set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
> +xfer:	/* We may be recursing from the signal callback of another i915 fence */
> +	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> +
> +	if (!test_and_set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags))
> +		list_move_tail(&request->sched.link, &engine->active.requests);
>   
>   	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
>   	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags) &&
> @@ -429,12 +438,7 @@ void __i915_request_submit(struct i915_request *request)
>   
>   	spin_unlock(&request->lock);
>   
> -	engine->emit_fini_breadcrumb(request,
> -				     request->ring->vaddr + request->postfix);
> -
> -	engine->serial++;
> -
> -	trace_i915_request_execute(request);
> +	return result;
>   }
>   
>   void i915_request_submit(struct i915_request *request)
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index b18f49528ded..ec5bb4c2e5ae 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -292,7 +292,7 @@ int i915_request_await_execution(struct i915_request *rq,
>   
>   void i915_request_add(struct i915_request *rq);
>   
> -void __i915_request_submit(struct i915_request *request);
> +bool __i915_request_submit(struct i915_request *request);
>   void i915_request_submit(struct i915_request *request);
>   
>   void i915_request_skip(struct i915_request *request, int error);
> 

Regards,

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

Re: [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Tvrtko Ursulin]

On 23/09/2019 10:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-23 10:27:01)
>>
>> On 21/09/2019 10:55, Chris Wilson wrote:
>>> Pull setting -EIO on the hung requests into its own utility function.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
>>>    1 file changed, 17 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 1a2b71157f08..53e823d36b28 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
>>>                                     struct intel_engine_cs *engine,
>>>                                     struct intel_ring *ring);
>>>    
>>> +static void mark_eio(struct i915_request *rq)
>>> +{
>>> +     if (!i915_request_signaled(rq))
>>> +             dma_fence_set_error(&rq->fence, -EIO);
>>> +     i915_request_mark_complete(rq);
>>> +}
>>> +
>>>    static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>>>    {
>>>        return (i915_ggtt_offset(engine->status_page.vma) +
>>> @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>>        __execlists_reset(engine, true);
>>>    
>>>        /* Mark all executing requests as skipped. */
>>> -     list_for_each_entry(rq, &engine->active.requests, sched.link) {
>>> -             if (!i915_request_signaled(rq))
>>> -                     dma_fence_set_error(&rq->fence, -EIO);
>>> -
>>> -             i915_request_mark_complete(rq);
>>> -     }
>>> +     list_for_each_entry(rq, &engine->active.requests, sched.link)
>>> +             mark_eio(rq);
>>>    
>>>        /* Flush the queued requests to the timeline list (for retiring). */
>>>        while ((rb = rb_first_cached(&execlists->queue))) {
>>> @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>>                int i;
>>>    
>>>                priolist_for_each_request_consume(rq, rn, p, i) {
>>> -                     list_del_init(&rq->sched.link);
>>
>> list_del_init not needed any more? Should be mentioned in the commit
>> message.
> 
> It's not been needed for a long time; only just noticed it was still
> there.
>   
>>> +                     mark_eio(rq);
>>>                        __i915_request_submit(rq);
>>> -                     dma_fence_set_error(&rq->fence, -EIO);
>>> -                     i915_request_mark_complete(rq);
>>
>> I am also curious about Mika's question - if the change in ordering of
>> submit vs mark_complete is important it should be mentioned in the commit.
> 
> It's not important as the gpu is wedged, it just ties together with the
> next patch where if already completed, we take a short-path through
> __i915_request_submit.

Okay, after reading the next patch I see what will happen.

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

(Keep r-b after list_del respin.)

Regards,

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

Re: [PATCH 2/5] drm/i915: Only enqueue already completed requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 10:51:48)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > If we are asked to submit a completed request, just move it onto the
> > active-list without modifying it's payload. If we try to emit the
> > modified payload of a completed request, we risk racing with the
> > ring->head update during retirement which may advance the head past our
> > breadcrumb and so we generate a warning for the emission being behind
> > the RING_HEAD.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 45 +++++++++++++----------------
> >   drivers/gpu/drm/i915/i915_request.c | 28 ++++++++++--------
> >   drivers/gpu/drm/i915/i915_request.h |  2 +-
> >   3 files changed, 37 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 53e823d36b28..53bc4308793c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -806,6 +806,9 @@ static bool can_merge_rq(const struct i915_request *prev,
> >       GEM_BUG_ON(prev == next);
> >       GEM_BUG_ON(!assert_priority_queue(prev, next));
> >   
> > +     if (i915_request_completed(next))
> > +             return true;
> > +
> 
> This reads very un-intuitive. Why would it always be okay to merge 
> possibly unrelated requests? From which it follows it must be a hack of 
> some kind - from which it follows it needs a comment to explain it.

We don't submit a known completed request, hence there is no context
switch.

> > @@ -1256,11 +1244,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                               GEM_BUG_ON(ve->siblings[0] != engine);
> >                       }
> >   
> > -                     __i915_request_submit(rq);
> > -                     if (!i915_request_completed(rq)) {
> > +                     if (__i915_request_submit(rq)) {
> >                               submit = true;
> >                               last = rq;
> >                       }
> > +
> > +                     if (!submit) {
> > +                             spin_unlock(&ve->base.active.lock);
> > +                             rb = rb_first_cached(&execlists->virtual);
> > +                             continue;
> > +                     }
> 
> This block also needs a comment I think. It's about skipping until first 
> incomplete request in the queue?

It's the same logic as before. It's just saying having detected that we
have a bunch of veng requests, keep searching that tree in decreasing
priority order until it is no longer relevant.

> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 9bd8538b1907..db1a0048a753 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
> >       return 0;
> >   }
> >   
> > -void __i915_request_submit(struct i915_request *request)
> > +bool __i915_request_submit(struct i915_request *request)
> >   {
> >       struct intel_engine_cs *engine = request->engine;
> > +     bool result = false;
> >   
> >       GEM_TRACE("%s fence %llx:%lld, current %d\n",
> >                 engine->name,
> > @@ -389,6 +390,9 @@ void __i915_request_submit(struct i915_request *request)
> >       GEM_BUG_ON(!irqs_disabled());
> >       lockdep_assert_held(&engine->active.lock);
> >   
> > +     if (i915_request_completed(request))
> > +             goto xfer;
> 
> A comment here as well I think to explain what happens next with 
> completed requests put on the active list. They just get removed during 
> retire? Do they need to even be put on that list?

They get removed during retire. They must be removed from the priority
queue; and if they had been retired already they would not be in the
priority queue, ergo at this point they are completed and not retired.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Only enqueue already completed requests

[Chris Wilson]

If we are asked to submit a completed request, just move it onto the
active-list without modifying it's payload. If we try to emit the
modified payload of a completed request, we risk racing with the
ring->head update during retirement which may advance the head past our
breadcrumb and so we generate a warning for the emission being behind
the RING_HEAD.

v2: Commentary for the sneaky, shared responsibility between functions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 60 +++++++++++++++++------------
 drivers/gpu/drm/i915/i915_request.c | 44 +++++++++++++++------
 drivers/gpu/drm/i915/i915_request.h |  2 +-
 3 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 0a4812ebd184..8c1ea5c315ac 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -799,6 +799,17 @@ static bool can_merge_rq(const struct i915_request *prev,
 	GEM_BUG_ON(prev == next);
 	GEM_BUG_ON(!assert_priority_queue(prev, next));
 
+	/*
+	 * We not submit known completed requests. Therefore if the next
+	 * request is already completed, we can pretend to merge it in
+	 * with the previous context (and we will skip updating the ELSP
+	 * and tracking). Thus hopefully keeping the ELSP full with active
+	 * contexts, despite the best efforts of preempt-to-busy to confuse
+	 * us.
+	 */
+	if (i915_request_completed(next))
+		return true;
+
 	if (!can_merge_ctx(prev->hw_context, next->hw_context))
 		return false;
 
@@ -1181,21 +1192,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				continue;
 			}
 
-			if (i915_request_completed(rq)) {
-				ve->request = NULL;
-				ve->base.execlists.queue_priority_hint = INT_MIN;
-				rb_erase_cached(rb, &execlists->virtual);
-				RB_CLEAR_NODE(rb);
-
-				rq->engine = engine;
-				__i915_request_submit(rq);
-
-				spin_unlock(&ve->base.active.lock);
-
-				rb = rb_first_cached(&execlists->virtual);
-				continue;
-			}
-
 			if (last && !can_merge_rq(last, rq)) {
 				spin_unlock(&ve->base.active.lock);
 				return; /* leave this for another */
@@ -1249,11 +1245,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				GEM_BUG_ON(ve->siblings[0] != engine);
 			}
 
-			__i915_request_submit(rq);
-			if (!i915_request_completed(rq)) {
+			if (__i915_request_submit(rq)) {
 				submit = true;
 				last = rq;
 			}
+
+			/*
+			 * Hmm, we have a bunch of virtual engine requests,
+			 * but the first one was already complete (thanks
+			 * preempt-to-busy!). Keep looking at the veng queue
+			 * until we have no more relevent requests (i.e.
+			 * the normal submit queue has higher priority).
+			 */
+			if (!submit) {
+				spin_unlock(&ve->base.active.lock);
+				rb = rb_first_cached(&execlists->virtual);
+				continue;
+			}
 		}
 
 		spin_unlock(&ve->base.active.lock);
@@ -1266,8 +1274,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			if (i915_request_completed(rq))
-				goto skip;
+			bool merge = true;
 
 			/*
 			 * Can we combine this request with the current port?
@@ -1308,14 +1315,17 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				    ctx_single_port_submission(rq->hw_context))
 					goto done;
 
-				*port = execlists_schedule_in(last, port - execlists->pending);
-				port++;
+				merge = false;
 			}
 
-			last = rq;
-			submit = true;
-skip:
-			__i915_request_submit(rq);
+			if (__i915_request_submit(rq)) {
+				if (!merge) {
+					*port = execlists_schedule_in(last, port - execlists->pending);
+					port++;
+				}
+				submit = true;
+				last = rq;
+			}
 		}
 
 		rb_erase_cached(&p->node, &execlists->queue);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 9bd8538b1907..0ca43ca15ca0 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
 	return 0;
 }
 
-void __i915_request_submit(struct i915_request *request)
+bool __i915_request_submit(struct i915_request *request)
 {
 	struct intel_engine_cs *engine = request->engine;
+	bool result = false;
 
 	GEM_TRACE("%s fence %llx:%lld, current %d\n",
 		  engine->name,
@@ -389,6 +390,25 @@ void __i915_request_submit(struct i915_request *request)
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->active.lock);
 
+	/*
+	 * With the advent of preempt-to-busy, we frequently encounter
+	 * requests that we have unsubmitted from HW, but left running
+	 * until the next ack and have completed in the meantime. On
+	 * resubmission of that completed request, we can skip
+	 * updating the payload (and execlists can even skip submitting
+	 * the request).
+	 *
+	 * We must remove the request from the caller's priority queue,
+	 * and the caller must only call us when the request is in their
+	 * priority queue, under the active.lock. This ensures that the
+	 * request has *not* yet been retired and we can safely move
+	 * the request into the engine->active.list where it will be
+	 * dropped upon retiring. (Otherwise if resubmit a *retired*
+	 * request, this would be a horrible use-after-free.)
+	 */
+	if (i915_request_completed(request))
+		goto xfer;
+
 	if (i915_gem_context_is_banned(request->gem_context))
 		i915_request_skip(request, -EIO);
 
@@ -412,13 +432,18 @@ void __i915_request_submit(struct i915_request *request)
 	    i915_sw_fence_signaled(&request->semaphore))
 		engine->saturated |= request->sched.semaphores;
 
-	/* We may be recursing from the signal callback of another i915 fence */
-	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
+	engine->emit_fini_breadcrumb(request,
+				     request->ring->vaddr + request->postfix);
+
+	trace_i915_request_execute(request);
+	engine->serial++;
+	result = true;
 
-	list_move_tail(&request->sched.link, &engine->active.requests);
+xfer:	/* 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));
-	set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
+	if (!test_and_set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags))
+		list_move_tail(&request->sched.link, &engine->active.requests);
 
 	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
 	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags) &&
@@ -429,12 +454,7 @@ void __i915_request_submit(struct i915_request *request)
 
 	spin_unlock(&request->lock);
 
-	engine->emit_fini_breadcrumb(request,
-				     request->ring->vaddr + request->postfix);
-
-	engine->serial++;
-
-	trace_i915_request_execute(request);
+	return result;
 }
 
 void i915_request_submit(struct i915_request *request)
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index b18f49528ded..ec5bb4c2e5ae 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -292,7 +292,7 @@ int i915_request_await_execution(struct i915_request *rq,
 
 void i915_request_add(struct i915_request *rq);
 
-void __i915_request_submit(struct i915_request *request);
+bool __i915_request_submit(struct i915_request *request);
 void i915_request_submit(struct i915_request *request);
 
 void i915_request_skip(struct i915_request *request, int error);
-- 
2.23.0

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

Re: [PATCH v2] drm/i915: Only enqueue already completed requests

[Tvrtko Ursulin]

On 23/09/2019 11:32, Chris Wilson wrote:
> If we are asked to submit a completed request, just move it onto the
> active-list without modifying it's payload. If we try to emit the
> modified payload of a completed request, we risk racing with the
> ring->head update during retirement which may advance the head past our
> breadcrumb and so we generate a warning for the emission being behind
> the RING_HEAD.
> 
> v2: Commentary for the sneaky, shared responsibility between functions.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 60 +++++++++++++++++------------
>   drivers/gpu/drm/i915/i915_request.c | 44 +++++++++++++++------
>   drivers/gpu/drm/i915/i915_request.h |  2 +-
>   3 files changed, 68 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 0a4812ebd184..8c1ea5c315ac 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -799,6 +799,17 @@ static bool can_merge_rq(const struct i915_request *prev,
>   	GEM_BUG_ON(prev == next);
>   	GEM_BUG_ON(!assert_priority_queue(prev, next));
>   
> +	/*
> +	 * We not submit known completed requests. Therefore if the next

"We _do_ not submit"?

> +	 * request is already completed, we can pretend to merge it in
> +	 * with the previous context (and we will skip updating the ELSP
> +	 * and tracking). Thus hopefully keeping the ELSP full with active
> +	 * contexts, despite the best efforts of preempt-to-busy to confuse
> +	 * us.
> +	 */
> +	if (i915_request_completed(next))
> +		return true;

It works with the current use of can_merge_rq but leaves a bit of a 
concern for the future. I did not come up with any interesting 
GEM_BUG_ONs to add. I was thinking along the lines of making sure we 
never end up coalescing different contexts to the same port. But no 
ideas how to do that.

> +
>   	if (!can_merge_ctx(prev->hw_context, next->hw_context))
>   		return false;
>   
> @@ -1181,21 +1192,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				continue;
>   			}
>   
> -			if (i915_request_completed(rq)) {
> -				ve->request = NULL;
> -				ve->base.execlists.queue_priority_hint = INT_MIN;
> -				rb_erase_cached(rb, &execlists->virtual);
> -				RB_CLEAR_NODE(rb);
> -
> -				rq->engine = engine;
> -				__i915_request_submit(rq);
> -
> -				spin_unlock(&ve->base.active.lock);
> -
> -				rb = rb_first_cached(&execlists->virtual);
> -				continue;
> -			}
> -
>   			if (last && !can_merge_rq(last, rq)) {
>   				spin_unlock(&ve->base.active.lock);
>   				return; /* leave this for another */
> @@ -1249,11 +1245,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				GEM_BUG_ON(ve->siblings[0] != engine);
>   			}
>   
> -			__i915_request_submit(rq);
> -			if (!i915_request_completed(rq)) {
> +			if (__i915_request_submit(rq)) {
>   				submit = true;
>   				last = rq;
>   			}
> +
> +			/*
> +			 * Hmm, we have a bunch of virtual engine requests,
> +			 * but the first one was already complete (thanks

Complete or completed? Not sure myself..

> +			 * preempt-to-busy!). Keep looking at the veng queue
> +			 * until we have no more relevent requests (i.e.

relevant

> +			 * the normal submit queue has higher priority).
> +			 */
> +			if (!submit) {
> +				spin_unlock(&ve->base.active.lock);
> +				rb = rb_first_cached(&execlists->virtual);
> +				continue;
> +			}
>   		}
>   
>   		spin_unlock(&ve->base.active.lock);
> @@ -1266,8 +1274,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		int i;
>   
>   		priolist_for_each_request_consume(rq, rn, p, i) {
> -			if (i915_request_completed(rq))
> -				goto skip;
> +			bool merge = true;
>   
>   			/*
>   			 * Can we combine this request with the current port?
> @@ -1308,14 +1315,17 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				    ctx_single_port_submission(rq->hw_context))
>   					goto done;
>   
> -				*port = execlists_schedule_in(last, port - execlists->pending);
> -				port++;
> +				merge = false;
>   			}
>   
> -			last = rq;
> -			submit = true;
> -skip:
> -			__i915_request_submit(rq);
> +			if (__i915_request_submit(rq)) {
> +				if (!merge) {
> +					*port = execlists_schedule_in(last, port - execlists->pending);
> +					port++;
> +				}
> +				submit = true;
> +				last = rq;
> +			}
>   		}
>   
>   		rb_erase_cached(&p->node, &execlists->queue);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 9bd8538b1907..0ca43ca15ca0 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
>   	return 0;
>   }
>   
> -void __i915_request_submit(struct i915_request *request)
> +bool __i915_request_submit(struct i915_request *request)
>   {
>   	struct intel_engine_cs *engine = request->engine;
> +	bool result = false;
>   
>   	GEM_TRACE("%s fence %llx:%lld, current %d\n",
>   		  engine->name,
> @@ -389,6 +390,25 @@ void __i915_request_submit(struct i915_request *request)
>   	GEM_BUG_ON(!irqs_disabled());
>   	lockdep_assert_held(&engine->active.lock);
>   
> +	/*
> +	 * With the advent of preempt-to-busy, we frequently encounter
> +	 * requests that we have unsubmitted from HW, but left running
> +	 * until the next ack and have completed in the meantime. On
> +	 * resubmission of that completed request, we can skip
> +	 * updating the payload (and execlists can even skip submitting
> +	 * the request).
> +	 *
> +	 * We must remove the request from the caller's priority queue,
> +	 * and the caller must only call us when the request is in their
> +	 * priority queue, under the active.lock. This ensures that the
> +	 * request has *not* yet been retired and we can safely move
> +	 * the request into the engine->active.list where it will be
> +	 * dropped upon retiring. (Otherwise if resubmit a *retired*
> +	 * request, this would be a horrible use-after-free.)
> +	 */
> +	if (i915_request_completed(request))
> +		goto xfer;
> +
>   	if (i915_gem_context_is_banned(request->gem_context))
>   		i915_request_skip(request, -EIO);
>   
> @@ -412,13 +432,18 @@ void __i915_request_submit(struct i915_request *request)
>   	    i915_sw_fence_signaled(&request->semaphore))
>   		engine->saturated |= request->sched.semaphores;
>   
> -	/* We may be recursing from the signal callback of another i915 fence */
> -	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> +	engine->emit_fini_breadcrumb(request,
> +				     request->ring->vaddr + request->postfix);
> +
> +	trace_i915_request_execute(request);
> +	engine->serial++;
> +	result = true;
>   
> -	list_move_tail(&request->sched.link, &engine->active.requests);
> +xfer:	/* 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));
> -	set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
> +	if (!test_and_set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags))
> +		list_move_tail(&request->sched.link, &engine->active.requests);
>   
>   	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
>   	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags) &&
> @@ -429,12 +454,7 @@ void __i915_request_submit(struct i915_request *request)
>   
>   	spin_unlock(&request->lock);
>   
> -	engine->emit_fini_breadcrumb(request,
> -				     request->ring->vaddr + request->postfix);
> -
> -	engine->serial++;
> -
> -	trace_i915_request_execute(request);
> +	return result;
>   }
>   
>   void i915_request_submit(struct i915_request *request)
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index b18f49528ded..ec5bb4c2e5ae 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -292,7 +292,7 @@ int i915_request_await_execution(struct i915_request *rq,
>   
>   void i915_request_add(struct i915_request *rq);
>   
> -void __i915_request_submit(struct i915_request *request);
> +bool __i915_request_submit(struct i915_request *request);
>   void i915_request_submit(struct i915_request *request);
>   
>   void i915_request_skip(struct i915_request *request, int error);
> 

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: [PATCH v2] drm/i915: Only enqueue already completed requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 11:44:01)
> 
> On 23/09/2019 11:32, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 0a4812ebd184..8c1ea5c315ac 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -799,6 +799,17 @@ static bool can_merge_rq(const struct i915_request *prev,
> >       GEM_BUG_ON(prev == next);
> >       GEM_BUG_ON(!assert_priority_queue(prev, next));
> >   
> > +     /*
> > +      * We not submit known completed requests. Therefore if the next
> 
> "We _do_ not submit"?

Verbs are for the weak.

> > +      * request is already completed, we can pretend to merge it in
> > +      * with the previous context (and we will skip updating the ELSP
> > +      * and tracking). Thus hopefully keeping the ELSP full with active
> > +      * contexts, despite the best efforts of preempt-to-busy to confuse
> > +      * us.
> > +      */
> > +     if (i915_request_completed(next))
> > +             return true;
> 
> It works with the current use of can_merge_rq but leaves a bit of a 
> concern for the future. I did not come up with any interesting 
> GEM_BUG_ONs to add. I was thinking along the lines of making sure we 
> never end up coalescing different contexts to the same port. But no 
> ideas how to do that.

That should just be

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 96dd95d8252c..924cd51e1140 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1322,7 +1322,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
                                if (!merge) {
                                        *port = execlists_schedule_in(last, port - execlists->pending);
                                        port++;
+                                       last = NULL;
                                }
+
+                               GEM_BUG_ON(last &&
+                                          !can_merge_ctx(last->hw_context,
+                                                         rq->hw_context));
+
                                submit = true;
                                last = rq;
                        }

> > @@ -1249,11 +1245,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                               GEM_BUG_ON(ve->siblings[0] != engine);
> >                       }
> >   
> > -                     __i915_request_submit(rq);
> > -                     if (!i915_request_completed(rq)) {
> > +                     if (__i915_request_submit(rq)) {
> >                               submit = true;
> >                               last = rq;
> >                       }
> > +
> > +                     /*
> > +                      * Hmm, we have a bunch of virtual engine requests,
> > +                      * but the first one was already complete (thanks
> 
> Complete or completed? Not sure myself..

Hmm. I suppose you can treat it as complete == "whole", completed ==
"finished". +d for consistency.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Tvrtko Ursulin]

On 21/09/2019 10:55, Chris Wilson wrote:
> As preempt-to-busy leaves the request on the HW as the resubmission is
> processed, that request may complete in the background and even cause a
> second virtual request to enter queue. This second virtual request
> breaks our "single request in the virtual pipeline" assumptions.
> Furthermore, as the virtual request may be completed and retired, we
> lose the reference the virtual engine assumes is held. Normally, just
> removing the request from the scheduler queue removes it from the
> engine, but the virtual engine keeps track of its singleton request via
> its ve->request. This pointer needs protecting with a reference.
> 
> Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 34 ++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 53bc4308793c..1b2bacc60300 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -529,7 +529,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>   				i915_request_cancel_breadcrumb(rq);
>   				spin_unlock(&rq->lock);
>   			}
> -			rq->engine = owner;
>   			owner->submit_request(rq);
>   			active = NULL;
>   		}
> @@ -1248,6 +1247,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   				submit = true;
>   				last = rq;
>   			}
> +			i915_request_put(rq);
>   
>   			if (!submit) {
>   				spin_unlock(&ve->base.active.lock);
> @@ -2535,6 +2535,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>   
>   			rq->engine = engine;
>   			__i915_request_submit(rq);
> +			i915_request_put(rq);
>   
>   			ve->base.execlists.queue_priority_hint = INT_MIN;
>   		}
> @@ -3787,7 +3788,9 @@ static void virtual_submission_tasklet(unsigned long data)
>   
>   static void virtual_submit_request(struct i915_request *rq)
>   {
> -	struct virtual_engine *ve = to_virtual_engine(rq->engine);
> +	struct virtual_engine *ve = to_virtual_engine(rq->hw_context->engine);
> +	struct i915_request *stale;
> +	unsigned long flags;
>   
>   	GEM_TRACE("%s: rq=%llx:%lld\n",
>   		  ve->base.name,
> @@ -3796,15 +3799,30 @@ static void virtual_submit_request(struct i915_request *rq)
>   
>   	GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
>   
> -	GEM_BUG_ON(ve->request);
> -	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> +	spin_lock_irqsave(&ve->base.active.lock, flags);
> +
> +	stale = ve->request;

fetch_and_zero so you don't have to set it to NULL a bit lower?

s/stale/completed/, plus a comment describing preempt-to-busy is to blame?

> +	if (stale) {
> +		GEM_BUG_ON(!i915_request_completed(stale));
> +		__i915_request_submit(stale);
> +		i915_request_put(stale);
> +	}
> +
> +	if (i915_request_completed(rq)) {
> +		__i915_request_submit(rq);
> +		ve->request = NULL;
> +	} else {
> +		ve->base.execlists.queue_priority_hint = rq_prio(rq);
> +		ve->request = i915_request_get(rq);
> +		rq->engine = &ve->base; /* fixup from unwind */

The last line has me confused. Isn't this the normal veng rq submission 
path? In which case eq->engine will already be set to veng. But on the 
unwind path you have removed reset-back of rq->engine to owner. Ah.. the 
unwind calls veng->submit_request on it, and then we end up in here.. 
Okay, this is outside the normal path, I mean the else block has two 
functions/paths, and this should be explained in a comment.

>   
> -	ve->base.execlists.queue_priority_hint = rq_prio(rq);
> -	WRITE_ONCE(ve->request, rq);
> +		GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> +		list_move_tail(&rq->sched.link, virtual_queue(ve));
>   
> -	list_move_tail(&rq->sched.link, virtual_queue(ve));
> +		tasklet_schedule(&ve->base.execlists.tasklet);
> +	}
>   
> -	tasklet_schedule(&ve->base.execlists.tasklet);
> +	spin_unlock_irqrestore(&ve->base.active.lock, flags);
>   }
>   
>   static struct ve_bond *
> 

Otherwise I *think* it's 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

Re: [PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 14:32:23)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > As preempt-to-busy leaves the request on the HW as the resubmission is
> > processed, that request may complete in the background and even cause a
> > second virtual request to enter queue. This second virtual request
> > breaks our "single request in the virtual pipeline" assumptions.
> > Furthermore, as the virtual request may be completed and retired, we
> > lose the reference the virtual engine assumes is held. Normally, just
> > removing the request from the scheduler queue removes it from the
> > engine, but the virtual engine keeps track of its singleton request via
> > its ve->request. This pointer needs protecting with a reference.
> > 
> > Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 34 ++++++++++++++++++++++-------
> >   1 file changed, 26 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 53bc4308793c..1b2bacc60300 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -529,7 +529,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> >                               i915_request_cancel_breadcrumb(rq);
> >                               spin_unlock(&rq->lock);
> >                       }
> > -                     rq->engine = owner;
> >                       owner->submit_request(rq);
> >                       active = NULL;
> >               }
> > @@ -1248,6 +1247,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                               submit = true;
> >                               last = rq;
> >                       }
> > +                     i915_request_put(rq);
> >   
> >                       if (!submit) {
> >                               spin_unlock(&ve->base.active.lock);
> > @@ -2535,6 +2535,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >   
> >                       rq->engine = engine;
> >                       __i915_request_submit(rq);
> > +                     i915_request_put(rq);
> >   
> >                       ve->base.execlists.queue_priority_hint = INT_MIN;
> >               }
> > @@ -3787,7 +3788,9 @@ static void virtual_submission_tasklet(unsigned long data)
> >   
> >   static void virtual_submit_request(struct i915_request *rq)
> >   {
> > -     struct virtual_engine *ve = to_virtual_engine(rq->engine);
> > +     struct virtual_engine *ve = to_virtual_engine(rq->hw_context->engine);
> > +     struct i915_request *stale;
> > +     unsigned long flags;
> >   
> >       GEM_TRACE("%s: rq=%llx:%lld\n",
> >                 ve->base.name,
> > @@ -3796,15 +3799,30 @@ static void virtual_submit_request(struct i915_request *rq)
> >   
> >       GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
> >   
> > -     GEM_BUG_ON(ve->request);
> > -     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> > +     spin_lock_irqsave(&ve->base.active.lock, flags);
> > +
> > +     stale = ve->request;
> 
> fetch_and_zero so you don't have to set it to NULL a bit lower?

I iterated through xchg then fetch_and_zero, before settling on this
variant. My feeling was setting ve->request in both sides of the if()
was more balanced.

> s/stale/completed/, plus a comment describing preempt-to-busy is to blame?

completed is a little long, old? Already added a comment to remind about
the preempt-to-busy link :)

> > +     if (stale) {
> > +             GEM_BUG_ON(!i915_request_completed(stale));
> > +             __i915_request_submit(stale);
> > +             i915_request_put(stale);
> > +     }
> > +
> > +     if (i915_request_completed(rq)) {
> > +             __i915_request_submit(rq);
> > +             ve->request = NULL;
> > +     } else {
> > +             ve->base.execlists.queue_priority_hint = rq_prio(rq);
> > +             ve->request = i915_request_get(rq);
> > +             rq->engine = &ve->base; /* fixup from unwind */
> 
> The last line has me confused. Isn't this the normal veng rq submission 
> path?

It is also on the normal submission path.

> In which case rq->engine will already be set to veng.

Yes

> But on the 
> unwind path you have removed reset-back of rq->engine to owner. Ah.. the 
> unwind calls veng->submit_request on it, and then we end up in here.. 
> Okay, this is outside the normal path, I mean the else block has two 
> functions/paths, and this should be explained in a comment.

That was the intent of "fixup from unwind?"

I can squeeze in /* fixup __unwind_incomplete_requests */ is that more
clueful? Or do you think it needs more?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Chris Wilson]

Quoting Chris Wilson (2019-09-23 14:39:20)
> Quoting Tvrtko Ursulin (2019-09-23 14:32:23)
> > 
> > On 21/09/2019 10:55, Chris Wilson wrote:
> > > +     if (i915_request_completed(rq)) {
> > > +             __i915_request_submit(rq);
> > > +             ve->request = NULL;
> > > +     } else {
> > > +             ve->base.execlists.queue_priority_hint = rq_prio(rq);
> > > +             ve->request = i915_request_get(rq);
> > > +             rq->engine = &ve->base; /* fixup from unwind */
> > 
> > The last line has me confused. Isn't this the normal veng rq submission 
> > path?
> 
> It is also on the normal submission path.
> 
> > In which case rq->engine will already be set to veng.
> 
> Yes
> 
> > But on the 
> > unwind path you have removed reset-back of rq->engine to owner. Ah.. the 
> > unwind calls veng->submit_request on it, and then we end up in here.. 
> > Okay, this is outside the normal path, I mean the else block has two 
> > functions/paths, and this should be explained in a comment.
> 
> That was the intent of "fixup from unwind?"
> 
> I can squeeze in /* fixup __unwind_incomplete_requests */ is that more
> clueful? Or do you think it needs more?

Also it doesn't strictly have to be moved. And certainly there's not
good reason why it needs to be done in this patch -- initially I was
moving it to avoid dereferencing incomplete virtual engine state inside
__i915_request_submit(), but that's better handled by the earlier
patches.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Fixup preempt-to-busy vs reset of a virtual request

[Tvrtko Ursulin]

On 21/09/2019 10:55, Chris Wilson wrote:
> Due to the nature of preempt-to-busy the execlists active tracking and
> the schedule queue may become temporarily desync'ed (between resubmission
> to HW and its ack from HW). This means that we may have unwound a
> request and passed it back to the virtual engine, but it is still
> inflight on the HW and may even result in a GPU hang. If we detect that
> GPU hang and try to reset, the hanging request->engine will no longer
> match the current engine, which means that the request is not on the
> execlists active list and we should not try to find an older incomplete
> request. Given that we have deduced this must be a request on a virtual
> engine, it is the single active request in the context and so must be
> guilty (as the context is still inflight, it is prevented from being
> executed on another engine as we process the reset).
> 
> Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c   | 7 +++++--
>   drivers/gpu/drm/i915/gt/intel_reset.c | 4 +---
>   2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 1b2bacc60300..3eadd294bcd7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2326,11 +2326,14 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
>   
>   static struct i915_request *active_request(struct i915_request *rq)
>   {
> -	const struct list_head * const list =
> -		&i915_request_active_timeline(rq)->requests;
>   	const struct intel_context * const ce = rq->hw_context;
>   	struct i915_request *active = NULL;
> +	struct list_head *list;
>   
> +	if (!i915_request_is_active(rq)) /* unwound, but incomplete! */

Is it time to store the veng pointer separately in the request so we can 
add the assert here? Like 
GEM_BUG_ON(!...engine_is_virtual(rq->orig_engine)) ?

> +		return rq;
> +
> +	list = &i915_request_active_timeline(rq)->requests;
>   	list_for_each_entry_from_reverse(rq, list, link) {
>   		if (i915_request_completed(rq))
>   			break;
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 89048ca8924c..ae68c3786f63 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -42,11 +42,10 @@ static void engine_skip_context(struct i915_request *rq)
>   	struct intel_engine_cs *engine = rq->engine;
>   	struct i915_gem_context *hung_ctx = rq->gem_context;
>   
> -	lockdep_assert_held(&engine->active.lock);
> -
>   	if (!i915_request_is_active(rq))
>   		return;
>   
> +	lockdep_assert_held(&engine->active.lock);
>   	list_for_each_entry_continue(rq, &engine->active.requests, sched.link)
>   		if (rq->gem_context == hung_ctx)
>   			i915_request_skip(rq, -EIO);
> @@ -123,7 +122,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>   		  rq->fence.seqno,
>   		  yesno(guilty));
>   
> -	lockdep_assert_held(&rq->engine->active.lock);
>   	GEM_BUG_ON(i915_request_completed(rq));
>   
>   	if (guilty) {
> 

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: [PATCH 5/5] drm/i915: Prevent bonded requests from overtaking each other on preemption

[Tvrtko Ursulin]

On 21/09/2019 10:55, Chris Wilson wrote:
> Force bonded requests to run on distinct engines so that they cannot be
> shuffled onto the same engine where timeslicing will reverse the order.
> A bonded request will often wait on a semaphore signaled by its master,
> creating an implicit dependency -- if we ignore that implicit dependency
> and allow the bonded request to run on the same engine and before its
> master, we will cause a GPU hang. [Whether it will hang the GPU is
> debatable, we should keep on timeslicing and each timeslice should be
> "accidentally" counted as forward progress, in which case it should run
> but at one-half to one-third speed.]
> 
> We can prevent this inversion by restricting which engines we allow
> ourselves to jump to upon preemption, i.e. baking in the arrangement
> established at first execution. (We should also consider capturing the
> implicit dependency using i915_sched_add_dependency(), but first we need
> to think about the constraints that requires on the execution/retirement
> ordering.)
> 
> Fixes: 8ee36e048c98 ("drm/i915/execlists: Minimalistic timeslicing")
> References: ee1136908e9b ("drm/i915/execlists: Virtual engine bonding")
> Testcase: igt/gem_exec_balancer/bonded-slice
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 3eadd294bcd7..9872bb4c99fc 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -3846,18 +3846,22 @@ static void
>   virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
>   {
>   	struct virtual_engine *ve = to_virtual_engine(rq->engine);
> +	intel_engine_mask_t allowed, exec;
>   	struct ve_bond *bond;
>   
> +	allowed = ~to_request(signal)->engine->mask;
> +
>   	bond = virtual_find_bond(ve, to_request(signal)->engine);
> -	if (bond) {
> -		intel_engine_mask_t old, new, cmp;
> +	if (bond)
> +		allowed &= bond->sibling_mask;
>   
> -		cmp = READ_ONCE(rq->execution_mask);
> -		do {
> -			old = cmp;
> -			new = cmp & bond->sibling_mask;
> -		} while ((cmp = cmpxchg(&rq->execution_mask, old, new)) != old);
> -	}
> +	/* Restrict the bonded request to run on only the available engines */
> +	exec = READ_ONCE(rq->execution_mask);
> +	while (!try_cmpxchg(&rq->execution_mask, &exec, exec & allowed))
> +		;
> +
> +	/* Prevent the master from being re-run on the bonded engines */
> +	to_request(signal)->execution_mask &= ~allowed;

I had an open here whether we should also fix the slave. At re-submit 
time what is preventing the slave being put on the same engine as the 
master?

Regards,

Tvrtko

>   }
>   
>   struct intel_context *
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Prevent bonded requests from overtaking each other on preemption

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 14:48:44)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > Force bonded requests to run on distinct engines so that they cannot be
> > shuffled onto the same engine where timeslicing will reverse the order.
> > A bonded request will often wait on a semaphore signaled by its master,
> > creating an implicit dependency -- if we ignore that implicit dependency
> > and allow the bonded request to run on the same engine and before its
> > master, we will cause a GPU hang. [Whether it will hang the GPU is
> > debatable, we should keep on timeslicing and each timeslice should be
> > "accidentally" counted as forward progress, in which case it should run
> > but at one-half to one-third speed.]
> > 
> > We can prevent this inversion by restricting which engines we allow
> > ourselves to jump to upon preemption, i.e. baking in the arrangement
> > established at first execution. (We should also consider capturing the
> > implicit dependency using i915_sched_add_dependency(), but first we need
> > to think about the constraints that requires on the execution/retirement
> > ordering.)
> > 
> > Fixes: 8ee36e048c98 ("drm/i915/execlists: Minimalistic timeslicing")
> > References: ee1136908e9b ("drm/i915/execlists: Virtual engine bonding")
> > Testcase: igt/gem_exec_balancer/bonded-slice
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 20 ++++++++++++--------
> >   1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 3eadd294bcd7..9872bb4c99fc 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -3846,18 +3846,22 @@ static void
> >   virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
> >   {
> >       struct virtual_engine *ve = to_virtual_engine(rq->engine);
> > +     intel_engine_mask_t allowed, exec;
> >       struct ve_bond *bond;
> >   
> > +     allowed = ~to_request(signal)->engine->mask;
> > +
> >       bond = virtual_find_bond(ve, to_request(signal)->engine);
> > -     if (bond) {
> > -             intel_engine_mask_t old, new, cmp;
> > +     if (bond)
> > +             allowed &= bond->sibling_mask;
> >   
> > -             cmp = READ_ONCE(rq->execution_mask);
> > -             do {
> > -                     old = cmp;
> > -                     new = cmp & bond->sibling_mask;
> > -             } while ((cmp = cmpxchg(&rq->execution_mask, old, new)) != old);
> > -     }
> > +     /* Restrict the bonded request to run on only the available engines */
> > +     exec = READ_ONCE(rq->execution_mask);
> > +     while (!try_cmpxchg(&rq->execution_mask, &exec, exec & allowed))
> > +             ;
> > +
> > +     /* Prevent the master from being re-run on the bonded engines */
> > +     to_request(signal)->execution_mask &= ~allowed;
> 
> I had an open here whether we should also fix the slave. At re-submit 
> time what is preventing the slave being put on the same engine as the 
> master?

We removed the master engine from the list of allowed engines for the
slave, and we never reset the execution_mask on unsubmit (and we don't
re-execute the bonding on resubmit). That's the hard bit I left open for
the reader.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Fixup preempt-to-busy vs reset of a virtual request

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 14:46:27)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > Due to the nature of preempt-to-busy the execlists active tracking and
> > the schedule queue may become temporarily desync'ed (between resubmission
> > to HW and its ack from HW). This means that we may have unwound a
> > request and passed it back to the virtual engine, but it is still
> > inflight on the HW and may even result in a GPU hang. If we detect that
> > GPU hang and try to reset, the hanging request->engine will no longer
> > match the current engine, which means that the request is not on the
> > execlists active list and we should not try to find an older incomplete
> > request. Given that we have deduced this must be a request on a virtual
> > engine, it is the single active request in the context and so must be
> > guilty (as the context is still inflight, it is prevented from being
> > executed on another engine as we process the reset).
> > 
> > Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c   | 7 +++++--
> >   drivers/gpu/drm/i915/gt/intel_reset.c | 4 +---
> >   2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 1b2bacc60300..3eadd294bcd7 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -2326,11 +2326,14 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
> >   
> >   static struct i915_request *active_request(struct i915_request *rq)
> >   {
> > -     const struct list_head * const list =
> > -             &i915_request_active_timeline(rq)->requests;
> >       const struct intel_context * const ce = rq->hw_context;
> >       struct i915_request *active = NULL;
> > +     struct list_head *list;
> >   
> > +     if (!i915_request_is_active(rq)) /* unwound, but incomplete! */
> 
> Is it time to store the veng pointer separately in the request so we can 
> add the assert here? Like 
> GEM_BUG_ON(!...engine_is_virtual(rq->orig_engine)) ?

intel_engine_is_virtual(rq->hw_context->engine).

But this is currently agnostic, and here we are only asking question is
this request in the engine->active.list, which applies to both normal
and virtual setups. Hmm, this bug does apply to normal reset, I only
spotted it because we added the lockdep warnings that picked up that
rq->engine was different.

There maybe some use for that assertion, but I don't think this is one.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Tvrtko Ursulin]

On 23/09/2019 14:39, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-23 14:32:23)
>>
>> On 21/09/2019 10:55, Chris Wilson wrote:
>>> As preempt-to-busy leaves the request on the HW as the resubmission is
>>> processed, that request may complete in the background and even cause a
>>> second virtual request to enter queue. This second virtual request
>>> breaks our "single request in the virtual pipeline" assumptions.
>>> Furthermore, as the virtual request may be completed and retired, we
>>> lose the reference the virtual engine assumes is held. Normally, just
>>> removing the request from the scheduler queue removes it from the
>>> engine, but the virtual engine keeps track of its singleton request via
>>> its ve->request. This pointer needs protecting with a reference.
>>>
>>> Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c | 34 ++++++++++++++++++++++-------
>>>    1 file changed, 26 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 53bc4308793c..1b2bacc60300 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -529,7 +529,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>>>                                i915_request_cancel_breadcrumb(rq);
>>>                                spin_unlock(&rq->lock);
>>>                        }
>>> -                     rq->engine = owner;
>>>                        owner->submit_request(rq);
>>>                        active = NULL;
>>>                }
>>> @@ -1248,6 +1247,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>>                                submit = true;
>>>                                last = rq;
>>>                        }
>>> +                     i915_request_put(rq);
>>>    
>>>                        if (!submit) {
>>>                                spin_unlock(&ve->base.active.lock);
>>> @@ -2535,6 +2535,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>>    
>>>                        rq->engine = engine;
>>>                        __i915_request_submit(rq);
>>> +                     i915_request_put(rq);
>>>    
>>>                        ve->base.execlists.queue_priority_hint = INT_MIN;
>>>                }
>>> @@ -3787,7 +3788,9 @@ static void virtual_submission_tasklet(unsigned long data)
>>>    
>>>    static void virtual_submit_request(struct i915_request *rq)
>>>    {
>>> -     struct virtual_engine *ve = to_virtual_engine(rq->engine);
>>> +     struct virtual_engine *ve = to_virtual_engine(rq->hw_context->engine);
>>> +     struct i915_request *stale;
>>> +     unsigned long flags;
>>>    
>>>        GEM_TRACE("%s: rq=%llx:%lld\n",
>>>                  ve->base.name,
>>> @@ -3796,15 +3799,30 @@ static void virtual_submit_request(struct i915_request *rq)
>>>    
>>>        GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
>>>    
>>> -     GEM_BUG_ON(ve->request);
>>> -     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
>>> +     spin_lock_irqsave(&ve->base.active.lock, flags);
>>> +
>>> +     stale = ve->request;
>>
>> fetch_and_zero so you don't have to set it to NULL a bit lower?
> 
> I iterated through xchg then fetch_and_zero, before settling on this
> variant. My feeling was setting ve->request in both sides of the if()
> was more balanced.

Ok.

> 
>> s/stale/completed/, plus a comment describing preempt-to-busy is to blame?
> 
> completed is a little long, old? Already added a comment to remind about
> the preempt-to-busy link :)

I disliked stale since it has negative connotations and this is actually 
normal/expected situation (albeit rare), but maybe it's just me. Old is 
perhaps better. But you can also keep stale I guess.

>>> +     if (stale) {
>>> +             GEM_BUG_ON(!i915_request_completed(stale));
>>> +             __i915_request_submit(stale);
>>> +             i915_request_put(stale);
>>> +     }
>>> +
>>> +     if (i915_request_completed(rq)) {
>>> +             __i915_request_submit(rq);
>>> +             ve->request = NULL;
>>> +     } else {
>>> +             ve->base.execlists.queue_priority_hint = rq_prio(rq);
>>> +             ve->request = i915_request_get(rq);
>>> +             rq->engine = &ve->base; /* fixup from unwind */
>>
>> The last line has me confused. Isn't this the normal veng rq submission
>> path?
> 
> It is also on the normal submission path.
> 
>> In which case rq->engine will already be set to veng.
> 
> Yes
> 
>> But on the
>> unwind path you have removed reset-back of rq->engine to owner. Ah.. the
>> unwind calls veng->submit_request on it, and then we end up in here..
>> Okay, this is outside the normal path, I mean the else block has two
>> functions/paths, and this should be explained in a comment.
> 
> That was the intent of "fixup from unwind?"
> 
> I can squeeze in /* fixup __unwind_incomplete_requests */ is that more
> clueful? Or do you think it needs more?

I thought it does, but then I did not know what I would write so it's 
useful. So I think it's fine after all.

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

Regards,

Tvrtko

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

✗ Fi.CI.BUILD: failure for series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/execlists: Refactor -EIO markup of hung requests (rev3)
URL   : https://patchwork.freedesktop.org/series/67033/
State : failure

== Summary ==

Applying: drm/i915/execlists: Refactor -EIO markup of hung requests
Applying: drm/i915: Only enqueue already completed requests
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/gt/intel_lrc.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 drm/i915: Only enqueue already completed requests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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