[PATCH] drm/i915: Force ordering on request submission and hangcheck

[PATCH] drm/i915: Force ordering on request submission and hangcheck

[Mika Kuoppala]

Hangcheck is run on irq context and might be active on a
completely different CPU that is submitting requests. And as
we have been very careful not to add locking to hangcheck to guard
against driver failures, we need to be careful with the coherency.

Update ring last seqno and add to request lists early, with
write memory barrier, before pushing the request to ring. On
hangcheck side, use read memory barrier before inspecting the ring
last seqno. This ensures that when hangcheck is inspecting the
state, it will not see a active ring without requests in it,
and then falsely report it as a missed irq situation.

References: https://bugs.freedesktop.org/show_bug.cgi?id=93693
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
 drivers/gpu/drm/i915/i915_irq.c |  7 ++++---
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ddc21d4b388d..0ca3856ffbf1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2584,6 +2584,17 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 	 */
 	request->postfix = intel_ring_get_tail(ringbuf);
 
+	/* Hangcheck runs in irq context and might be even
+	 * on different CPU. So we need to the do last seqno and
+	 * list addition early with memory barrier. Otherwise hangcheck
+	 * might see active ring without any requests and
+	 * consider it falsely as a missed irq situation.
+	 */
+	request->previous_seqno = ring->last_submitted_seqno;
+	ring->last_submitted_seqno = request->seqno;
+	list_add_tail(&request->list, &ring->request_list);
+	wmb(); /* Flush last_submitted_seqno and request->list for hangcheck */
+
 	if (i915.enable_execlists)
 		ret = ring->emit_request(request);
 	else {
@@ -2605,9 +2616,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 	request->batch_obj = obj;
 
 	request->emitted_jiffies = jiffies;
-	request->previous_seqno = ring->last_submitted_seqno;
-	ring->last_submitted_seqno = request->seqno;
-	list_add_tail(&request->list, &ring->request_list);
 
 	trace_i915_gem_request_add(request);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 25a89373df63..4dd8e6f0c1de 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2801,8 +2801,9 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 static bool
 ring_idle(struct intel_engine_cs *ring, u32 seqno)
 {
-	return (list_empty(&ring->request_list) ||
-		i915_seqno_passed(seqno, ring->last_submitted_seqno));
+	rmb(); /* Ordering against __i915_add_request() */
+	return (i915_seqno_passed(seqno, ring->last_submitted_seqno)
+		|| list_empty(&ring->request_list));
 }
 
 static bool
@@ -3101,8 +3102,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 
 		semaphore_clear_deadlocks(dev_priv);
 
-		seqno = ring->get_seqno(ring, false);
 		acthd = intel_ring_get_active_head(ring);
+		seqno = ring->get_seqno(ring, false);
 
 		if (ring->hangcheck.seqno == seqno) {
 			if (ring_idle(ring, seqno)) {
-- 
2.5.0

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

Re: [PATCH] drm/i915: Force ordering on request submission and hangcheck

[Chris Wilson]

On Wed, Jan 13, 2016 at 07:04:53PM +0200, Mika Kuoppala wrote:
> Hangcheck is run on irq context and might be active on a

s/irq/process/ that we pretend is irq-like.

> completely different CPU that is submitting requests. And as
> we have been very careful not to add locking to hangcheck to guard
> against driver failures, we need to be careful with the coherency.
> 
> Update ring last seqno and add to request lists early, with
> write memory barrier, before pushing the request to ring. On
> hangcheck side, use read memory barrier before inspecting the ring
> last seqno. This ensures that when hangcheck is inspecting the
> state, it will not see a active ring without requests in it,
> and then falsely report it as a missed irq situation.

You are just moving the race, not closing it afaict.

>  ring_idle(struct intel_engine_cs *ring, u32 seqno)
>  {
> -	return (list_empty(&ring->request_list) ||
> -		i915_seqno_passed(seqno, ring->last_submitted_seqno));
> +	rmb(); /* Ordering against __i915_add_request() */
> +	return (i915_seqno_passed(seqno, ring->last_submitted_seqno)
> +		|| list_empty(&ring->request_list));

But we can get rid of list_empty()!

If we just embrace the race, we can do this simply by checking for the
same idle waiter twice.

http://lists.freedesktop.org/archives/intel-gfx/2016-January/084514.html

which is the most recent variant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ success: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 058740f8fced6851aeda34f366f5330322cd585f drm-intel-nightly: 2016y-01m-13d-17h-07m-44s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (bdw-nuci7)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
ilk-hp8440p      total:141  pass:101  dwarn:3   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1176/

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