Re: [PATCH 2/2] drm/i915: Filter out spurious execlists context-switch interrupts

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2017-10-20 14:59:44)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Back in commit a4b2b01523a8 ("drm/i915: Don't mark an execlists
> > context-switch when idle") we noticed the presence of late
> > context-switch interrupts. We were able to filter those out by looking
> > at whether the ELSP remained active, but in commit beecec901790
> > ("drm/i915/execlists: Preemption!") that became problematic as we now
> > anticipate receiving a context-switch event for preemption while ELSP
> > may be empty. To restore the spurious interrupt suppression, add a
> > counter for the expected number of pending context-switches and skip if
> > we do not need to handle this interrupt to make forward progress.
> >
> > Fixes: beecec901790 ("drm/i915/execlists: Preemption!")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++-----
> >  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++--
> >  drivers/gpu/drm/i915/intel_engine_cs.c     |  5 +++--
> >  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |  7 +++++++
> >  5 files changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index a2e8114b739d..c22d0a07467c 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -550,11 +550,9 @@ static void port_assign(struct execlist_port *port,
> >                       struct drm_i915_gem_request *rq)
> >  {
> >       GEM_BUG_ON(rq == port_request(port));
> > +     GEM_BUG_ON(port_isset(port));
> >  
> > -     if (port_isset(port))
> > -             i915_gem_request_put(port_request(port));
> > -
> > -     port_set(port, port_pack(i915_gem_request_get(rq), port_count(port)));
> > +     port_set(port, i915_gem_request_get(rq));
> 
> No lite restore with guc so we can make this more strict and lean?

Yes. We are not coalescing requests into an active slot, so we always
know that we are assigning a new request into a new slot.

> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a47a9c6bea52..8aecbc321786 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1548,8 +1548,8 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> >       if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
> >               return false;
> >  
> > -     /* Both ports drained, no more ELSP submission? */
> > -     if (port_request(&engine->execlists.port[0]))
> > +     /* Waiting to drain ELSP? */
> > +     if (READ_ONCE(engine->execlists.active))
> >               return false;
> >
> 
> It would not make sense now to keep the port[0] check but
> we could get more fine grained debugging info if we keep
> it?

This function is the antithesis of fine grained debugging. It is called
in a tight loop, watching for idle so reporting from here ends up with a
lot of noise. Often I ask exactly what isn't idle... Now that we do a
single upfront wait, and then a check all is well, moving this to
intel_engine_park where we can be more verbose in the one-off state
check is the next step.

The importance of making this change is so the gen8_cs_irq_handler() and
the idle_worker are in sync about when to drop the GT wakeref.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx