[Intel-gfx] [PATCH 13/37] drm/i915: Priority boost switching to an idle ring

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-06-29 11:08:48)
> 
> On 29/06/2018 08:53, Chris Wilson wrote:
> > In order to maximise concurrency between engines, if we queue a request
> > to a current idle ring, reorder its dependencies to execute that request
> > as early as possible and ideally improve occupancy of multiple engines
> > simultaneously.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_request.c     | 6 +++++-
> >   drivers/gpu/drm/i915/i915_scheduler.h   | 5 +++--
> >   drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++++++
> >   3 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 2d7a785dd88c..d618e7127e88 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1053,7 +1053,8 @@ void i915_request_add(struct i915_request *request)
> >       local_bh_disable();
> >       rcu_read_lock(); /* RCU serialisation for set-wedged protection */
> >       if (engine->schedule) {
> > -             struct i915_sched_attr attr = request->gem_context->sched;
> > +             struct i915_gem_context *ctx = request->gem_context;
> > +             struct i915_sched_attr attr = ctx->sched;
> >   
> >               /*
> >                * Boost priorities to new clients (new request flows).
> > @@ -1064,6 +1065,9 @@ void i915_request_add(struct i915_request *request)
> >               if (!prev || i915_request_completed(prev))
> >                       attr.priority |= I915_PRIORITY_NEWCLIENT;
> >   
> > +             if (intel_engine_queue_is_empty(engine))
> > +                     attr.priority |= I915_PRIORITY_STALL;
> > +
> >               engine->schedule(request, &attr);
> >       }
> >       rcu_read_unlock();
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> > index e9fb6c1d5e42..be132ceb83d9 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.h
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> > @@ -19,13 +19,14 @@ enum {
> >       I915_PRIORITY_INVALID = INT_MIN
> >   };
> >   
> > -#define I915_USER_PRIORITY_SHIFT 1
> > +#define I915_USER_PRIORITY_SHIFT 2
> >   #define I915_USER_PRIORITY(x) ((x) << I915_USER_PRIORITY_SHIFT)
> >   
> >   #define I915_PRIORITY_COUNT BIT(I915_USER_PRIORITY_SHIFT)
> >   #define I915_PRIORITY_MASK (-I915_PRIORITY_COUNT)
> >   
> > -#define I915_PRIORITY_NEWCLIENT BIT(0)
> > +#define I915_PRIORITY_NEWCLIENT BIT(1)
> > +#define I915_PRIORITY_STALL BIT(0)
> 
> So lower priority than new clients.
> 
> Again I have big concerns.
> 
> A client which happens to be the only one using some exotic engine would 
> now be able to trash everything else running on the system. Just because 
> so it happens no one else uses its favourite engine. And that's 
> regardless how much work it has queued up on other, potentially busy, 
> engines.

Within the same priority level, below others, but just above steady
state. Because of that the starvation issue here is no worse than at
current.
-Chris