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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
