[Intel-gfx] [PATCH] drm/i915/gt: Flush submission tasklet before waiting/retiring

[Summers, Stuart]

On Tue, 2019-10-08 at 15:56 +0100, Chris Wilson wrote:
> Quoting Summers, Stuart (2019-10-08 15:52:15)
> > On Tue, 2019-10-08 at 11:56 +0100, Chris Wilson wrote:
> > > A common bane of ours is arbitrary delays in ksoftirqd processing
> > > our
> > > submission tasklet. Give the submission tasklet a kick before we
> > > wait
> > > to
> > > avoid those delays eating into a tight timeout.
> > > 
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

Reviewed-by: Stuart Summers <stuart.summers at intel.com>

Ok, thanks for both of these responses. I agree with what you have here
in that case.

> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_engine.h      |  3 +-
> > >  drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 33 +++++++++++++
> > > ----
> > > ----
> > >  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 12 ++++++++
> > >  3 files changed, 34 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> > > b/drivers/gpu/drm/i915/gt/intel_engine.h
> > > index c9e8c8ccbd47..d624752f2a92 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> > > @@ -407,8 +407,9 @@ static inline void
> > > __intel_engine_reset(struct
> > > intel_engine_cs *engine,
> > >       engine->serial++; /* contexts lost */
> > >  }
> > >  
> > > -bool intel_engine_is_idle(struct intel_engine_cs *engine);
> > >  bool intel_engines_are_idle(struct intel_gt *gt);
> > > +bool intel_engine_is_idle(struct intel_engine_cs *engine);
> > > +void intel_engine_flush_submission(struct intel_engine_cs
> > > *engine);
> > >  
> > >  void intel_engines_reset_default_submission(struct intel_gt
> > > *gt);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > index 6220b7151bb9..7e2aa7a6bef0 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > > @@ -1040,6 +1040,25 @@ static bool ring_is_idle(struct
> > > intel_engine_cs *engine)
> > >       return idle;
> > >  }
> > >  
> > > +void intel_engine_flush_submission(struct intel_engine_cs
> > > *engine)
> > > +{
> > > +     struct tasklet_struct *t = &engine->execlists.tasklet;
> > > +
> > > +     if (__tasklet_is_scheduled(t)) {
> > > +             local_bh_disable();
> > > +             if (tasklet_trylock(t)) {
> > > +                     /* Must wait for any GPU reset in progress.
> > > */
> > > +                     if (__tasklet_is_enabled(t))
> > > +                             t->func(t->data);
> > > +                     tasklet_unlock(t);
> > > +             }
> > > +             local_bh_enable();
> > > +     }
> > > +
> > > +     /* Otherwise flush the tasklet if it was running on another
> > > cpu
> > > */
> > > +     tasklet_unlock_wait(t);
> > > +}
> > > +
> > >  /**
> > >   * intel_engine_is_idle() - Report if the engine has finished
> > > process all work
> > >   * @engine: the intel_engine_cs
> > > @@ -1058,21 +1077,9 @@ bool intel_engine_is_idle(struct
> > > intel_engine_cs *engine)
> > >  
> > >       /* Waiting to drain ELSP? */
> > >       if (execlists_active(&engine->execlists)) {
> > > -             struct tasklet_struct *t = &engine-
> > > >execlists.tasklet;
> > > -
> > >               synchronize_hardirq(engine->i915->drm.pdev->irq);
> > >  
> > > -             local_bh_disable();
> > > -             if (tasklet_trylock(t)) {
> > > -                     /* Must wait for any GPU reset in progress.
> > > */
> > > -                     if (__tasklet_is_enabled(t))
> > > -                             t->func(t->data);
> > > -                     tasklet_unlock(t);
> > > -             }
> > > -             local_bh_enable();
> > > -
> > > -             /* Otherwise flush the tasklet if it was on another
> > > cpu
> > > */
> > > -             tasklet_unlock_wait(t);
> > > +             intel_engine_flush_submission(engine);
> > >  
> > >               if (execlists_active(&engine->execlists))
> > >                       return false;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > index ca606b79fd5e..cbb4069b11e1 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > @@ -4,6 +4,7 @@
> > >   * Copyright © 2019 Intel Corporation
> > >   */
> > >  
> > > +#include "i915_drv.h" /* for_each_engine() */
> > >  #include "i915_request.h"
> > >  #include "intel_gt.h"
> > >  #include "intel_gt_pm.h"
> > > @@ -19,6 +20,15 @@ static void retire_requests(struct
> > > intel_timeline
> > > *tl)
> > >                       break;
> > >  }
> > >  
> > > +static void flush_submission(struct intel_gt *gt)
> > > +{
> > > +     struct intel_engine_cs *engine;
> > > +     enum intel_engine_id id;
> > > +
> > > +     for_each_engine(engine, gt->i915, id)
> > > +             intel_engine_flush_submission(engine);
> > > +}
> > > +
> > >  long intel_gt_retire_requests_timeout(struct intel_gt *gt, long
> > > timeout)
> > >  {
> > >       struct intel_gt_timelines *timelines = &gt->timelines;
> > > @@ -32,6 +42,8 @@ long intel_gt_retire_requests_timeout(struct
> > > intel_gt *gt, long timeout)
> > >       if (unlikely(timeout < 0))
> > >               timeout = -timeout, interruptible = false;
> > >  
> > > +     flush_submission(gt); /* kick the ksoftirqd tasklets */
> > 
> > Won't this add a performance hit if we are doing this across all
> > engines? Is there a way we can isolate this a bit more?
> 
> It's a global wait, and it is just the cost of running the interrupt
> handler once. We have fundamental problems if that runs for more than
> a
> microsecond.
> -Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20191008/64755035/attachment.bin>