On Tue, Mar 20, 2018 at 05:17:34PM -0700, Jeff McGee wrote:
> On Tue, Mar 20, 2018 at 12:18:48AM +0000, 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 <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > CC: Michel Thierry <michel.thierry@xxxxxxxxx>
> > Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_lrc.c | 355 ++++++++++++++++++++++-----------------
> > 1 file changed, 197 insertions(+), 158 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index e5a3ffbc273a..beb81f13a3cc 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -828,186 +828,192 @@ 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 * const port = execlists->port;
> > - struct drm_i915_private *dev_priv = engine->i915;
> > + struct drm_i915_private *i915 = engine->i915;
> > + unsigned int head, tail;
> > + const u32 *buf;
> > 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);
> > + if (unlikely(execlists->csb_use_mmio)) {
> > + buf = (u32 * __force)
> > + (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0)));
> > + execlists->csb_head = -1; /* force mmio read of CSB ptrs */
> > + } else {
> > + /* The HWSP contains a (cacheable) mirror of the CSB */
> > + buf = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> > + }
> >
> > - /* 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).
> > + /*
> > + * The write will be ordered by the uncached read (itself
> > + * a memory barrier), so we do not need another in the form
> > + * of a locked instruction. The race between the interrupt
> > + * handler and the split test/clear is harmless as we order
> > + * our clear before the CSB read. If the interrupt arrived
> > + * first between the test and the clear, we read the updated
> > + * CSB and clear the bit. If the interrupt arrives as we read
> > + * the CSB or later (i.e. after we had cleared the bit) the bit
> > + * is set and we do a new loop.
> > */
> > - while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)) {
> > - /* The HWSP contains a (cacheable) mirror of the CSB */
> > - const u32 *buf =
> > - &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> > - unsigned int head, tail;
> > -
> > - 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 */
> > - }
> > + __clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> > + if (unlikely(execlists->csb_head == -1)) { /* following a reset */
> > + intel_uncore_forcewake_get(i915, execlists->fw_domains);
> > + fw = true;
> > +
> > + 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(i915) -
> > + I915_HWS_CSB_BUF0_INDEX;
> > +
> > + head = execlists->csb_head;
> > + tail = READ_ONCE(buf[write_idx]);
> > + }
> > + GEM_TRACE("%s cs-irq head=%d [%d%s], tail=%d [%d%s]\n",
> > + engine->name,
> > + head, GEN8_CSB_READ_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?",
> > + tail, GEN8_CSB_WRITE_PTR(readl(i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)))), fw ? "" : "?");
> > +
> > + while (head != tail) {
> > + struct i915_request *rq;
> > + unsigned int status;
> > + unsigned int count;
> > +
> > + if (++head == GEN8_CSB_ENTRIES)
> > + head = 0;
> >
> > - /* The write will be ordered by the uncached read (itself
> > - * a memory barrier), so we do not need another in the form
> > - * of a locked instruction. The race between the interrupt
> > - * handler and the split test/clear is harmless as we order
> > - * our clear before the CSB read. If the interrupt arrived
> > - * first between the test and the clear, we read the updated
> > - * CSB and clear the bit. If the interrupt arrives as we read
> > - * the CSB or later (i.e. after we had cleared the bit) the bit
> > - * is set and we do a new loop.
> > + /*
> > + * We are flying near dragons again.
> > + *
> > + * We hold a reference to the request in execlist_port[]
> > + * but no more than that. We are operating in softirq
> > + * context and so cannot hold any mutex or sleep. That
> > + * prevents us stopping the requests we are processing
> > + * in port[] from being retired simultaneously (the
> > + * breadcrumb will be complete before we see the
> > + * context-switch). As we only hold the reference to the
> > + * request, any pointer chasing underneath the request
> > + * is subject to a potential use-after-free. Thus we
> > + * store all of the bookkeeping within port[] as
> > + * required, and avoid using unguarded pointers beneath
> > + * request itself. The same applies to the atomic
> > + * status notifier.
> > */
> > - __clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> > - if (unlikely(execlists->csb_head == -1)) { /* following a reset */
> > - if (!fw) {
> > - intel_uncore_forcewake_get(dev_priv,
> > - execlists->fw_domains);
> > - fw = true;
> > - }
> >
> > - head = readl(dev_priv->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) -
> > - I915_HWS_CSB_BUF0_INDEX;
> > + status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
> > + GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
> > + engine->name, head,
> > + status, buf[2*head + 1],
> > + execlists->active);
> > +
> > + 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;
> >
> > - head = execlists->csb_head;
> > - tail = READ_ONCE(buf[write_idx]);
> > - }
> > - 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 ? "" : "?");
> > + /* We should never get a COMPLETED | IDLE_ACTIVE! */
> > + GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
> >
> > - while (head != tail) {
> > - struct i915_request *rq;
> > - unsigned int status;
> > - unsigned int count;
> > + if (status & GEN8_CTX_STATUS_COMPLETE &&
> > + buf[2*head + 1] == execlists->preempt_complete_status) {
> > + GEM_TRACE("%s preempt-idle\n", engine->name);
> > + complete_preempt_context(execlists);
> > + continue;
> > + }
> >
> > - if (++head == GEN8_CSB_ENTRIES)
> > - head = 0;
> > + if (status & GEN8_CTX_STATUS_PREEMPTED &&
> > + execlists_is_active(execlists,
> > + EXECLISTS_ACTIVE_PREEMPT))
> > + continue;
> >
> > - /* We are flying near dragons again.
> > - *
> > - * We hold a reference to the request in execlist_port[]
> > - * but no more than that. We are operating in softirq
> > - * context and so cannot hold any mutex or sleep. That
> > - * prevents us stopping the requests we are processing
> > - * in port[] from being retired simultaneously (the
> > - * breadcrumb will be complete before we see the
> > - * context-switch). As we only hold the reference to the
> > - * request, any pointer chasing underneath the request
> > - * is subject to a potential use-after-free. Thus we
> > - * store all of the bookkeeping within port[] as
> > - * required, and avoid using unguarded pointers beneath
> > - * request itself. The same applies to the atomic
> > - * status notifier.
> > - */
> > + GEM_BUG_ON(!execlists_is_active(execlists,
> > + EXECLISTS_ACTIVE_USER));
> >
> > - status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
> > - GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
> > - engine->name, head,
> > - status, buf[2*head + 1],
> > - execlists->active);
> > -
> > - 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;
> > + rq = port_unpack(port, &count);
> > + GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x, prio=%d\n",
> > + engine->name,
> > + port->context_id, count,
> > + rq ? rq->global_seqno : 0,
> > + rq ? rq_prio(rq) : 0);
> > +
> > + /* Check the context/desc id for this event matches */
> > + GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
> > +
> > + 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_request_completed(rq));
> > + execlists_context_schedule_out(rq);
> > + trace_i915_request_out(rq);
> > + i915_request_put(rq);
> > +
> > + GEM_TRACE("%s completed ctx=%d\n",
> > + engine->name, port->context_id);
> > +
> > + execlists_port_complete(execlists, port);
> > + } else {
> > + port_set(port, port_pack(rq, count));
> > + }
> >
> > - /* We should never get a COMPLETED | IDLE_ACTIVE! */
> > - GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
> > + /* 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 (status & GEN8_CTX_STATUS_COMPLETE &&
> > - buf[2*head + 1] == execlists->preempt_complete_status) {
> > - GEM_TRACE("%s preempt-idle\n", engine->name);
> > - complete_preempt_context(execlists);
> > - continue;
> > - }
> > + if (head != execlists->csb_head) {
> > + execlists->csb_head = head;
> > + writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> > + i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> > + }
> >
> > - if (status & GEN8_CTX_STATUS_PREEMPTED &&
> > - execlists_is_active(execlists,
> > - EXECLISTS_ACTIVE_PREEMPT))
> > - continue;
> > + if (unlikely(fw))
> > + intel_uncore_forcewake_put(i915, execlists->fw_domains);
> > +}
> >
> > - GEM_BUG_ON(!execlists_is_active(execlists,
> > - EXECLISTS_ACTIVE_USER));
> > -
> > - rq = port_unpack(port, &count);
> > - GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x, prio=%d\n",
> > - engine->name,
> > - port->context_id, count,
> > - rq ? rq->global_seqno : 0,
> > - rq ? rq_prio(rq) : 0);
> > -
> > - /* Check the context/desc id for this event matches */
> > - GEM_DEBUG_BUG_ON(buf[2 * head + 1] != port->context_id);
> > -
> > - 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_request_completed(rq));
> > - execlists_context_schedule_out(rq);
> > - trace_i915_request_out(rq);
> > - i915_request_put(rq);
> > -
> > - GEM_TRACE("%s completed ctx=%d\n",
> > - engine->name, port->context_id);
> > -
> > - execlists_port_complete(execlists, port);
> > - } else {
> > - port_set(port, port_pack(rq, count));
> > - }
> > +/*
> > + * 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)
> > +{
> > + struct intel_engine_cs * const engine = (struct intel_engine_cs *)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);
> > - }
> > + /*
> > + * 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(!engine->i915->gt.awake);
> >
> > - if (head != execlists->csb_head) {
> > - execlists->csb_head = head;
> > - writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> > - dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> > - }
> > - }
> > + /*
> > + * 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).
> > + */
> > + if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
> This was a 'while' before refactoring. Shouldn't it still be in order to catch
> new irq_posted during processing?
>
> BTW, I have revised and rebased the force preemption patches on drm-tip with
> this and the other related patches you have proposed. I just started running
> my IGT test that covers the innocent context on port[1] scenario that we
> discussed on IRC. Getting a sporadic failure but need more time to root cause.
> Will update soon.
>
> > + process_csb(engine);
> >
> > - if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
> > + if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))
> > execlists_dequeue(engine);
> > -
> > - if (fw)
> > - intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
> > }
> >
> > static void queue_request(struct intel_engine_cs *engine,
> > @@ -1667,6 +1673,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;
> >
> > /*
> > * Prevent request submission to the hardware until we have
> > @@ -1688,7 +1695,39 @@ execlists_reset_prepare(struct intel_engine_cs *engine)
> > tasklet_kill(&execlists->tasklet);
> > tasklet_disable(&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.
> > + */
> > + 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 is 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;
Now we can see an abort of the reset if process_csb() processes a completed
preemption. So we need to kick the tasklet to get a dequeue of the waiting
request to happen. Currently we only kick if needed in gen8_init_common_ring
which is skipped if we abort the reset.
Otherwise this is working well in my force preemption tests.
-Jeff
> > }
> >
> > static void reset_irq(struct intel_engine_cs *engine)
> > --
> > 2.16.2
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
