[Intel-gfx] [PATCH 2/2] drm/i915/gt: Suppress internal I915_PRIORITY_WAIT for timeslicing

[Chris Wilson]

Quoting Chris Wilson (2020-05-06 15:36:16)
> Make sure we ignore the I915_PRIORITY_WAIT hint when looking at
> timeslicing, as we do not treat it as a preemption request but as a soft
> ordering hint. If we apply the hint, then when we recompute the ordering
> after unwinding for the timeslice, we will often leave the order
> unchanged due to the soft-hint. However, if we apply it to all those we
> unwind, then the two equivalent levels may be reordered, and since the
> dependencies will be replayed in order, we will not change the order of
> dependencies.
> 
> There is a small issue with the lack of cross-engine priority bumping on
> unwind, leaving the total graph slightly unordered; but that will not
> result in any misordering of rendering on remote machines as any
> signalers will also be live. Though there may be a danger that this will
> upset our sanitychecks.
> 
> Why keep the I915_PRIORITY_WAIT soft-hint, I hear Tvrtko ask? Despite
> the many hairy tricks we play to have the hint and then ignore it, I
> still like the concept of codel and the promise that it gives for low
> latency of independent queues!
> 
> Testcase: igt/gem_exec_fence/submit
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 10109f661bcb..3606a7946707 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -414,6 +414,12 @@ static inline int rq_prio(const struct i915_request *rq)
>         return READ_ONCE(rq->sched.attr.priority);
>  }
>  
> +static int __effective_prio(int prio)
> +{
> +       BUILD_BUG_ON(__NO_PREEMPTION & ~I915_PRIORITY_MASK); /* only internal */
> +       return prio | __NO_PREEMPTION;
> +}
> +
>  static int effective_prio(const struct i915_request *rq)
>  {
>         int prio = rq_prio(rq);
> @@ -439,8 +445,7 @@ static int effective_prio(const struct i915_request *rq)
>                 prio |= I915_PRIORITY_NOSEMAPHORE;
>  
>         /* Restrict mere WAIT boosts from triggering preemption */
> -       BUILD_BUG_ON(__NO_PREEMPTION & ~I915_PRIORITY_MASK); /* only internal */
> -       return prio | __NO_PREEMPTION;
> +       return __effective_prio(prio);
>  }
>  
>  static int queue_prio(const struct intel_engine_execlists *execlists)
> @@ -1126,6 +1131,7 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>                         continue; /* XXX */
>  
>                 __i915_request_unsubmit(rq);
> +               rq->sched.attr.priority |= __NO_PREEMPTION;

And this doesn't work as it gives special consideration to anything that
made it to the GPU, stays on the GPU. That is we then cannot select
something from the top of the queue that deserves to be run.
-Chris