All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
@ 2017-10-23 21:32 Chris Wilson
  2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-23 21:32 UTC (permalink / raw)
  To: intel-gfx

In the idle worker we drop the prolonged GT wakeref used to cover such
essentials as interrupt delivery. (When a CS interrupt arrives, we also
assert that the GT is awake.) However, it turns out that 10ms is not
long enough to be assured that the last CS interrupt has been delivered,
so bump that to 200ms, and move the entirety of that wait to before we
take the struct_mutex to avoid blocking. As this is now a potentially
long wait, restore the earlier behaviour of bailing out early when a new
request arrives.

v2: Break out the repeated check for new requests into its own little
helper to try and improve the self-commentary.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 026cb52ece0b..bb0e85043e01 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3276,13 +3276,20 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	}
 }
 
+static inline bool
+new_requests_since_last_retire(const struct drm_i915_private *i915)
+{
+	return (READ_ONCE(i915->gt.active_requests) ||
+		work_pending(&i915->gt.idle_work.work));
+}
+
 static void
 i915_gem_idle_work_handler(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), gt.idle_work.work);
-	struct drm_device *dev = &dev_priv->drm;
 	bool rearm_hangcheck;
+	ktime_t end;
 
 	if (!READ_ONCE(dev_priv->gt.awake))
 		return;
@@ -3291,14 +3298,21 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	 * Wait for last execlists context complete, but bail out in case a
 	 * new request is submitted.
 	 */
-	wait_for(intel_engines_are_idle(dev_priv), 10);
-	if (READ_ONCE(dev_priv->gt.active_requests))
-		return;
+	end = ktime_add_ms(ktime_get(), 200);
+	do {
+		if (new_requests_since_last_retire(dev_priv))
+			return;
+
+		if (intel_engines_are_idle(dev_priv))
+			break;
+
+		usleep_range(100, 500);
+	} while (ktime_before(ktime_get(), end));
 
 	rearm_hangcheck =
 		cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 
-	if (!mutex_trylock(&dev->struct_mutex)) {
+	if (!mutex_trylock(&dev_priv->drm.struct_mutex)) {
 		/* Currently busy, come back later */
 		mod_delayed_work(dev_priv->wq,
 				 &dev_priv->gt.idle_work,
@@ -3310,13 +3324,14 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	 * New request retired after this work handler started, extend active
 	 * period until next instance of the work.
 	 */
-	if (work_pending(work))
-		goto out_unlock;
-
-	if (dev_priv->gt.active_requests)
+	if (new_requests_since_last_retire(dev_priv))
 		goto out_unlock;
 
-	if (wait_for(intel_engines_are_idle(dev_priv), 10))
+	/*
+	 * We are committed now to parking the engines, make sure there
+	 * will be no more interrupts arriving later.
+	 */
+	if (!intel_engines_are_idle(dev_priv))
 		DRM_ERROR("Timeout waiting for engines to idle\n");
 
 	intel_engines_mark_idle(dev_priv);
@@ -3330,7 +3345,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
 		gen6_rps_idle(dev_priv);
 	intel_runtime_pm_put(dev_priv);
 out_unlock:
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
 
 out_rearm:
 	if (rearm_hangcheck) {
-- 
2.15.0.rc1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
@ 2017-10-23 21:32 ` Chris Wilson
  2017-10-24  8:01   ` Chris Wilson
  2017-10-24  9:05   ` Chris Wilson
  2017-10-23 21:32 ` [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-23 21:32 UTC (permalink / raw)
  To: intel-gfx

When we park the engine (upon idling), we kill the irq tasklet. However,
to be sure that it is not restarted by a final interrupt after doing so,
flush the interrupt handler before parking. As we only park the engines
when we believe the system is idle, there should not be any spurious
interrupts later to distrub us; so flushing the final in-flight
interrupt should be sufficient.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bb0e85043e01..b547a6327d34 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	if (new_requests_since_last_retire(dev_priv))
 		goto out_unlock;
 
+	/*
+	 * Be paranoid and flush a concurrent interrupt to make sure
+	 * we don't reactivate any irq tasklets after parking.
+	 */
+	synchronize_irq(dev_priv->drm.irq);
+
 	/*
 	 * We are committed now to parking the engines, make sure there
 	 * will be no more interrupts arriving later.
-- 
2.15.0.rc1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
  2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
@ 2017-10-23 21:32 ` Chris Wilson
  2017-10-24 14:40   ` Mika Kuoppala
  2017-10-23 21:32 ` [PATCH 4/4] warn Chris Wilson
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2017-10-23 21:32 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
context-switch when idle") we noticed the presence of late
context-switch interrupts. We were able to filter those out by looking
at whether the ELSP remained active, but in commit beecec901790
("drm/i915/execlists: Preemption!") that became problematic as we now
anticipate receiving a context-switch event for preemption while ELSP
may be empty. To restore the spurious interrupt suppression, add a
counter for the expected number of pending context-switches and skip if
we do not need to handle this interrupt to make forward progress.

v2: Don't forget to switch on for preempt.
v3: Reduce the counter to a on/off boolean tracker. Declare the HW as
active when we first submit, and idle after the final completion event
(with which we confirm the HW says it is idle), and track each source
of activity separately. With a finite number of sources, it should us
debug which gets stuck.

Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
 drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.c           | 27 ++++++++++++++++++------
 drivers/gpu/drm/i915/intel_ringbuffer.h    | 34 ++++++++++++++++++++++++++++--
 5 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114b739d..f84c267728fd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 	execlists->first = rb;
 	if (submit) {
 		port_assign(port, last);
+		execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
 		i915_guc_submit(engine);
 	}
 	spin_unlock_irq(&engine->timeline->lock);
@@ -633,6 +634,8 @@ static void i915_guc_irq_handler(unsigned long data)
 
 		rq = port_request(&port[0]);
 	}
+	if (!rq)
+		execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
 
 	if (!port_isset(last_port))
 		i915_guc_dequeue(engine);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1296a55c1e4..f8205841868b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 	bool tasklet = false;
 
 	if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
-		__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-		tasklet = true;
+		if (READ_ONCE(engine->execlists.active)) {
+			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
+			tasklet = true;
+		}
 	}
 
 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a47a9c6bea52..ab5bf4e2e28e 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
 	if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
 		return false;
 
-	/* Both ports drained, no more ELSP submission? */
-	if (port_request(&engine->execlists.port[0]))
+	/* Waiting to drain ELSP? */
+	if (READ_ONCE(engine->execlists.active))
 		return false;
 
 	/* ELSP is empty, but there are ready requests? */
@@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
 					   idx);
 			}
 		}
+		drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
 		rcu_read_unlock();
 	} else if (INTEL_GEN(dev_priv) > 6) {
 		drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f45dd7dc3e5..233c992a886b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -575,7 +575,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			 * the state of the GPU is known (idle).
 			 */
 			inject_preempt_context(engine);
-			execlists->preempt = true;
+			execlists_set_active(execlists,
+					     EXECLISTS_ACTIVE_PREEMPT);
 			goto unlock;
 		} else {
 			/*
@@ -683,8 +684,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 unlock:
 	spin_unlock_irq(&engine->timeline->lock);
 
-	if (submit)
+	if (submit) {
+		execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
 		execlists_submit_ports(engine);
+	}
 }
 
 static void
@@ -696,6 +699,7 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
 	while (num_ports-- && port_isset(port)) {
 		struct drm_i915_gem_request *rq = port_request(port);
 
+		GEM_BUG_ON(!execlists->active);
 		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
 		i915_gem_request_put(rq);
 
@@ -861,15 +865,21 @@ static void intel_lrc_irq_handler(unsigned long data)
 				unwind_incomplete_requests(engine);
 				spin_unlock_irq(&engine->timeline->lock);
 
-				GEM_BUG_ON(!execlists->preempt);
-				execlists->preempt = false;
+				GEM_BUG_ON(!execlists_is_active(execlists,
+								EXECLISTS_ACTIVE_PREEMPT));
+				execlists_clear_active(execlists,
+						       EXECLISTS_ACTIVE_PREEMPT);
 				continue;
 			}
 
 			if (status & GEN8_CTX_STATUS_PREEMPTED &&
-			    execlists->preempt)
+			    execlists_is_active(execlists,
+					       EXECLISTS_ACTIVE_PREEMPT))
 				continue;
 
+			GEM_BUG_ON(!execlists_is_active(execlists,
+							EXECLISTS_ACTIVE_USER));
+
 			/* Check the context/desc id for this event matches */
 			GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
 
@@ -892,6 +902,9 @@ static void intel_lrc_irq_handler(unsigned long data)
 			/* After the final element, the hw should be idle */
 			GEM_BUG_ON(port_count(port) == 0 &&
 				   !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
+			if (port_count(port) == 0)
+				execlists_clear_active(execlists,
+						       EXECLISTS_ACTIVE_USER);
 		}
 
 		if (head != execlists->csb_head) {
@@ -901,7 +914,7 @@ static void intel_lrc_irq_handler(unsigned long data)
 		}
 	}
 
-	if (!execlists->preempt)
+	if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
 		execlists_dequeue(engine);
 
 	intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
@@ -1460,7 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 		   GT_CONTEXT_SWITCH_INTERRUPT << engine->irq_shift);
 	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 	execlists->csb_head = -1;
-	execlists->preempt = false;
+	execlists->active = 0;
 
 	/* After a GPU reset, we may have requests to replay */
 	if (!i915_modparams.enable_guc_submission && execlists->first)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 17186f067408..51bc704bf413 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -241,9 +241,17 @@ struct intel_engine_execlists {
 	} port[EXECLIST_MAX_PORTS];
 
 	/**
-	 * @preempt: are we currently handling a preempting context switch?
+	 * @active: is the HW active? We consider the HW as active after
+	 * submitted any context for execution until we have seen the last
+	 * context completion event. After that, we do not expect any more
+	 * events until we submit, and so can park the HW.
+	 *
+	 * As we have a small number of different sources from which we feed
+	 * the HW, we track the state of each inside a single bitfield.
 	 */
-	bool preempt;
+	unsigned int active;
+#define EXECLISTS_ACTIVE_USER 0
+#define EXECLISTS_ACTIVE_PREEMPT 1
 
 	/**
 	 * @port_mask: number of execlist ports - 1
@@ -525,6 +533,27 @@ struct intel_engine_cs {
 	u32 (*get_cmd_length_mask)(u32 cmd_header);
 };
 
+static inline void
+execlists_set_active(struct intel_engine_execlists *execlists,
+		     unsigned int bit)
+{
+	__set_bit(bit, (unsigned long *)&execlists->active);
+}
+
+static inline void
+execlists_clear_active(struct intel_engine_execlists *execlists,
+		       unsigned int bit)
+{
+	__clear_bit(bit, (unsigned long *)&execlists->active);
+}
+
+static inline bool
+execlists_is_active(const struct intel_engine_execlists *execlists,
+		    unsigned int bit)
+{
+	return test_bit(bit, (unsigned long *)&execlists->active);
+}
+
 static inline unsigned int
 execlists_num_ports(const struct intel_engine_execlists * const execlists)
 {
@@ -538,6 +567,7 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
 	const unsigned int m = execlists->port_mask;
 
 	GEM_BUG_ON(port_index(port, execlists) != 0);
+	GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
 
 	memmove(port, port + 1, m * sizeof(struct execlist_port));
 	memset(port + m, 0, sizeof(struct execlist_port));
-- 
2.15.0.rc1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] warn
  2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
  2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
  2017-10-23 21:32 ` [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
@ 2017-10-23 21:32 ` Chris Wilson
  2017-10-24 14:46   ` Mika Kuoppala
  2017-10-23 22:04 ` ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
  2017-10-23 23:15 ` ✓ Fi.CI.IGT: " Patchwork
  4 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2017-10-23 21:32 UTC (permalink / raw)
  To: intel-gfx

---
 drivers/gpu/drm/i915/i915_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f8205841868b..e4bd20ce031d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1391,6 +1391,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 		if (READ_ONCE(engine->execlists.active)) {
 			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 			tasklet = true;
+		} else if (WARN_ON(!engine->i915->gt.awake)) {
+			struct drm_printer p = drm_info_printer(engine->i915->drm.dev);
+			intel_engine_dump(engine, &p);
 		}
 	}
 
-- 
2.15.0.rc1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
  2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
                   ` (2 preceding siblings ...)
  2017-10-23 21:32 ` [PATCH 4/4] warn Chris Wilson
@ 2017-10-23 22:04 ` Patchwork
  2017-10-23 23:15 ` ✓ Fi.CI.IGT: " Patchwork
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2017-10-23 22:04 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
URL   : https://patchwork.freedesktop.org/series/32493/
State : success

== Summary ==

Series 32493v1 series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
https://patchwork.freedesktop.org/api/1.0/series/32493/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705
Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> INCOMPLETE (fi-cfl-s) fdo#103206

fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fdo#103206 https://bugs.freedesktop.org/show_bug.cgi?id=103206

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:438s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:450s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:373s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:532s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:503s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:494s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:495s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:487s
fi-cfl-s         total:287  pass:253  dwarn:2   dfail:0   fail:0   skip:31 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:245s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:579s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:448s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:427s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:432s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:493s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:458s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:572s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:583s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:541s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:644s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:457s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:558s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:414s

5c82a37eff83ab4e60e760fbaf03db5ba0563497 drm-tip: 2017y-10m-23d-18h-06m-28s UTC integration manifest
c8aac7abc28d warn
a00f168756fa drm/i915: Filter out spurious execlists context-switch interrupts
0590b88c7a50 drm/i915: Synchronize irq before parking each engine
ae84ddb5f695 drm/i915: Bump wait-times for the final CS interrupt before parking

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
  2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
                   ` (3 preceding siblings ...)
  2017-10-23 22:04 ` ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
@ 2017-10-23 23:15 ` Patchwork
  4 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2017-10-23 23:15 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking
URL   : https://patchwork.freedesktop.org/series/32493/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (shard-hsw) fdo#102707
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
                dmesg-warn -> PASS       (shard-hsw) fdo#102249 +1

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249

shard-hsw        total:2540 pass:1433 dwarn:2   dfail:0   fail:8   skip:1097 time:9202s

== Logs ==

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
@ 2017-10-24  8:01   ` Chris Wilson
  2017-10-24  9:05   ` Chris Wilson
  1 sibling, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-24  8:01 UTC (permalink / raw)
  To: intel-gfx

Quoting Chris Wilson (2017-10-23 22:32:35)
> When we park the engine (upon idling), we kill the irq tasklet. However,
> to be sure that it is not restarted by a final interrupt after doing so,
> flush the interrupt handler before parking. As we only park the engines
> when we believe the system is idle, there should not be any spurious
> interrupts later to distrub us; so flushing the final in-flight
> interrupt should be sufficient.

And even this is not enough for some mischievous hw:

<4>[  329.637536] WARN_ON(!engine->i915->gt.awake)
<4>[  329.637551] ------------[ cut here ]------------
<4>[  329.637569] WARNING: CPU: 3 PID: 74 at drivers/gpu/drm/i915/i915_irq.c:1394 gen8_cs_irq_handler+0x7c/0xe0 [i915]
<4>[  329.637571] Modules linked in: vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 snd_hda_intel snd_hda_codec snd_hwdep x86_pkg_temp_thermal intel_powerclamp snd_hda_core coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel e1000e snd_pcm ptp pps_core mei_me prime_numbers mei pinctrl_sunrisepoint pinctrl_intel i2c_hid
<4>[  329.637605] CPU: 3 PID: 74 Comm: kworker/u8:1 Tainted: G     U          4.14.0-rc6-CI-Patchwork_6149+ #1
<4>[  329.637606] Hardware name:                  /NUC7i5BNB, BIOS BNKBL357.86A.0048.2017.0704.1415 07/04/2017
<4>[  329.637624] Workqueue: i915 i915_gem_idle_work_handler [i915]
<4>[  329.637627] task: ffff880272ff2880 task.stack: ffffc9000063c000
<4>[  329.637640] RIP: 0010:gen8_cs_irq_handler+0x7c/0xe0 [i915]
<4>[  329.637642] RSP: 0018:ffff88027ed83e30 EFLAGS: 00010086
<4>[  329.637644] RAX: 0000000000000020 RBX: ffff8802698a8008 RCX: 0000000000010002
<4>[  329.637645] RDX: 0000000000000000 RSI: ffffffff81d0dbdc RDI: ffffffff81cc1b56
<4>[  329.637647] RBP: ffff88027ed83e60 R08: 0000000000000000 R09: 0000000000000001
<4>[  329.637648] R10: 00000000d8873c29 R11: 00000000148e3c06 R12: 0000000000000100
<4>[  329.637650] R13: ffff880269240000 R14: 0000000000000092 R15: ffff88026a6a6918
<4>[  329.637651] FS:  0000000000000000(0000) GS:ffff88027ed80000(0000) knlGS:0000000000000000
<4>[  329.637653] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[  329.637654] CR2: 00007f099468d198 CR3: 0000000003e0f002 CR4: 00000000003606e0
<4>[  329.637655] Call Trace:
<4>[  329.637657]  <IRQ>
<4>[  329.637672]  gen8_gt_irq_handler+0x102/0x120 [i915]
<4>[  329.637685]  gen8_irq_handler+0x90/0x670 [i915]
<4>[  329.637690]  __handle_irq_event_percpu+0x49/0x350
<4>[  329.637694]  handle_irq_event_percpu+0x23/0x60
<4>[  329.637696]  handle_irq_event+0x39/0x60
<4>[  329.637699]  handle_edge_irq+0xf4/0x1c0
<4>[  329.637702]  handle_irq+0x1a/0x30
<4>[  329.637704]  do_IRQ+0x68/0x130
<4>[  329.637707]  common_interrupt+0x9a/0x9a
<4>[  329.637709]  </IRQ>
<4>[  329.637711] RIP: 0010:_raw_spin_unlock_irq+0x32/0x50
<4>[  329.637712] RSP: 0018:ffffc9000063fda8 EFLAGS: 00000206 ORIG_RAX: ffffffffffffff6d
<4>[  329.637715] RAX: ffff880272ff2880 RBX: ffff8802692443e0 RCX: 0000000000000006
<4>[  329.637716] RDX: 0000000000001379 RSI: ffffffff81d0dbdc RDI: 0000000000000001
<4>[  329.637718] RBP: ffffc9000063fdb0 R08: ffff880272ff3190 R09: 0000000000000000
<4>[  329.637719] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8802692443e0
<4>[  329.637720] R13: ffff880269240070 R14: ffff880269247358 R15: 0000000081e40e00
<4>[  329.637726]  ? _raw_spin_unlock_irq+0x2c/0x50
<4>[  329.637739]  gen6_disable_rps_interrupts+0x62/0x90 [i915]
<4>[  329.637753]  gen6_rps_idle+0x1d/0xe0 [i915]
<4>[  329.637770]  i915_gem_idle_work_handler+0x188/0x190 [i915]
<4>[  329.637773]  process_one_work+0x221/0x650
<4>[  329.637777]  worker_thread+0x1db/0x3b0
<4>[  329.637781]  kthread+0x114/0x150
<4>[  329.637783]  ? process_one_work+0x650/0x650
<4>[  329.637785]  ? kthread_create_on_node+0x40/0x40
<4>[  329.637788]  ret_from_fork+0x27/0x40
<4>[  329.637792] Code: 48 83 c4 20 5b 41 5c 5d c3 48 8b 07 80 b8 cc 81 00 00 00 75 42 48 c7 c6 f0 27 29 a0 48 c7 c7 3e 48 28 a0 89 55 d4 e8 b5 ca f9 e0 <0f> ff 48 8b 03 48 8d 75 d8 48 89 df 48 c7 45 d8 e0 78 60 81 48 
<4>[  329.637867] ---[ end trace e6209c9962196e53 ]---
<6>[  329.637870] i915 0000:00:02.0: [drm] rcs0
<6>[  329.637872] i915 0000:00:02.0: [drm] 	current seqno 88f5, last 88f5, hangcheck 0 [29638 ms], inflight 0
<6>[  329.637874] i915 0000:00:02.0: [drm] 	Reset count: 2
<6>[  329.637876] i915 0000:00:02.0: [drm] 	Requests:
<6>[  329.637887] i915 0000:00:02.0: [drm] 	RING_START: 0x0000f000 [0x00000000]
<6>[  329.637889] i915 0000:00:02.0: [drm] 	RING_HEAD:  0x00000c10 [0x00000000]
<6>[  329.637891] i915 0000:00:02.0: [drm] 	RING_TAIL:  0x00000c10 [0x00000000]
<6>[  329.637894] i915 0000:00:02.0: [drm] 	RING_CTL:   0x00003000 []
<6>[  329.637897] i915 0000:00:02.0: [drm] 	ACTHD:  0x00000000_00000c10
<6>[  329.637900] i915 0000:00:02.0: [drm] 	BBADDR: 0x00000000_00000004
<6>[  329.637902] i915 0000:00:02.0: [drm] 	Execlist status: 0x00000301 00000000
<6>[  329.637904] i915 0000:00:02.0: [drm] 	Execlist CSB read 4 [4 cached], write 4 [4 from hws], interrupt posted? no
<6>[  329.637906] i915 0000:00:02.0: [drm] 		ELSP[0] idle
<6>[  329.637908] i915 0000:00:02.0: [drm] 		ELSP[1] idle
<6>[  329.637909] i915 0000:00:02.0: [drm] 		HW active? 0x0

So if we want to completely eradicate that last unwanted interrupt, it
looks like IER/IMR time. However, that can wait until we have 
intel_engine_park/unpark (the spurious interrupt filter is good enough
for now).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
  2017-10-24  8:01   ` Chris Wilson
@ 2017-10-24  9:05   ` Chris Wilson
  2017-10-24  9:19     ` Mika Kuoppala
  2017-10-24 14:26     ` Mika Kuoppala
  1 sibling, 2 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-24  9:05 UTC (permalink / raw)
  To: intel-gfx

Quoting Chris Wilson (2017-10-23 22:32:35)
> When we park the engine (upon idling), we kill the irq tasklet. However,
> to be sure that it is not restarted by a final interrupt after doing so,
> flush the interrupt handler before parking. As we only park the engines
> when we believe the system is idle, there should not be any spurious
> interrupts later to distrub us; so flushing the final in-flight
> interrupt should be sufficient.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index bb0e85043e01..b547a6327d34 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
>         if (new_requests_since_last_retire(dev_priv))
>                 goto out_unlock;
>  
> +       /*
> +        * Be paranoid and flush a concurrent interrupt to make sure
> +        * we don't reactivate any irq tasklets after parking.

* FIXME: Note that even though we have waited for execlists to be idle,
* there may still be an in-flight interrupt even though the CSB
* is now empty. synchronize_irq() makes sure that the residual
* interrupt is completed before we continue, but it doesn't prevent
* the HW from raising that residual interrupt later. To complete
* the shield we should coordinate disabling the CS irq with
* flushing the residual interrupt.

> +        */
> +       synchronize_irq(dev_priv->drm.irq);
> +
>         /*
>          * We are committed now to parking the engines, make sure there
>          * will be no more interrupts arriving later.
> -- 
> 2.15.0.rc1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-24  9:05   ` Chris Wilson
@ 2017-10-24  9:19     ` Mika Kuoppala
  2017-10-24  9:32       ` Chris Wilson
  2017-10-24 14:26     ` Mika Kuoppala
  1 sibling, 1 reply; 14+ messages in thread
From: Mika Kuoppala @ 2017-10-24  9:19 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Quoting Chris Wilson (2017-10-23 22:32:35)
>> When we park the engine (upon idling), we kill the irq tasklet. However,
>> to be sure that it is not restarted by a final interrupt after doing so,
>> flush the interrupt handler before parking. As we only park the engines
>> when we believe the system is idle, there should not be any spurious
>> interrupts later to distrub us; so flushing the final in-flight
>> interrupt should be sufficient.
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index bb0e85043e01..b547a6327d34 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
>>         if (new_requests_since_last_retire(dev_priv))
>>                 goto out_unlock;
>>  
>> +       /*
>> +        * Be paranoid and flush a concurrent interrupt to make sure
>> +        * we don't reactivate any irq tasklets after parking.
>
> * FIXME: Note that even though we have waited for execlists to be idle,
> * there may still be an in-flight interrupt even though the CSB
> * is now empty. synchronize_irq() makes sure that the residual
> * interrupt is completed before we continue, but it doesn't prevent
> * the HW from raising that residual interrupt later. To complete
> * the shield we should coordinate disabling the CS irq with
> * flushing the residual interrupt.

Apparently tasklet_kill is 'broken' in a sence that
yeah new schedule reanimates the dead one. This
seems weird guarantee for an api of such name and
someone has even tried to fix it:

https://patchwork.kernel.org/patch/9298717/

I am pondering that can we combine the tasklet_disable
into the mix. We would just defer the tasklet until
we are ready and willing to process again?

-Mika

>
>> +        */
>> +       synchronize_irq(dev_priv->drm.irq);
>> +
>>         /*
>>          * We are committed now to parking the engines, make sure there
>>          * will be no more interrupts arriving later.
>> -- 
>> 2.15.0.rc1
>> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-24  9:19     ` Mika Kuoppala
@ 2017-10-24  9:32       ` Chris Wilson
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-24  9:32 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2017-10-24 10:19:15)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Chris Wilson (2017-10-23 22:32:35)
> >> When we park the engine (upon idling), we kill the irq tasklet. However,
> >> to be sure that it is not restarted by a final interrupt after doing so,
> >> flush the interrupt handler before parking. As we only park the engines
> >> when we believe the system is idle, there should not be any spurious
> >> interrupts later to distrub us; so flushing the final in-flight
> >> interrupt should be sufficient.
> >> 
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index bb0e85043e01..b547a6327d34 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
> >>         if (new_requests_since_last_retire(dev_priv))
> >>                 goto out_unlock;
> >>  
> >> +       /*
> >> +        * Be paranoid and flush a concurrent interrupt to make sure
> >> +        * we don't reactivate any irq tasklets after parking.
> >
> > * FIXME: Note that even though we have waited for execlists to be idle,
> > * there may still be an in-flight interrupt even though the CSB
> > * is now empty. synchronize_irq() makes sure that the residual
> > * interrupt is completed before we continue, but it doesn't prevent
> > * the HW from raising that residual interrupt later. To complete
> > * the shield we should coordinate disabling the CS irq with
> > * flushing the residual interrupt.
> 
> Apparently tasklet_kill is 'broken' in a sence that
> yeah new schedule reanimates the dead one. This
> seems weird guarantee for an api of such name and
> someone has even tried to fix it:
> 
> https://patchwork.kernel.org/patch/9298717/
> 
> I am pondering that can we combine the tasklet_disable
> into the mix. We would just defer the tasklet until
> we are ready and willing to process again?

Not for an extended period of time. tasklet_disable() has the
side-effect of spinning if a tasklet is scheduled between the
disable/enable calls. (It doesn't prevent the disabled tasklet from
being queued and doesn't remove it from the queue if it runs whilst
disabled.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] drm/i915: Synchronize irq before parking each engine
  2017-10-24  9:05   ` Chris Wilson
  2017-10-24  9:19     ` Mika Kuoppala
@ 2017-10-24 14:26     ` Mika Kuoppala
  1 sibling, 0 replies; 14+ messages in thread
From: Mika Kuoppala @ 2017-10-24 14:26 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Quoting Chris Wilson (2017-10-23 22:32:35)
>> When we park the engine (upon idling), we kill the irq tasklet. However,
>> to be sure that it is not restarted by a final interrupt after doing so,
>> flush the interrupt handler before parking. As we only park the engines
>> when we believe the system is idle, there should not be any spurious
>> interrupts later to distrub us; so flushing the final in-flight
>> interrupt should be sufficient.
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index bb0e85043e01..b547a6327d34 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3327,6 +3327,12 @@ i915_gem_idle_work_handler(struct work_struct *work)
>>         if (new_requests_since_last_retire(dev_priv))
>>                 goto out_unlock;
>>  
>> +       /*
>> +        * Be paranoid and flush a concurrent interrupt to make sure
>> +        * we don't reactivate any irq tasklets after parking.
>
> * FIXME: Note that even though we have waited for execlists to be idle,
> * there may still be an in-flight interrupt even though the CSB
> * is now empty. synchronize_irq() makes sure that the residual
> * interrupt is completed before we continue, but it doesn't prevent
> * the HW from raising that residual interrupt later. To complete
> * the shield we should coordinate disabling the CS irq with
> * flushing the residual interrupt.

I would call it spurious instead of residual as there is no real
reason for it.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>
>> +        */
>> +       synchronize_irq(dev_priv->drm.irq);
>> +
>>         /*
>>          * We are committed now to parking the engines, make sure there
>>          * will be no more interrupts arriving later.
>> -- 
>> 2.15.0.rc1
>> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-23 21:32 ` [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
@ 2017-10-24 14:40   ` Mika Kuoppala
  0 siblings, 0 replies; 14+ messages in thread
From: Mika Kuoppala @ 2017-10-24 14:40 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> context-switch when idle") we noticed the presence of late
> context-switch interrupts. We were able to filter those out by looking
> at whether the ELSP remained active, but in commit beecec901790
> ("drm/i915/execlists: Preemption!") that became problematic as we now
> anticipate receiving a context-switch event for preemption while ELSP
> may be empty. To restore the spurious interrupt suppression, add a
> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.
>
> v2: Don't forget to switch on for preempt.
> v3: Reduce the counter to a on/off boolean tracker. Declare the HW as
> active when we first submit, and idle after the final completion event
> (with which we confirm the HW says it is idle), and track each source
> of activity separately. With a finite number of sources, it should us
> debug which gets stuck.
>
> Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c           | 27 ++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    | 34 ++++++++++++++++++++++++++++--
>  5 files changed, 62 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114b739d..f84c267728fd 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  	execlists->first = rb;
>  	if (submit) {
>  		port_assign(port, last);
> +		execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
>  		i915_guc_submit(engine);
>  	}
>  	spin_unlock_irq(&engine->timeline->lock);
> @@ -633,6 +634,8 @@ static void i915_guc_irq_handler(unsigned long data)
>  
>  		rq = port_request(&port[0]);
>  	}
> +	if (!rq)
> +		execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
>  
>  	if (!port_isset(last_port))
>  		i915_guc_dequeue(engine);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a55c1e4..f8205841868b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1388,8 +1388,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
>  	bool tasklet = false;
>  
>  	if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
> -		__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> -		tasklet = true;
> +		if (READ_ONCE(engine->execlists.active)) {
> +			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> +			tasklet = true;

These two in here feel naturally attracted to eachothers. Perhaps
a future patch will meld them together.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +		}
>  	}
>  
>  	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a47a9c6bea52..ab5bf4e2e28e 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
>  	if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
>  		return false;
>  
> -	/* Both ports drained, no more ELSP submission? */
> -	if (port_request(&engine->execlists.port[0]))
> +	/* Waiting to drain ELSP? */
> +	if (READ_ONCE(engine->execlists.active))
>  		return false;
>  
>  	/* ELSP is empty, but there are ready requests? */
> @@ -1749,6 +1749,7 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
>  					   idx);
>  			}
>  		}
> +		drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
>  		rcu_read_unlock();
>  	} else if (INTEL_GEN(dev_priv) > 6) {
>  		drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 7f45dd7dc3e5..233c992a886b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -575,7 +575,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  			 * the state of the GPU is known (idle).
>  			 */
>  			inject_preempt_context(engine);
> -			execlists->preempt = true;
> +			execlists_set_active(execlists,
> +					     EXECLISTS_ACTIVE_PREEMPT);
>  			goto unlock;
>  		} else {
>  			/*
> @@ -683,8 +684,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  unlock:
>  	spin_unlock_irq(&engine->timeline->lock);
>  
> -	if (submit)
> +	if (submit) {
> +		execlists_set_active(execlists, EXECLISTS_ACTIVE_USER);
>  		execlists_submit_ports(engine);
> +	}
>  }
>  
>  static void
> @@ -696,6 +699,7 @@ execlist_cancel_port_requests(struct intel_engine_execlists *execlists)
>  	while (num_ports-- && port_isset(port)) {
>  		struct drm_i915_gem_request *rq = port_request(port);
>  
> +		GEM_BUG_ON(!execlists->active);
>  		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
>  		i915_gem_request_put(rq);
>  
> @@ -861,15 +865,21 @@ static void intel_lrc_irq_handler(unsigned long data)
>  				unwind_incomplete_requests(engine);
>  				spin_unlock_irq(&engine->timeline->lock);
>  
> -				GEM_BUG_ON(!execlists->preempt);
> -				execlists->preempt = false;
> +				GEM_BUG_ON(!execlists_is_active(execlists,
> +								EXECLISTS_ACTIVE_PREEMPT));
> +				execlists_clear_active(execlists,
> +						       EXECLISTS_ACTIVE_PREEMPT);
>  				continue;
>  			}
>  
>  			if (status & GEN8_CTX_STATUS_PREEMPTED &&
> -			    execlists->preempt)
> +			    execlists_is_active(execlists,
> +					       EXECLISTS_ACTIVE_PREEMPT))
>  				continue;
>  
> +			GEM_BUG_ON(!execlists_is_active(execlists,
> +							EXECLISTS_ACTIVE_USER));
> +
>  			/* Check the context/desc id for this event matches */
>  			GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
>  
> @@ -892,6 +902,9 @@ static void intel_lrc_irq_handler(unsigned long data)
>  			/* After the final element, the hw should be idle */
>  			GEM_BUG_ON(port_count(port) == 0 &&
>  				   !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
> +			if (port_count(port) == 0)
> +				execlists_clear_active(execlists,
> +						       EXECLISTS_ACTIVE_USER);
>  		}
>  
>  		if (head != execlists->csb_head) {
> @@ -901,7 +914,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>  		}
>  	}
>  
> -	if (!execlists->preempt)
> +	if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
>  		execlists_dequeue(engine);
>  
>  	intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
> @@ -1460,7 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  		   GT_CONTEXT_SWITCH_INTERRUPT << engine->irq_shift);
>  	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>  	execlists->csb_head = -1;
> -	execlists->preempt = false;
> +	execlists->active = 0;
>  
>  	/* After a GPU reset, we may have requests to replay */
>  	if (!i915_modparams.enable_guc_submission && execlists->first)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 17186f067408..51bc704bf413 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -241,9 +241,17 @@ struct intel_engine_execlists {
>  	} port[EXECLIST_MAX_PORTS];
>  
>  	/**
> -	 * @preempt: are we currently handling a preempting context switch?
> +	 * @active: is the HW active? We consider the HW as active after
> +	 * submitted any context for execution until we have seen the last
> +	 * context completion event. After that, we do not expect any more
> +	 * events until we submit, and so can park the HW.
> +	 *
> +	 * As we have a small number of different sources from which we feed
> +	 * the HW, we track the state of each inside a single bitfield.
>  	 */
> -	bool preempt;
> +	unsigned int active;
> +#define EXECLISTS_ACTIVE_USER 0
> +#define EXECLISTS_ACTIVE_PREEMPT 1
>  
>  	/**
>  	 * @port_mask: number of execlist ports - 1
> @@ -525,6 +533,27 @@ struct intel_engine_cs {
>  	u32 (*get_cmd_length_mask)(u32 cmd_header);
>  };
>  
> +static inline void
> +execlists_set_active(struct intel_engine_execlists *execlists,
> +		     unsigned int bit)
> +{
> +	__set_bit(bit, (unsigned long *)&execlists->active);
> +}
> +
> +static inline void
> +execlists_clear_active(struct intel_engine_execlists *execlists,
> +		       unsigned int bit)
> +{
> +	__clear_bit(bit, (unsigned long *)&execlists->active);
> +}
> +
> +static inline bool
> +execlists_is_active(const struct intel_engine_execlists *execlists,
> +		    unsigned int bit)
> +{
> +	return test_bit(bit, (unsigned long *)&execlists->active);
> +}
> +
>  static inline unsigned int
>  execlists_num_ports(const struct intel_engine_execlists * const execlists)
>  {
> @@ -538,6 +567,7 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
>  	const unsigned int m = execlists->port_mask;
>  
>  	GEM_BUG_ON(port_index(port, execlists) != 0);
> +	GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
>  
>  	memmove(port, port + 1, m * sizeof(struct execlist_port));
>  	memset(port + m, 0, sizeof(struct execlist_port));
> -- 
> 2.15.0.rc1
>
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] warn
  2017-10-23 21:32 ` [PATCH 4/4] warn Chris Wilson
@ 2017-10-24 14:46   ` Mika Kuoppala
  2017-10-24 15:01     ` Chris Wilson
  0 siblings, 1 reply; 14+ messages in thread
From: Mika Kuoppala @ 2017-10-24 14:46 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f8205841868b..e4bd20ce031d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1391,6 +1391,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
>  		if (READ_ONCE(engine->execlists.active)) {
>  			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>  			tasklet = true;
> +		} else if (WARN_ON(!engine->i915->gt.awake)) {

READ_ONCE

I'll get my coat.
-Mika

> +			struct drm_printer p = drm_info_printer(engine->i915->drm.dev);
> +			intel_engine_dump(engine, &p);
>  		}
>  	}
>  
> -- 
> 2.15.0.rc1
>
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] warn
  2017-10-24 14:46   ` Mika Kuoppala
@ 2017-10-24 15:01     ` Chris Wilson
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2017-10-24 15:01 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2017-10-24 15:46:27)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index f8205841868b..e4bd20ce031d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1391,6 +1391,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
> >               if (READ_ONCE(engine->execlists.active)) {
> >                       __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> >                       tasklet = true;
> > +             } else if (WARN_ON(!engine->i915->gt.awake)) {
> 
> READ_ONCE

Sure. Too bad this patch is just a figment of your imagination.

Pushed the rest, hopefully ending the plague of random deaths.
Thanks for the review,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-10-24 15:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-23 21:32 [PATCH 1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
2017-10-23 21:32 ` [PATCH 2/4] drm/i915: Synchronize irq before parking each engine Chris Wilson
2017-10-24  8:01   ` Chris Wilson
2017-10-24  9:05   ` Chris Wilson
2017-10-24  9:19     ` Mika Kuoppala
2017-10-24  9:32       ` Chris Wilson
2017-10-24 14:26     ` Mika Kuoppala
2017-10-23 21:32 ` [PATCH 3/4] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
2017-10-24 14:40   ` Mika Kuoppala
2017-10-23 21:32 ` [PATCH 4/4] warn Chris Wilson
2017-10-24 14:46   ` Mika Kuoppala
2017-10-24 15:01     ` Chris Wilson
2017-10-23 22:04 ` ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
2017-10-23 23:15 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.