[Intel-gfx] [PATCH v7] drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-06-28 14:21:06)
> 
> On 28/06/2018 14:11, Chris Wilson wrote:
> > +/*
> > + * 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;
> > +     unsigned long flags;
> > +
> > +     GEM_TRACE("%s awake?=%d, active=%x\n",
> > +               engine->name,
> > +               engine->i915->gt.awake,
> > +               engine->execlists.active);
> > +
> > +     spin_lock_irqsave(&engine->timeline.lock, flags);
> > +
> > +     if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
> 
> No races between the check and tasklet call? In other words the code 
> path which you were describing that can race is taking the timeline lock?

intel_engine_is_idle() is unserialised.

> > +             __execlists_submission_tasklet(engine);
> > +
> > +     spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > +}
> > +
> >   static void queue_request(struct intel_engine_cs *engine,
> >                         struct i915_sched_node *node,
> >                         int prio)
> > @@ -1144,16 +1155,30 @@ static void queue_request(struct intel_engine_cs *engine,
> >                     &lookup_priolist(engine, prio)->requests);
> >   }
> >   
> > -static void __submit_queue(struct intel_engine_cs *engine, int prio)
> > +static void __update_queue(struct intel_engine_cs *engine, int prio)
> >   {
> >       engine->execlists.queue_priority = prio;
> > -     tasklet_hi_schedule(&engine->execlists.tasklet);
> > +}
> > +
> > +static void __submit_queue_imm(struct intel_engine_cs *engine)
> > +{
> > +     struct intel_engine_execlists * const execlists = &engine->execlists;
> > +
> > +     if (reset_in_progress(execlists))
> > +             return; /* defer until we restart the engine following reset */
> 
> We have a tasklet kick somewhere in that path?

In execlists_reset_finish()

> > +     if (execlists->tasklet.func == execlists_submission_tasklet)
> > +             __execlists_submission_tasklet(engine);
> > +     else
> > +             tasklet_hi_schedule(&execlists->tasklet);
> >   }
> >   
> >   static void submit_queue(struct intel_engine_cs *engine, int prio)
> >   {
> > -     if (prio > engine->execlists.queue_priority)
> > -             __submit_queue(engine, prio);
> > +     if (prio > engine->execlists.queue_priority) {
> > +             __update_queue(engine, prio);
> > +             __submit_queue_imm(engine);
> > +     }
> >   }
> >   
> >   static void execlists_submit_request(struct i915_request *request)
> > @@ -1165,11 +1190,12 @@ static void execlists_submit_request(struct i915_request *request)
> >       spin_lock_irqsave(&engine->timeline.lock, flags);
> >   
> >       queue_request(engine, &request->sched, rq_prio(request));
> > -     submit_queue(engine, rq_prio(request));
> >   
> >       GEM_BUG_ON(!engine->execlists.first);
> >       GEM_BUG_ON(list_empty(&request->sched.link));
> >   
> > +     submit_queue(engine, rq_prio(request));
> > +
> >       spin_unlock_irqrestore(&engine->timeline.lock, flags);
> >   }
> >   
> > @@ -1296,8 +1322,11 @@ static void execlists_schedule(struct i915_request *request,
> >               }
> >   
> >               if (prio > engine->execlists.queue_priority &&
> > -                 i915_sw_fence_done(&sched_to_request(node)->submit))
> > -                     __submit_queue(engine, prio);
> > +                 i915_sw_fence_done(&sched_to_request(node)->submit)) {
> > +                     /* defer submission until after all of our updates */
> > +                     __update_queue(engine, prio);
> > +                     tasklet_hi_schedule(&engine->execlists.tasklet);
> 
> _imm suffix on __submit_queue_imm sounds unused if there is no plain 
> __submit_queue, which could have been used here. But meh.

I thought of trying to emphasis the immediate nature of this path. It's
not a huge deal, but I didn't particularly like calling it
direct_submit_queue() (or just direct_submit(), too many submits!)
__ prefix to indicate that it's an inner function to submit_queue,
_imm suffix to indicate what's special.
-Chris