Re: [Intel-gfx] [PATCH 6/7] drm/i915/execlists: Flush pending preemption events during reset
On 16/05/2018 16:12, Chris Wilson wrote:
Catch up with the inflight CSB events, after disabling the tasklet
before deciding which request was truly guilty of hanging the GPU.
Signed-off-by: Chris Wilson
Cc: Michał Winiarski
CC: Michel Thierry
Cc: Jeff McGee
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/intel_lrc.c | 36 +++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e70d8d624899..3b889bb4352a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1843,6 +1843,7 @@ static struct i915_request *
execlists_reset_prepare(struct intel_engine_cs *engine)
{
struct intel_engine_execlists * const execlists = &engine->execlists;
+ struct i915_request *request, *active;
GEM_TRACE("%s\n", engine->name);
@@ -1857,7 +1858,40 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
*/
__tasklet_disable_sync_once(&execlists->tasklet);
- return i915_gem_find_active_request(engine);
+ /*
+* We want to flush the pending context switches, having disabled
+* the tasklet above, we can assume exclusive access to the execlists.
+* For this allows us to catch up with an inflight preemption event,
+* and avoid blaming an innocent request if the stall was due to the
+* preemption itself.
+*/
+ if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
+ process_csb(engine);
+
+ /*
+* The last active request can then be no later than the last request
+* now in ELSP[0]. So search backwards from there, so that if the GPU
+* has advanced beyond the last CSB update, it will be pardoned.
+*/
+ active = NULL;
+ request = port_request(execlists->port);
+ if (request) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&engine->timeline.lock, flags);
+ list_for_each_entry_from_reverse(request,
+&engine->timeline.requests,
+link) {
+ if (__i915_request_completed(request,
+request->global_seqno))
+ break;
+
+ active = request;
+ }
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+ }
+
+ return active;
}
static void execlists_reset(struct intel_engine_cs *engine,
Sounds sensible to me, just don't ask me to describe preemption and
reset flow where things go bad without it. :)
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915/execlists: Flush pending preemption events during reset
Quoting Tvrtko Ursulin (2018-05-16 15:24:33)
> > + /*
> > + * The last active request can then be no later than the last request
> > + * now in ELSP[0]. So search backwards from there, so that if the GPU
> > + * has advanced beyond the last CSB update, it will be pardoned.
> > + */
> > + active = NULL;
> > + request = port_request(execlists->port);
> > + if (request) {
>
> Assignment to request is for nothing it seems.
>
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&engine->timeline.lock, flags);
> > + list_for_each_entry_from_reverse(request,
It's a 'from' list_for_each variant.
> > + &engine->timeline.requests,
> > + link) {
> > + if (__i915_request_completed(request,
> > + request->global_seqno))
> > + break;
> > +
> > + active = request;
> > + }
> > + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > + }
> > +
> > + return active;
> > }
> >
> > static void execlists_reset(struct intel_engine_cs *engine,
> >
>
> No other complaints and I could be bribed to look past the request to
> split it. :)
Is not clearing the backlog so we can get onto features not enough
incentive? Chocolate? Beer? Chocolate-flavoured beer?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915/execlists: Flush pending preemption events during reset
On 16/05/2018 07:47, Chris Wilson wrote:
Catch up with the inflight CSB events, after disabling the tasklet
before deciding which request was truly guilty of hanging the GPU.
v2: Restore checking of use_csb_mmio on every loop, don't forget old
vgpu.
Signed-off-by: Chris Wilson
Cc: Michał Winiarski
CC: Michel Thierry
Cc: Jeff McGee
---
drivers/gpu/drm/i915/intel_lrc.c | 127 +--
1 file changed, 87 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7afe52fa615d..cae10ebf9432 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -957,34 +957,14 @@ static void execlists_cancel_requests(struct
intel_engine_cs *engine)
local_irq_restore(flags);
}
-/*
- * Check the unread Context Status Buffers and manage the submission of new
- * contexts to the ELSP accordingly.
- */
-static void execlists_submission_tasklet(unsigned long data)
+static void process_csb(struct intel_engine_cs *engine)
{
- struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
struct intel_engine_execlists * const execlists = &engine->execlists;
struct execlist_port *port = execlists->port;
- struct drm_i915_private *dev_priv = engine->i915;
+ struct drm_i915_private *i915 = engine->i915;
bool fw = false;
- /*
-* We can skip acquiring intel_runtime_pm_get() here as it was taken
-* on our behalf by the request (see i915_gem_mark_busy()) and it will
-* not be relinquished until the device is idle (see
-* i915_gem_idle_work_handler()). As a precaution, we make sure
-* that all ELSP are drained i.e. we have processed the CSB,
-* before allowing ourselves to idle and calling intel_runtime_pm_put().
-*/
- GEM_BUG_ON(!dev_priv->gt.awake);
-
- /*
-* Prefer doing test_and_clear_bit() as a two stage operation to avoid
-* imposing the cost of a locked atomic transaction when submitting a
-* new request (outside of the context-switch interrupt).
-*/
- while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
+ do {
/* The HWSP contains a (cacheable) mirror of the CSB */
const u32 *buf =
&engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
@@ -992,28 +972,27 @@ static void execlists_submission_tasklet(unsigned long
data)
if (unlikely(execlists->csb_use_mmio)) {
buf = (u32 * __force)
- (dev_priv->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
- execlists->csb_head = -1; /* force mmio read of CSB
ptrs */
+ (i915->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
+ execlists->csb_head = -1; /* force mmio read of CSB */
}
/* Clear before reading to catch new interrupts */
clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
smp_mb__after_atomic();
- if (unlikely(execlists->csb_head == -1)) { /* following a reset */
+ if (unlikely(execlists->csb_head == -1)) { /* after a reset */
if (!fw) {
- intel_uncore_forcewake_get(dev_priv,
-
execlists->fw_domains);
+ intel_uncore_forcewake_get(i915,
execlists->fw_domains);
fw = true;
}
- head = readl(dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
+ head = readl(i915->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
tail = GEN8_CSB_WRITE_PTR(head);
head = GEN8_CSB_READ_PTR(head);
execlists->csb_head = head;
} else {
const int write_idx =
- intel_hws_csb_write_index(dev_priv) -
+ intel_hws_csb_write_index(i915) -
I915_HWS_CSB_BUF0_INDEX;
head = execlists->csb_head;
@@ -1022,8 +1001,8 @@ static void execlists_submission_tasklet(unsigned long
data)
}
GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
engine->name,
- head, GEN8_CSB_READ_PTR(readl(dev_priv->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine, fw ? "" : "?",
- tail, GEN8_CSB_WRITE_PTR(readl(dev_priv->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine, fw ? "" : "?");
+ head, GEN8_CSB_READ_PTR(readl(i915->regs +
i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine, fw ? "" : "?",
+
