[PATCH 1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

[PATCH 1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

[Chris Wilson]

Since commit e1fee72c2ea2e9c0c6e6743d32a6832f21337d6c
Author: Oscar Mateo <oscar.mateo@intel.com>
Date:   Thu Jul 24 17:04:40 2014 +0100

    drm/i915/bdw: Avoid non-lite-restore preemptions

execlists has listened to (ACTIVE_IDLE | ELEMENT_SWITCH) for detecting
when one context completed and it either continued onto the next (in port
1) or idled. We would always see COMPLETE | ACTIVE_IDLE on the final
context-switch event, but on recent gen it appears that we now get
separate ACTIVE_IDLE and COMPLETE events. In particular, the ACTIVE_IDLE
events may not be coupled to a context (since it is a general state rather
than a specific context completion event).

v2: Update the history, execlists did originally start out by listening
to the COMPLETE event not ACTIVE_IDLE.
v3: Update preempt completion test to also use COMPLETE not ACTIVE_IDLE.

References: bspec/12255
References: https://bugs.freedesktop.org/show_bug.cgi?id=103800
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Acked-by: Michel Thierry <michel.thierry@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index be6c39adebdf..c2cfdfdc0722 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -154,7 +154,7 @@
 #define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
 
 #define GEN8_CTX_STATUS_COMPLETED_MASK \
-	 (GEN8_CTX_STATUS_ACTIVE_IDLE | \
+	 (GEN8_CTX_STATUS_COMPLETE | \
 	  GEN8_CTX_STATUS_PREEMPTED | \
 	  GEN8_CTX_STATUS_ELEMENT_SWITCH)
 
@@ -876,7 +876,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
-			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE &&
+			if (status & GEN8_CTX_STATUS_COMPLETE &&
 			    buf[2*head + 1] == PREEMPT_ID) {
 				execlists_cancel_port_requests(execlists);
 				execlists_unwind_incomplete_requests(execlists);
-- 
2.15.0

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

[PATCH 2/4] drm/i915/execlists: Reduce completed event mask to COMPLETE | PREEMPTED

[Chris Wilson]

Since we get a COMPLETE event when the context switch occurs on
RING_HEAD == RING_TAIL and a PREEMPTED event when a switch occurs
before that point COMPLETE | PREEMPTED should cover all possible context
switch completion events. We can move the ELEMENT_SWITCH info message
from the COMPLETED_MASK into an assertion for when we are performing a
switch to port[1].

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c2cfdfdc0722..a2c7fc7bf4a4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -154,9 +154,7 @@
 #define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
 
 #define GEN8_CTX_STATUS_COMPLETED_MASK \
-	 (GEN8_CTX_STATUS_COMPLETE | \
-	  GEN8_CTX_STATUS_PREEMPTED | \
-	  GEN8_CTX_STATUS_ELEMENT_SWITCH)
+	 (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)
 
 #define CTX_LRI_HEADER_0		0x01
 #define CTX_CONTEXT_CONTROL		0x02
@@ -907,6 +905,8 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_BUG_ON(count == 0);
 			if (--count == 0) {
 				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
+				GEM_BUG_ON(port_isset(&port[1]) &&
+					   !(status & GEN8_CTX_STATUS_ELEMENT_SWITCH));
 				GEM_BUG_ON(!i915_gem_request_completed(rq));
 				execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 
-- 
2.15.0

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

[PATCH 3/4] drm/i915/execlists: Assert that we don't get mixed IDLE_ACTIVE | COMPLETE events

[Chris Wilson]

If IDLE_ACTIVE is set, then all other bits are invalid. For us, we can
assert that if we see a COMPLETE | PREEMPTED event, then it should be
impossible for it to also contain an IDLE_ACTIVE flag.

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a2c7fc7bf4a4..4aed00323780 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -874,6 +874,9 @@ static void execlists_submission_tasklet(unsigned long data)
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
+			/* We should never get a COMPLETED | IDLE_ACTIVE! */
+			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+
 			if (status & GEN8_CTX_STATUS_COMPLETE &&
 			    buf[2*head + 1] == PREEMPT_ID) {
 				execlists_cancel_port_requests(execlists);
-- 
2.15.0

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

[PATCH 4/4] drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

[Chris Wilson]

The hardware needs some time to process the information received in the
ExecList Submission Port, and expects us to not write anything more until
it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
PREEMPTED CSB event.

If we do not follow this, the driver could write new data into the ELSP
before HW had finishing fetching the previous one, putting us in
'undefined behaviour' space.

This seems to be the problem causing the spurious PREEMPTED & COMPLETE
events after a COMPLETE like the one below:

[] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
[] vcs0:  Execlist CSB[0]: 0x00000018 _ 0x00000007
[] vcs0:  Execlist CSB[1]: 0x00000001 _ 0x00000000
[] vcs0:  Execlist CSB[2]: 0x00000018 _ 0x00000007  <<< COMPLETE
[] vcs0:  Execlist CSB[3]: 0x00000012 _ 0x00000007  <<< PREEMPTED & COMPLETE
[] vcs0:  Execlist CSB[4]: 0x00008002 _ 0x00000006
[] vcs0:  Execlist CSB[5]: 0x00000014 _ 0x00000006

The ELSP writes that lead to this CSB sequence show that the HW hadn't
started executing the previous execlist (the one with only ctx 0x6) by the
time the new one was submitted; this is a bit more clear in the data
show in the EXECLIST_STATUS register at the time of the ELSP write.

[] vcs0: ELSP[0] = 0x0_0        [execlist1] - status_reg = 0x0_302
[] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302

[] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
[] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308

Note that having to wait for this ack does not disable lite-restores,
although it may reduce their numbers.

v2: Rewrote Michel's patch, his digging and his fix, my spelling.
v3: Reorder to ack early to allow preemption

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
Suggested-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4aed00323780..8d0c49388863 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -477,6 +477,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
 
 		elsp_write(desc, elsp);
 	}
+	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
 }
 
 static bool ctx_single_port_submission(const struct i915_gem_context *ctx)
@@ -529,6 +530,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 		elsp_write(0, elsp);
 
 	elsp_write(ce->lrc_desc, elsp);
+	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
 }
 
 static void execlists_dequeue(struct intel_engine_cs *engine)
@@ -575,9 +577,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		 * know the next preemption status we see corresponds
 		 * to this ELSP update.
 		 */
+		GEM_BUG_ON(!port_count(&port[0]));
 		if (port_count(&port[0]) > 1)
 			goto unlock;
 
+		/*
+		 * If we write to ELSP a second time before the HW has had
+		 * a chance to respond to the previous write, we can confuse
+		 * the HW and hit "undefined behaviour". After writing to ELSP,
+		 * we must then wait until we see a context-switch event from
+		 * the HW to indicate that it has had a chance to respond.
+		 */
+		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
+			goto unlock;
+
 		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
 		    rb_entry(rb, struct i915_priolist, node)->priority >
 		    max(last->priotree.priority, 0)) {
@@ -871,6 +884,15 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_TRACE("%s csb[%dd]: status=0x%08x:0x%08x\n",
 				  engine->name, head,
 				  status, buf[2*head + 1]);
+
+			if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+				      GEN8_CTX_STATUS_PREEMPTED))
+				execlists_set_active(execlists,
+						     EXECLISTS_ACTIVE_HWACK);
+			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+				execlists_clear_active(execlists,
+						       EXECLISTS_ACTIVE_HWACK);
+
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f867aa6c31fc..add7a30c1a61 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -252,6 +252,7 @@ struct intel_engine_execlists {
 	unsigned int active;
 #define EXECLISTS_ACTIVE_USER 0
 #define EXECLISTS_ACTIVE_PREEMPT 1
+#define EXECLISTS_ACTIVE_HWACK 2
 
 	/**
 	 * @port_mask: number of execlist ports - 1
-- 
2.15.0

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE
URL   : https://patchwork.freedesktop.org/series/34099/
State : success

== Summary ==

Series 34099v1 series starting with [1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE
https://patchwork.freedesktop.org/api/1.0/series/34099/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#103163
Test kms_busy:
        Subgroup basic-flip-a:
                fail       -> PASS       (fi-gdg-551) fdo#102654
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103163 https://bugs.freedesktop.org/show_bug.cgi?id=103163
fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:454s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:380s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:541s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:278s
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:505s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:502s
fi-cfl-s2        total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:621s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:432s
fi-gdg-551       total:289  pass:177  dwarn:1   dfail:0   fail:2   skip:109 time:274s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:439s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:427s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:475s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:531s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:480s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:535s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:578s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:545s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:568s
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:497s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:418s
Blacklisted hosts:
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:500s

4b69b21e93c02a8b45781c490077dc1a35adc608 drm-tip: 2017y-11m-20d-10h-20m-58s UTC integration manifest
5e306ad55da5 drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write
25f84524ef0c drm/i915/execlists: Assert that we don't get mixed IDLE_ACTIVE | COMPLETE events
ab5e5daf9607 drm/i915/execlists: Reduce completed event mask to COMPLETE | PREEMPTED
2ff5aaa88dd4 drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

== Logs ==

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

Re: [PATCH 3/4] drm/i915/execlists: Assert that we don't get mixed IDLE_ACTIVE | COMPLETE events

[Joonas Lahtinen]

On Mon, 2017-11-20 at 12:34 +0000, Chris Wilson wrote:
> If IDLE_ACTIVE is set, then all other bits are invalid. For us, we can
> assert that if we see a COMPLETE | PREEMPTED event, then it should be
> impossible for it to also contain an IDLE_ACTIVE flag.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/execlists: Reduce completed event mask to COMPLETE | PREEMPTED

[Joonas Lahtinen]

On Mon, 2017-11-20 at 12:34 +0000, Chris Wilson wrote:
> Since we get a COMPLETE event when the context switch occurs on
> RING_HEAD == RING_TAIL and a PREEMPTED event when a switch occurs
> before that point COMPLETE | PREEMPTED should cover all possible context
> switch completion events. We can move the ELEMENT_SWITCH info message
> from the COMPLETED_MASK into an assertion for when we are performing a
> switch to port[1].
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/execlists: Listen to COMPLETE context event not ACTIVE_IDLE
URL   : https://patchwork.freedesktop.org/series/34099/
State : failure

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-snb) fdo#101623
Test gem_eio:
        Subgroup in-flight-suspend:
                fail       -> PASS       (shard-hsw) fdo#103375
Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                pass       -> INCOMPLETE (shard-snb)
Test drv_module_reload:
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (shard-hsw) fdo#102707

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707

shard-hsw        total:2585 pass:1473 dwarn:2   dfail:1   fail:10  skip:1099 time:9537s
shard-snb        total:2531 pass:1231 dwarn:1   dfail:1   fail:11  skip:1286 time:7764s
Blacklisted hosts:
shard-apl        total:2565 pass:1605 dwarn:1   dfail:0   fail:22  skip:936 time:12977s
shard-kbl        total:2565 pass:1701 dwarn:7   dfail:0   fail:24  skip:832 time:10594s

== Logs ==

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

Re: [PATCH 3/4] drm/i915/execlists: Assert that we don't get mixed IDLE_ACTIVE | COMPLETE events

[Chris Wilson]

Quoting Joonas Lahtinen (2017-11-20 13:25:58)
> On Mon, 2017-11-20 at 12:34 +0000, Chris Wilson wrote:
> > If IDLE_ACTIVE is set, then all other bits are invalid. For us, we can
> > assert that if we see a COMPLETE | PREEMPTED event, then it should be
> > impossible for it to also contain an IDLE_ACTIVE flag.
> > 
> > Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Applied up to this point, as we are just tidying and adding more
asserts. The next patch is the crutch.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

[Michel Thierry]

On 11/20/2017 4:34 AM, Chris Wilson wrote:
> The hardware needs some time to process the information received in the
> ExecList Submission Port, and expects us to not write anything more until
> it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
> PREEMPTED CSB event.
> 
> If we do not follow this, the driver could write new data into the ELSP
> before HW had finishing fetching the previous one, putting us in
> 'undefined behaviour' space.
> 
> This seems to be the problem causing the spurious PREEMPTED & COMPLETE
> events after a COMPLETE like the one below:
> 
> [] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
> [] vcs0:  Execlist CSB[0]: 0x00000018 _ 0x00000007
> [] vcs0:  Execlist CSB[1]: 0x00000001 _ 0x00000000
> [] vcs0:  Execlist CSB[2]: 0x00000018 _ 0x00000007  <<< COMPLETE
> [] vcs0:  Execlist CSB[3]: 0x00000012 _ 0x00000007  <<< PREEMPTED & COMPLETE
> [] vcs0:  Execlist CSB[4]: 0x00008002 _ 0x00000006
> [] vcs0:  Execlist CSB[5]: 0x00000014 _ 0x00000006
> 
> The ELSP writes that lead to this CSB sequence show that the HW hadn't
> started executing the previous execlist (the one with only ctx 0x6) by the
> time the new one was submitted; this is a bit more clear in the data
> show in the EXECLIST_STATUS register at the time of the ELSP write.
> 
> [] vcs0: ELSP[0] = 0x0_0        [execlist1] - status_reg = 0x0_302
> [] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302
> 
> [] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
> [] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308
> 
> Note that having to wait for this ack does not disable lite-restores,
> although it may reduce their numbers.
> 
> v2: Rewrote Michel's patch, his digging and his fix, my spelling.
> v3: Reorder to ack early to allow preemption
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
> Suggested-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>

Isn't it nice to come back after the weekend and see everything is ok?

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Thanks,

> ---
>   drivers/gpu/drm/i915/intel_lrc.c        | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>   2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4aed00323780..8d0c49388863 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -477,6 +477,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
>   
>   		elsp_write(desc, elsp);
>   	}
> +	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
>   }
>   
>   static bool ctx_single_port_submission(const struct i915_gem_context *ctx)
> @@ -529,6 +530,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   		elsp_write(0, elsp);
>   
>   	elsp_write(ce->lrc_desc, elsp);
> +	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
>   }
>   
>   static void execlists_dequeue(struct intel_engine_cs *engine)
> @@ -575,9 +577,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		 * know the next preemption status we see corresponds
>   		 * to this ELSP update.
>   		 */
> +		GEM_BUG_ON(!port_count(&port[0]));
>   		if (port_count(&port[0]) > 1)
>   			goto unlock;
>   
> +		/*
> +		 * If we write to ELSP a second time before the HW has had
> +		 * a chance to respond to the previous write, we can confuse
> +		 * the HW and hit "undefined behaviour". After writing to ELSP,
> +		 * we must then wait until we see a context-switch event from
> +		 * the HW to indicate that it has had a chance to respond.
> +		 */
> +		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
> +			goto unlock;
> +
>   		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
>   		    rb_entry(rb, struct i915_priolist, node)->priority >
>   		    max(last->priotree.priority, 0)) {
> @@ -871,6 +884,15 @@ static void execlists_submission_tasklet(unsigned long data)
>   			GEM_TRACE("%s csb[%dd]: status=0x%08x:0x%08x\n",
>   				  engine->name, head,
>   				  status, buf[2*head + 1]);
> +
> +			if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
> +				      GEN8_CTX_STATUS_PREEMPTED))
> +				execlists_set_active(execlists,
> +						     EXECLISTS_ACTIVE_HWACK);
> +			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
> +				execlists_clear_active(execlists,
> +						       EXECLISTS_ACTIVE_HWACK);
> +
>   			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>   				continue;
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f867aa6c31fc..add7a30c1a61 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -252,6 +252,7 @@ struct intel_engine_execlists {
>   	unsigned int active;
>   #define EXECLISTS_ACTIVE_USER 0
>   #define EXECLISTS_ACTIVE_PREEMPT 1
> +#define EXECLISTS_ACTIVE_HWACK 2
>   
>   	/**
>   	 * @port_mask: number of execlist ports - 1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

[Chris Wilson]

Quoting Michel Thierry (2017-11-20 16:17:01)
> On 11/20/2017 4:34 AM, Chris Wilson wrote:
> > The hardware needs some time to process the information received in the
> > ExecList Submission Port, and expects us to not write anything more until
> > it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
> > PREEMPTED CSB event.
> > 
> > If we do not follow this, the driver could write new data into the ELSP
> > before HW had finishing fetching the previous one, putting us in
> > 'undefined behaviour' space.
> > 
> > This seems to be the problem causing the spurious PREEMPTED & COMPLETE
> > events after a COMPLETE like the one below:
> > 
> > [] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
> > [] vcs0:  Execlist CSB[0]: 0x00000018 _ 0x00000007
> > [] vcs0:  Execlist CSB[1]: 0x00000001 _ 0x00000000
> > [] vcs0:  Execlist CSB[2]: 0x00000018 _ 0x00000007  <<< COMPLETE
> > [] vcs0:  Execlist CSB[3]: 0x00000012 _ 0x00000007  <<< PREEMPTED & COMPLETE
> > [] vcs0:  Execlist CSB[4]: 0x00008002 _ 0x00000006
> > [] vcs0:  Execlist CSB[5]: 0x00000014 _ 0x00000006
> > 
> > The ELSP writes that lead to this CSB sequence show that the HW hadn't
> > started executing the previous execlist (the one with only ctx 0x6) by the
> > time the new one was submitted; this is a bit more clear in the data
> > show in the EXECLIST_STATUS register at the time of the ELSP write.
> > 
> > [] vcs0: ELSP[0] = 0x0_0        [execlist1] - status_reg = 0x0_302
> > [] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302
> > 
> > [] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
> > [] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308
> > 
> > Note that having to wait for this ack does not disable lite-restores,
> > although it may reduce their numbers.
> > 
> > v2: Rewrote Michel's patch, his digging and his fix, my spelling.
> > v3: Reorder to ack early to allow preemption
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
> > Suggested-by: Michel Thierry <michel.thierry@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> 
> Isn't it nice to come back after the weekend and see everything is ok?
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Do you want to do a quick review-author swap? You take authorship and
I'll take reviewership? It is basically your patch. :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

[Chris Wilson]

Quoting Chris Wilson (2017-11-20 16:45:44)
> Quoting Michel Thierry (2017-11-20 16:17:01)
> > On 11/20/2017 4:34 AM, Chris Wilson wrote:
> > > The hardware needs some time to process the information received in the
> > > ExecList Submission Port, and expects us to not write anything more until
> > > it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
> > > PREEMPTED CSB event.
> > > 
> > > If we do not follow this, the driver could write new data into the ELSP
> > > before HW had finishing fetching the previous one, putting us in
> > > 'undefined behaviour' space.
> > > 
> > > This seems to be the problem causing the spurious PREEMPTED & COMPLETE
> > > events after a COMPLETE like the one below:
> > > 
> > > [] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
> > > [] vcs0:  Execlist CSB[0]: 0x00000018 _ 0x00000007
> > > [] vcs0:  Execlist CSB[1]: 0x00000001 _ 0x00000000
> > > [] vcs0:  Execlist CSB[2]: 0x00000018 _ 0x00000007  <<< COMPLETE
> > > [] vcs0:  Execlist CSB[3]: 0x00000012 _ 0x00000007  <<< PREEMPTED & COMPLETE
> > > [] vcs0:  Execlist CSB[4]: 0x00008002 _ 0x00000006
> > > [] vcs0:  Execlist CSB[5]: 0x00000014 _ 0x00000006
> > > 
> > > The ELSP writes that lead to this CSB sequence show that the HW hadn't
> > > started executing the previous execlist (the one with only ctx 0x6) by the
> > > time the new one was submitted; this is a bit more clear in the data
> > > show in the EXECLIST_STATUS register at the time of the ELSP write.
> > > 
> > > [] vcs0: ELSP[0] = 0x0_0        [execlist1] - status_reg = 0x0_302
> > > [] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302
> > > 
> > > [] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
> > > [] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308
> > > 
> > > Note that having to wait for this ack does not disable lite-restores,
> > > although it may reduce their numbers.
> > > 
> > > v2: Rewrote Michel's patch, his digging and his fix, my spelling.
> > > v3: Reorder to ack early to allow preemption
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
> > > Suggested-by: Michel Thierry <michel.thierry@intel.com>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michel Thierry <michel.thierry@intel.com>
> > 
> > Isn't it nice to come back after the weekend and see everything is ok?
> > 
> > Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> 
> Do you want to do a quick review-author swap? You take authorship and
> I'll take reviewership? It is basically your patch. :)

Michel said, on irc, that he didn't mind who took authorship, so I gave
it back to him for doing the work, and pushed. Thanks for the really
nasty bug fix,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx