Quoting Jeff McGee (2018-03-22 15:14:01)
> 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.
Yes, it is imperative that the tasklet be disabled and process_csb() be
manually flushed in the preemption timeout (because of ksortirqd we may
not have run the submission tasklet at all before the timer expires).
Hence the desire to split out process_csb().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
