[Intel-gfx] [PATCH 04/17] drm/i915: Tweak scheduler's kick_submission()

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-03-10 10:07:33)
> 
> On 06/03/2020 13:38, Chris Wilson wrote:
> > Skip useless priority bumping on adding a new dependency, but otherwise
> > prevent tasklet scheduling until we have completed all the potential
> > rescheduling.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_scheduler.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> > index 52f71e83e088..603cba36d6a4 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.c
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> > @@ -209,6 +209,8 @@ static void kick_submission(struct intel_engine_cs *engine,
> >       if (!inflight)
> >               goto unlock;
> >   
> > +     engine->execlists.queue_priority_hint = prio;
> > +
> 
> What is the significance of moving this up? I couldn't correlate it to 
> the commit message.

It's correcting the priority bumping. If we have the same context as
active, we should not be skipping the hint update and so avoid the useless
bump on a later dependency.

> >       /*
> >        * If we are already the currently executing context, don't
> >        * bother evaluating if we should preempt ourselves.
> > @@ -216,7 +218,6 @@ static void kick_submission(struct intel_engine_cs *engine,
> >       if (inflight->context == rq->context)
> >               goto unlock;
> >   
> > -     engine->execlists.queue_priority_hint = prio;
> >       if (need_preempt(prio, rq_prio(inflight)))
> >               tasklet_hi_schedule(&engine->execlists.tasklet);
> >   
> > @@ -463,11 +464,15 @@ int i915_sched_node_add_dependency(struct i915_sched_node *node,
> >       if (!dep)
> >               return -ENOMEM;
> >   
> > +     local_bh_disable();
> > +
> >       if (!__i915_sched_node_add_dependency(node, signal, dep,
> >                                             I915_DEPENDENCY_EXTERNAL |
> >                                             I915_DEPENDENCY_ALLOC))
> >               i915_dependency_free(dep);
> >   
> > +     local_bh_enable(); /* kick submission tasklet */
> > +
> 
> And this presumably postpones the tasklet until __bump_priority -> 
> __i915_schedule is finished. But then why the request submission path 
> which calls __i915_sched_node_add_dependency directly does not need this 
> treatment?

Because we haven't completed the updates by that point, and upon actual
request submission the tasklet is flushed. Plus on not all request
submission paths would it be legal.
-Chris