Re: [PATCH v2 5/7] drm/i915/execlists: Direct submit onto idle engines

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

Quoting Tvrtko Ursulin (2018-05-08 11:23:09)
> 
> On 07/05/2018 14:57, Chris Wilson wrote:
> > Bypass using the tasklet to submit the first request to HW, as the
> > tasklet may be deferred unto ksoftirqd and at a minimum will add in
> > excess of 10us (and maybe tens of milliseconds) to our execution
> > latency. This latency reduction is most notable when execution flows
> > between engines.
> > 
> > v2: Beware handling preemption completion from the direct submit path as
> > well.
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_guc_submission.c | 12 +++-
> >   drivers/gpu/drm/i915/intel_lrc.c            | 66 +++++++++++++++++----
> >   drivers/gpu/drm/i915/intel_ringbuffer.h     |  7 +++
> >   3 files changed, 69 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> > index 2feb65096966..6bfe30af7826 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> > @@ -754,14 +754,20 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
> >   
> >   static void guc_dequeue(struct intel_engine_cs *engine)
> >   {
> > -     unsigned long flags;
> > +     unsigned long uninitialized_var(flags);
> >       bool submit;
> >   
> >       local_irq_save(flags);
> >   
> > -     spin_lock(&engine->timeline.lock);
> > +     GEM_BUG_ON(!test_bit(TASKLET_STATE_RUN,
> > +                          &engine->execlists.tasklet.state));
> 
> Soon it will be time for i915_tasklet. :)
> 
> > +     if (!intel_engine_direct_submit(engine))
> > +             spin_lock(&engine->timeline.lock);
> 
> A bit ugly both on the conditional locking and using engine->flags for 
> transient purposes.
> 
> Since you are locking the tasklet and own it (and open coding the call) 
> completely when calling directly, you could just the same cheat and call 
> a different function?

My first attempt was to call __execlists_dequeue() directly and not
tasklet->func(). But that then has this nasty
  if (tasklet->func == execlists_submission_tasklet)
or some such in the middle of otherwise generic code.
https://patchwork.freedesktop.org/patch/221105/

I was less happy about that. At least this does have the making of
something more generic like i915_tasklet ;)

> >       submit = __guc_dequeue(engine);
> > -     spin_unlock(&engine->timeline.lock);
> > +
> > +     if (!intel_engine_direct_submit(engine))
> > +             spin_unlock(&engine->timeline.lock);
> >   
> >       if (submit)
> >               guc_submit(engine);
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 15c373ea5b7e..ac7c5edee4ee 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -357,13 +357,16 @@ execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists)
> >   {
> >       struct intel_engine_cs *engine =
> >               container_of(execlists, typeof(*engine), execlists);
> > -     unsigned long flags;
> > +     unsigned long uninitialized_var(flags);
> >   
> > -     spin_lock_irqsave(&engine->timeline.lock, flags);
> > +     GEM_BUG_ON(!test_bit(TASKLET_STATE_RUN, &execlists->tasklet.state));
> > +     if (!intel_engine_direct_submit(engine))
> > +             spin_lock_irqsave(&engine->timeline.lock, flags);
> >   
> >       __unwind_incomplete_requests(engine);
> >   
> > -     spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > +     if (!intel_engine_direct_submit(engine))
> > +             spin_unlock_irqrestore(&engine->timeline.lock, flags);
> 
> Hm ok yes, this one would be a problem..
> 
> Maybe at least use some bit under execlists state instead of engine flags?

But I have engine->flags :-p Could I steal a bit from tasklet.state? I
tend to get funny looks everytime I ask for TASKLET_STATE_USER ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx