Re: [Intel-gfx] [PATCH 41/53] drm/i915/bdw: Avoid non-lite-restore preemptions

2014-07-07 Thread Daniel Vetter 
On Mon, Jun 23, 2014 at 11:52:11AM +, Mateo Lozano, Oscar wrote:
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
  Vetter
  Sent: Wednesday, June 18, 2014 9:49 PM
  To: Mateo Lozano, Oscar
  Cc: intel-gfx@lists.freedesktop.org
  Subject: Re: [Intel-gfx] [PATCH 41/53] drm/i915/bdw: Avoid non-lite-restore
  preemptions
  
  On Fri, Jun 13, 2014 at 04:37:59PM +0100, oscar.ma...@intel.com wrote:
   From: Oscar Mateo oscar.ma...@intel.com
  
   In the current Execlists feeding mechanism, full preemption is not
   supported yet: only lite-restores are allowed (this is: the GPU simply
   samples a new tail pointer for the context currently in execution).
  
   But we have identified an scenario in which a full preemption occurs:
   1) We submit two contexts for execution (A  B).
   2) The GPU finishes with the first one (A), switches to the second one
   (B) and informs us.
   3) We submit B again (hoping to cause a lite restore) together with C,
   but in the time we spend writing to the ELSP, the GPU finishes B.
   4) The GPU start executing B again (since we told it so).
   5) We receive a B finished interrupt and, mistakenly, we submit C
   (again) and D, causing a full preemption of B.
  
   By keeping a better track of our submissions, we can avoid the
   scenario described above.
  
  How? I don't see a way to fundamentally avoid the above race, and I don't
  really see an issue with it - the gpu should notice that there's not really 
  any
  work done and then switch to C.
  
  Or am I completely missing the point here?
  
  With no clue at all this looks really scary.
 
 The race is avoided by keeping track of how many times a context has
 been submitted to the hardware and by better discriminating the received
 context switch interrupts: in the example, when we have submitted B
 twice, we won´t submit C and D as soon as we receive the notification
 that B is completed because we were expecting to get a LITE_RESTORE and
 we didn´t, so we know a second completion will be received shortly.
 
 Without this explicit checking, the race condition happens and, somehow,
 the batch buffer execution order gets messed with. This can be verified
 with the IGT test I sent together with the series. I don´t know the
 exact mechanism by which the pre-emption messes with the execution order
 but, since other people is working on the Scheduler + Preemption on
 Execlists, I didn´t try to fix it. In these series, only Lite Restores
 are supported (other kind of preemptions WARN).
 
 I´ll add this clarification to the commit message.

Yeah, please elaborate more clearly in the commit message what exactly
seems to go wrong (and where you're unsure with your model of how the
hardware reacts). And please also reference the precise testcase (and
precise failure mode without this patch) so that if anyone stumbles over
this again we have some breadcrumbs to figure things out. And some stern
warnings as comments in the code to warn the unsuspecting reader.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 41/53] drm/i915/bdw: Avoid non-lite-restore preemptions

2014-06-18 Thread Daniel Vetter 
On Fri, Jun 13, 2014 at 04:37:59PM +0100, oscar.ma...@intel.com wrote:
 From: Oscar Mateo oscar.ma...@intel.com
 
 In the current Execlists feeding mechanism, full preemption is not
 supported yet: only lite-restores are allowed (this is: the GPU
 simply samples a new tail pointer for the context currently in
 execution).
 
 But we have identified an scenario in which a full preemption occurs:
 1) We submit two contexts for execution (A  B).
 2) The GPU finishes with the first one (A), switches to the second one
 (B) and informs us.
 3) We submit B again (hoping to cause a lite restore) together with C,
 but in the time we spend writing to the ELSP, the GPU finishes B.
 4) The GPU start executing B again (since we told it so).
 5) We receive a B finished interrupt and, mistakenly, we submit C (again)
 and D, causing a full preemption of B.
 
 By keeping a better track of our submissions, we can avoid the scenario
 described above.

How? I don't see a way to fundamentally avoid the above race, and I don't
really see an issue with it - the gpu should notice that there's not
really any work done and then switch to C.

Or am I completely missing the point here?

With no clue at all this looks really scary.

 v2: elsp_submitted belongs in the new intel_ctx_submit_request. Several
 rebase changes.
 
 Signed-off-by: Oscar Mateo oscar.ma...@intel.com
 ---
  drivers/gpu/drm/i915/intel_lrc.c | 28 
  drivers/gpu/drm/i915/intel_lrc.h |  2 ++
  2 files changed, 26 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
 b/drivers/gpu/drm/i915/intel_lrc.c
 index 290391c..f388b28 100644
 --- a/drivers/gpu/drm/i915/intel_lrc.c
 +++ b/drivers/gpu/drm/i915/intel_lrc.c
 @@ -248,6 +248,7 @@ static void execlists_context_unqueue(struct 
 intel_engine_cs *ring)
   else if (req0-ctx == cursor-ctx) {
   /* Same ctx: ignore first request, as second request
* will update tail past first request's workload */
 + cursor-elsp_submitted = req0-elsp_submitted;
   list_del(req0-execlist_link);
   queue_work(dev_priv-wq, req0-work);
   req0 = cursor;
 @@ -257,8 +258,14 @@ static void execlists_context_unqueue(struct 
 intel_engine_cs *ring)
   }
   }
  
 + WARN_ON(req1  req1-elsp_submitted);
 +
   BUG_ON(execlists_submit_context(ring, req0-ctx, req0-tail,
   req1? req1-ctx : NULL, req1? req1-tail : 0));

Aside: No BUG_ON except when you can prove that the kernel will die within
the current function anyway. I've seen too many cases where people
sprinkle BUG_ON instead of WARN_ON for not-completely-letal issues with
the argument that stopping the box helps debugging.

That's kinda true for initial development, but not true when shipping: The
usual result is a frustrated user/customer looking at a completely frozen
box (because someone managed to hit the BUG_ON within a spinlock that the
irq handler requires and then the machine is gone) and an equally
frustrated developer half a world away.

A dying kernel that spews useful crap into logs with his last breadth is
_much_ better, even when you know that there's no way we can ever recover
from a given situation.

/rant

Cheers, Daniel

 +
 + req0-elsp_submitted++;
 + if (req1)
 + req1-elsp_submitted++;
  }
  
  static bool execlists_check_remove_request(struct intel_engine_cs *ring,
 @@ -275,9 +282,13 @@ static bool execlists_check_remove_request(struct 
 intel_engine_cs *ring,
   struct drm_i915_gem_object *ctx_obj =
   head_req-ctx-engine[ring-id].obj;
   if (intel_execlists_ctx_id(ctx_obj) == request_id) {
 - list_del(head_req-execlist_link);
 - queue_work(dev_priv-wq, head_req-work);
 - return true;
 + WARN(head_req-elsp_submitted == 0,
 + Never submitted head request\n);
 + if (--head_req-elsp_submitted = 0) {
 + list_del(head_req-execlist_link);
 + queue_work(dev_priv-wq, head_req-work);
 + return true;
 + }
   }
   }
  
 @@ -310,7 +321,16 @@ void intel_execlists_handle_ctx_events(struct 
 intel_engine_cs *ring)
   status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
   (read_pointer % 6) * 8 + 4);
  
 - if (status  GEN8_CTX_STATUS_COMPLETE) {
 + if (status  GEN8_CTX_STATUS_PREEMPTED) {
 + if (status  GEN8_CTX_STATUS_LITE_RESTORE) {
 + if (execlists_check_remove_request(ring, 
 status_id))
 + WARN(1, Lite Restored request removed 
 from queue\n);
 + } else
 +

[Intel-gfx] [PATCH 41/53] drm/i915/bdw: Avoid non-lite-restore preemptions

2014-06-13 Thread oscar . mateo 
From: Oscar Mateo oscar.ma...@intel.com

In the current Execlists feeding mechanism, full preemption is not
supported yet: only lite-restores are allowed (this is: the GPU
simply samples a new tail pointer for the context currently in
execution).

But we have identified an scenario in which a full preemption occurs:
1) We submit two contexts for execution (A  B).
2) The GPU finishes with the first one (A), switches to the second one
(B) and informs us.
3) We submit B again (hoping to cause a lite restore) together with C,
but in the time we spend writing to the ELSP, the GPU finishes B.
4) The GPU start executing B again (since we told it so).
5) We receive a B finished interrupt and, mistakenly, we submit C (again)
and D, causing a full preemption of B.

By keeping a better track of our submissions, we can avoid the scenario
described above.

v2: elsp_submitted belongs in the new intel_ctx_submit_request. Several
rebase changes.

Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
 drivers/gpu/drm/i915/intel_lrc.c | 28 
 drivers/gpu/drm/i915/intel_lrc.h |  2 ++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 290391c..f388b28 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -248,6 +248,7 @@ static void execlists_context_unqueue(struct 
intel_engine_cs *ring)
else if (req0-ctx == cursor-ctx) {
/* Same ctx: ignore first request, as second request
 * will update tail past first request's workload */
+   cursor-elsp_submitted = req0-elsp_submitted;
list_del(req0-execlist_link);
queue_work(dev_priv-wq, req0-work);
req0 = cursor;
@@ -257,8 +258,14 @@ static void execlists_context_unqueue(struct 
intel_engine_cs *ring)
}
}
 
+   WARN_ON(req1  req1-elsp_submitted);
+
BUG_ON(execlists_submit_context(ring, req0-ctx, req0-tail,
req1? req1-ctx : NULL, req1? req1-tail : 0));
+
+   req0-elsp_submitted++;
+   if (req1)
+   req1-elsp_submitted++;
 }
 
 static bool execlists_check_remove_request(struct intel_engine_cs *ring,
@@ -275,9 +282,13 @@ static bool execlists_check_remove_request(struct 
intel_engine_cs *ring,
struct drm_i915_gem_object *ctx_obj =
head_req-ctx-engine[ring-id].obj;
if (intel_execlists_ctx_id(ctx_obj) == request_id) {
-   list_del(head_req-execlist_link);
-   queue_work(dev_priv-wq, head_req-work);
-   return true;
+   WARN(head_req-elsp_submitted == 0,
+   Never submitted head request\n);
+   if (--head_req-elsp_submitted = 0) {
+   list_del(head_req-execlist_link);
+   queue_work(dev_priv-wq, head_req-work);
+   return true;
+   }
}
}
 
@@ -310,7 +321,16 @@ void intel_execlists_handle_ctx_events(struct 
intel_engine_cs *ring)
status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
(read_pointer % 6) * 8 + 4);
 
-   if (status  GEN8_CTX_STATUS_COMPLETE) {
+   if (status  GEN8_CTX_STATUS_PREEMPTED) {
+   if (status  GEN8_CTX_STATUS_LITE_RESTORE) {
+   if (execlists_check_remove_request(ring, 
status_id))
+   WARN(1, Lite Restored request removed 
from queue\n);
+   } else
+   WARN(1, Preemption without Lite Restore\n);
+   }
+
+if ((status  GEN8_CTX_STATUS_ACTIVE_IDLE) ||
+(status  GEN8_CTX_STATUS_ELEMENT_SWITCH)) {
if (execlists_check_remove_request(ring, status_id))
submit_contexts++;
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 7949dff..ee877aa 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -51,6 +51,8 @@ struct intel_ctx_submit_request {
 
struct list_head execlist_link;
struct work_struct work;
+
+   int elsp_submitted;
 };
 
 void intel_execlists_handle_ctx_events(struct intel_engine_cs *ring);
-- 
1.9.0

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