All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking
@ 2017-10-23 20:06 Chris Wilson
  2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
  2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
  0 siblings, 2 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-23 20:06 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] 11+ messages in thread

* [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
@ 2017-10-23 20:06 ` Chris Wilson
  2017-10-23 21:12   ` Chris Wilson
  2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
  1 sibling, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-10-23 20:06 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] 11+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking
  2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
  2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
@ 2017-10-23 20:26 ` Patchwork
  1 sibling, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-10-23 20:26 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (fi-hsw-4770r)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:456s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:371s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:531s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:265s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:506s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:495s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:478s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:557s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:417s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:249s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:578s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:432s
fi-hsw-4770r     total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:426s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:434s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:497s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:460s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:494s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:570s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:592s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:548s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:645s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:527s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:506s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:567s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s

5c82a37eff83ab4e60e760fbaf03db5ba0563497 drm-tip: 2017y-10m-23d-18h-06m-28s UTC integration manifest
077bf0f23d24 drm/i915: Filter out spurious execlists context-switch interrupts
6129d04f29c2 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_6146/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
@ 2017-10-23 21:12   ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-23 21:12 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Quoting Chris Wilson (2017-10-23 21:06:16)
> 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.

Looking at an example from
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_1299/
the common case is where we still get the interrupt after already
parsing the whole CSB:

<6>[   22.723238] i915 0000:00:02.0: [drm] vecs0
<6>[   22.723246] i915 0000:00:02.0: [drm] 	current seqno 8, last 8, hangcheck 0 [-277277 ms], inflight 0
<6>[   22.723260] i915 0000:00:02.0: [drm] 	Reset count: 0
<6>[   22.723269] i915 0000:00:02.0: [drm] 	Requests:
<6>[   22.723278] i915 0000:00:02.0: [drm] 	RING_START: 0x007fb000 [0x00000000]
<6>[   22.723289] i915 0000:00:02.0: [drm] 	RING_HEAD:  0x00000278 [0x00000000]
<6>[   22.723300] i915 0000:00:02.0: [drm] 	RING_TAIL:  0x00000278 [0x00000000]
<6>[   22.723311] i915 0000:00:02.0: [drm] 	RING_CTL:   0x00003001 []
<6>[   22.723322] i915 0000:00:02.0: [drm] 	ACTHD:  0x00000000_00000278
<6>[   22.723333] i915 0000:00:02.0: [drm] 	BBADDR: 0x00000000_00000004
<6>[   22.723343] i915 0000:00:02.0: [drm] 	Execlist status: 0x00000301 00000000
<6>[   22.723355] i915 0000:00:02.0: [drm] 	Execlist CSB read 1 [1 cached], write 1 [1 from hws], interrupt posted? no
<6>[   22.723370] i915 0000:00:02.0: [drm] 		ELSP[0] idle
<6>[   22.723378] i915 0000:00:02.0: [drm] 		ELSP[1] idle
<6>[   22.723387] i915 0000:00:02.0: [drm] 		HW active? 0x0
<6>[   22.723402] i915 0000:00:02.0: [drm] 


Those should not lead to hitting BUG_ON(gt.awake) though as the tasklet
is flushed before we clear gt.awake. Except if maybe the interrupt
arrives after the tasklet_kill...

Given that we wait for the engines to be idle before parking, we should
be safe enough with

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bb0e85043e01..fa46137d431a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3327,6 +3327,8 @@ i915_gem_idle_work_handler(struct work_struct *work)
        if (new_requests_since_last_retire(dev_priv))
                goto out_unlock;
 
+       synchronize_irq(dev_priv->drm.irq);
+
        /*
         * We are committed now to parking the engines, make sure there
         * will be no more interrupts arriving later.

to flush a pending irq and not worry about a multi-phase park.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

Quoting Mika Kuoppala (2017-10-20 14:59:44)
> 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.
> >
> > 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 | 11 ++++++-----
> >  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
> >  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
> >  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
> >  5 files changed, 37 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..c22d0a07467c 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
> >                       struct drm_i915_gem_request *rq)
> >  {
> >       GEM_BUG_ON(rq == port_request(port));
> > +     GEM_BUG_ON(port_isset(port));
> >  
> > -     if (port_isset(port))
> > -             i915_gem_request_put(port_request(port));
> > -
> > -     port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> > +     port_set(port, i915_gem_request_get(rq));
> 
> No lite restore with guc so we can make this more strict and lean?

Yes. We are not coalescing requests into an active slot, so we always
know that we are assigning a new request into a new slot.

> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a47a9c6bea52..8aecbc321786 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;
> >
> 
> It would not make sense now to keep the port[0] check but
> we could get more fine grained debugging info if we keep
> it?

This function is the antithesis of fine grained debugging. It is called
in a tight loop, watching for idle so reporting from here ends up with a
lot of noise. Often I ask exactly what isn't idle... Now that we do a
single upfront wait, and then a check all is well, moving this to
intel_engine_park where we can be more verbose in the one-off state
check is the next step.

The importance of making this change is so the gen8_cs_irq_handler() and
the idle_worker are in sync about when to drop the GT wakeref.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20  9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
  2017-10-20 13:21   ` Mika Kuoppala
@ 2017-10-20 13:59   ` Mika Kuoppala
  2017-10-20 14:24     ` Chris Wilson
  1 sibling, 1 reply; 11+ messages in thread
From: Mika Kuoppala @ 2017-10-20 13:59 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.
>
> 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 | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
>  5 files changed, 37 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..c22d0a07467c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
>  	GEM_BUG_ON(rq == port_request(port));
> +	GEM_BUG_ON(port_isset(port));
>  
> -	if (port_isset(port))
> -		i915_gem_request_put(port_request(port));
> -
> -	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +	port_set(port, i915_gem_request_get(rq));

No lite restore with guc so we can make this more strict and lean?

>  	nested_enable_signaling(rq);
>  }
>  
> @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  					goto done;
>  				}
>  
> -				if (submit)
> +				if (submit) {
>  					port_assign(port, last);
> +					execlists->active++;
> +				}
>  				port++;
>  			}
>  
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  	execlists->first = rb;
>  	if (submit) {
>  		port_assign(port, last);
> +		execlists->active++;
>  		i915_guc_submit(engine);
>  	}
>  	spin_unlock_irq(&engine->timeline->lock);
> 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..8aecbc321786 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;
>

It would not make sense now to keep the port[0] check but
we could get more fine grained debugging info if we keep
it?

-Mika

>  	/* 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\tELSP active %d\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..8a47b8211c74 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev,
>  	return true;
>  }
>  
> -static void port_assign(struct execlist_port *port,
> +static bool port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
> +	bool was_idle;
> +
>  	GEM_BUG_ON(rq == port_request(port));
>  
> -	if (port_isset(port))
> +	if (port_isset(port)) {
>  		i915_gem_request_put(port_request(port));
> +		was_idle = false;
> +	} else {
> +		was_idle = true;
> +	}
>  
>  	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +
> +	return was_idle;
>  }
>  
>  static void inject_preempt_context(struct intel_engine_cs *engine)
> @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  				GEM_BUG_ON(last->ctx == rq->ctx);
>  
>  				if (submit)
> -					port_assign(port, last);
> +					execlists->active += port_assign(port, last);
>  				port++;
>  
>  				GEM_BUG_ON(port_isset(port));
> @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  done:
>  	execlists->first = rb;
>  	if (submit)
> -		port_assign(port, last);
> +		execlists->active += port_assign(port, last);
>  unlock:
>  	spin_unlock_irq(&engine->timeline->lock);
>  
> @@ -696,10 +704,12 @@ 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);
>  
>  		memset(port, 0, sizeof(*port));
> +		execlists->active--;
>  		port++;
>  	}
>  }
> @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data)
>  
>  				GEM_BUG_ON(!execlists->preempt);
>  				execlists->preempt = false;
> +				execlists->active--;
> +				GEM_BUG_ON(execlists->active);
>  				continue;
>  			}
>  
> @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  	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..588f5fe9d2b7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -245,6 +245,11 @@ struct intel_engine_execlists {
>  	 */
>  	bool preempt;
>  
> +	/**
> +	 * @active: count of the number of outstanding CSB events
> +	 */
> +	unsigned int active;
> +
>  	/**
>  	 * @port_mask: number of execlist ports - 1
>  	 */
> @@ -538,9 +543,11 @@ 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->active);
>  
>  	memmove(port, port + 1, m * sizeof(struct execlist_port));
>  	memset(port + m, 0, sizeof(struct execlist_port));
> +	execlists->active--;
>  }
>  
>  static inline unsigned int
> -- 
> 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] 11+ messages in thread

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20 13:31       ` Mika Kuoppala
@ 2017-10-20 13:47         ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-20 13:47 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2017-10-20 14:31:14)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2017-10-20 14:21:08)
> >> 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
> >> 
> >> This confuses me, how can we preempt something that was never submitted.
> >> Could you elaborate?
> >
> > The ELSP may become empty after we told the hw to switch to the preempt
> > context. So the preemption context-switch may appear as a separate
> > interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
> > communications.
> 
> Let me check if I got this right: as we dont use port[0] to
> switch to preempt context and thus we might get 2 interrupts in a
> a row, first one clearing port[0], we can't assume any idleness
> by port[0] status alone?

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

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

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20 13:24     ` Chris Wilson
@ 2017-10-20 13:31       ` Mika Kuoppala
  2017-10-20 13:47         ` Chris Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Mika Kuoppala @ 2017-10-20 13:31 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Quoting Mika Kuoppala (2017-10-20 14:21:08)
>> 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
>> 
>> This confuses me, how can we preempt something that was never submitted.
>> Could you elaborate?
>
> The ELSP may become empty after we told the hw to switch to the preempt
> context. So the preemption context-switch may appear as a separate
> interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
> communications.

Let me check if I got this right: as we dont use port[0] to
switch to preempt context and thus we might get 2 interrupts in a
a row, first one clearing port[0], we can't assume any idleness
by port[0] status alone?

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

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

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20 13:21   ` Mika Kuoppala
@ 2017-10-20 13:24     ` Chris Wilson
  2017-10-20 13:31       ` Mika Kuoppala
  0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-10-20 13:24 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2017-10-20 14:21:08)
> 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
> 
> This confuses me, how can we preempt something that was never submitted.
> Could you elaborate?

The ELSP may become empty after we told the hw to switch to the preempt
context. So the preemption context-switch may appear as a separate
interrupt after !port_isset(execlists.port[0]). The joy of asynchronous
communications.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20  9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
@ 2017-10-20 13:21   ` Mika Kuoppala
  2017-10-20 13:24     ` Chris Wilson
  2017-10-20 13:59   ` Mika Kuoppala
  1 sibling, 1 reply; 11+ messages in thread
From: Mika Kuoppala @ 2017-10-20 13:21 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

This confuses me, how can we preempt something that was never submitted.
Could you elaborate?

Thanks,
-Mika

> counter for the expected number of pending context-switches and skip if
> we do not need to handle this interrupt to make forward progress.
>
> 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 | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
>  5 files changed, 37 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..c22d0a07467c 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
>  	GEM_BUG_ON(rq == port_request(port));
> +	GEM_BUG_ON(port_isset(port));
>  
> -	if (port_isset(port))
> -		i915_gem_request_put(port_request(port));
> -
> -	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +	port_set(port, i915_gem_request_get(rq));
>  	nested_enable_signaling(rq);
>  }
>  
> @@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  					goto done;
>  				}
>  
> -				if (submit)
> +				if (submit) {
>  					port_assign(port, last);
> +					execlists->active++;
> +				}
>  				port++;
>  			}
>  
> @@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
>  	execlists->first = rb;
>  	if (submit) {
>  		port_assign(port, last);
> +		execlists->active++;
>  		i915_guc_submit(engine);
>  	}
>  	spin_unlock_irq(&engine->timeline->lock);
> 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..8aecbc321786 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\tELSP active %d\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..8a47b8211c74 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev,
>  	return true;
>  }
>  
> -static void port_assign(struct execlist_port *port,
> +static bool port_assign(struct execlist_port *port,
>  			struct drm_i915_gem_request *rq)
>  {
> +	bool was_idle;
> +
>  	GEM_BUG_ON(rq == port_request(port));
>  
> -	if (port_isset(port))
> +	if (port_isset(port)) {
>  		i915_gem_request_put(port_request(port));
> +		was_idle = false;
> +	} else {
> +		was_idle = true;
> +	}
>  
>  	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> +
> +	return was_idle;
>  }
>  
>  static void inject_preempt_context(struct intel_engine_cs *engine)
> @@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  				GEM_BUG_ON(last->ctx == rq->ctx);
>  
>  				if (submit)
> -					port_assign(port, last);
> +					execlists->active += port_assign(port, last);
>  				port++;
>  
>  				GEM_BUG_ON(port_isset(port));
> @@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  done:
>  	execlists->first = rb;
>  	if (submit)
> -		port_assign(port, last);
> +		execlists->active += port_assign(port, last);
>  unlock:
>  	spin_unlock_irq(&engine->timeline->lock);
>  
> @@ -696,10 +704,12 @@ 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);
>  
>  		memset(port, 0, sizeof(*port));
> +		execlists->active--;
>  		port++;
>  	}
>  }
> @@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data)
>  
>  				GEM_BUG_ON(!execlists->preempt);
>  				execlists->preempt = false;
> +				execlists->active--;
> +				GEM_BUG_ON(execlists->active);
>  				continue;
>  			}
>  
> @@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  	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..588f5fe9d2b7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -245,6 +245,11 @@ struct intel_engine_execlists {
>  	 */
>  	bool preempt;
>  
> +	/**
> +	 * @active: count of the number of outstanding CSB events
> +	 */
> +	unsigned int active;
> +
>  	/**
>  	 * @port_mask: number of execlist ports - 1
>  	 */
> @@ -538,9 +543,11 @@ 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->active);
>  
>  	memmove(port, port + 1, m * sizeof(struct execlist_port));
>  	memset(port + m, 0, sizeof(struct execlist_port));
> +	execlists->active--;
>  }
>  
>  static inline unsigned int
> -- 
> 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] 11+ messages in thread

* [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts
  2017-10-20  9:59 [PATCH 1/2] " Chris Wilson
@ 2017-10-20  9:59 ` Chris Wilson
  2017-10-20 13:21   ` Mika Kuoppala
  2017-10-20 13:59   ` Mika Kuoppala
  0 siblings, 2 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-20  9:59 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.

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 | 11 ++++++-----
 drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
 drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
 drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
 5 files changed, 37 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..c22d0a07467c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
 			struct drm_i915_gem_request *rq)
 {
 	GEM_BUG_ON(rq == port_request(port));
+	GEM_BUG_ON(port_isset(port));
 
-	if (port_isset(port))
-		i915_gem_request_put(port_request(port));
-
-	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
+	port_set(port, i915_gem_request_get(rq));
 	nested_enable_signaling(rq);
 }
 
@@ -586,8 +584,10 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 					goto done;
 				}
 
-				if (submit)
+				if (submit) {
 					port_assign(port, last);
+					execlists->active++;
+				}
 				port++;
 			}
 
@@ -610,6 +610,7 @@ static void i915_guc_dequeue(struct intel_engine_cs *engine)
 	execlists->first = rb;
 	if (submit) {
 		port_assign(port, last);
+		execlists->active++;
 		i915_guc_submit(engine);
 	}
 	spin_unlock_irq(&engine->timeline->lock);
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..8aecbc321786 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\tELSP active %d\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..8a47b8211c74 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -482,15 +482,23 @@ static bool can_merge_ctx(const struct i915_gem_context *prev,
 	return true;
 }
 
-static void port_assign(struct execlist_port *port,
+static bool port_assign(struct execlist_port *port,
 			struct drm_i915_gem_request *rq)
 {
+	bool was_idle;
+
 	GEM_BUG_ON(rq == port_request(port));
 
-	if (port_isset(port))
+	if (port_isset(port)) {
 		i915_gem_request_put(port_request(port));
+		was_idle = false;
+	} else {
+		was_idle = true;
+	}
 
 	port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
+
+	return was_idle;
 }
 
 static void inject_preempt_context(struct intel_engine_cs *engine)
@@ -657,7 +665,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				GEM_BUG_ON(last->ctx == rq->ctx);
 
 				if (submit)
-					port_assign(port, last);
+					execlists->active += port_assign(port, last);
 				port++;
 
 				GEM_BUG_ON(port_isset(port));
@@ -679,7 +687,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 done:
 	execlists->first = rb;
 	if (submit)
-		port_assign(port, last);
+		execlists->active += port_assign(port, last);
 unlock:
 	spin_unlock_irq(&engine->timeline->lock);
 
@@ -696,10 +704,12 @@ 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);
 
 		memset(port, 0, sizeof(*port));
+		execlists->active--;
 		port++;
 	}
 }
@@ -863,6 +873,8 @@ static void intel_lrc_irq_handler(unsigned long data)
 
 				GEM_BUG_ON(!execlists->preempt);
 				execlists->preempt = false;
+				execlists->active--;
+				GEM_BUG_ON(execlists->active);
 				continue;
 			}
 
@@ -1461,6 +1473,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 	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..588f5fe9d2b7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -245,6 +245,11 @@ struct intel_engine_execlists {
 	 */
 	bool preempt;
 
+	/**
+	 * @active: count of the number of outstanding CSB events
+	 */
+	unsigned int active;
+
 	/**
 	 * @port_mask: number of execlist ports - 1
 	 */
@@ -538,9 +543,11 @@ 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->active);
 
 	memmove(port, port + 1, m * sizeof(struct execlist_port));
 	memset(port + m, 0, sizeof(struct execlist_port));
+	execlists->active--;
 }
 
 static inline unsigned int
-- 
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] 11+ messages in thread

end of thread, other threads:[~2017-10-23 21:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-23 20:06 [PATCH 1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Chris Wilson
2017-10-23 20:06 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
2017-10-23 21:12   ` Chris Wilson
2017-10-23 20:26 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Bump wait-times for the final CS interrupt before parking Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2017-10-20  9:59 [PATCH 1/2] " Chris Wilson
2017-10-20  9:59 ` [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts Chris Wilson
2017-10-20 13:21   ` Mika Kuoppala
2017-10-20 13:24     ` Chris Wilson
2017-10-20 13:31       ` Mika Kuoppala
2017-10-20 13:47         ` Chris Wilson
2017-10-20 13:59   ` Mika Kuoppala
2017-10-20 14:24     ` Chris Wilson

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.