[Intel-gfx] [PATCH 4/5] drm/i915: Downgrade NEWCLIENT to non-preemptive

[Tvrtko Ursulin]

On 15/05/2019 14:00, Chris Wilson wrote:
> Commit 1413b2bc0717 ("drm/i915: Trim NEWCLIENT boosting") had the
> intended consequence of not allowing a sequence of work that merely
> crossed into a new engine the privilege to be promoted to NEWCLIENT

What do you mean with crossed into a new engine? At first I thought the 
statement implies the engine timeline was used to query for previous 
request, but that's not true.

Regards,

Tvrtko

> status. It also had the unintended consequence of actually making
> NEWCLIENT effective on heavily oversubscribed transcode machines and
> impacting upon their throughput.
> 
> If we consider a client packet composed of (rcsA, rcsB, vcs) and 30 of
> those clients, using the NEWCLIENT boost that will be scheduled as
> 
> 	rcsA x 30, (rcsB, vcs) x 30
> 
> where as before it would have been
> 
> 	(rcsA, rcsB, vcs) x 30
> 
> That is with NEWCLIENT only boosting the first request of each client,
> we would execute all rcsA requests prior to running on the vcs engines;
> acruing a lot of dead time as compared to the previous case where the
> vcs engine would be started in parallel to processing the second client.
> 
> The previous patch has the effect of delaying submission until it is
> required by a third party (either the user with an explicit wait, or by
> another client/engine). We reduce the NEWCLIENT bump to a mere WAIT,
> which has the effect of removing its preemptive grant and reducing it to
> the same level as any other user interaction -- that it will not be
> promoted above the interengine dependencies, and so preventing NEWCLIENTS
> from starving other engines. This a large nerf to the rrul properties of
> the current NEWCLIENT, but it still does give prioritised submission to
> new requests from light workloads.
> 
> References: b16c765122f9 ("drm/i915: Priority boost for new clients")
> Fixes: 1413b2bc0717 ("drm/i915: Trim NEWCLIENT boosting") # customer impact
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>
> Cc: Dmitry Ermilov <dmitry.ermilov at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c        | 2 +-
>   drivers/gpu/drm/i915/i915_priolist_types.h | 5 ++---
>   drivers/gpu/drm/i915/i915_request.c        | 2 +-
>   3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index e18623def282..b5e82171df8f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -164,7 +164,7 @@
>   #define WA_TAIL_DWORDS 2
>   #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
>   
> -#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT | I915_PRIORITY_NOSEMAPHORE)
> +#define ACTIVE_PRIORITY (I915_PRIORITY_NOSEMAPHORE)
>   
>   static int execlists_context_deferred_alloc(struct intel_context *ce,
>   					    struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/i915_priolist_types.h b/drivers/gpu/drm/i915/i915_priolist_types.h
> index cc44ebd3b553..49709de69875 100644
> --- a/drivers/gpu/drm/i915/i915_priolist_types.h
> +++ b/drivers/gpu/drm/i915/i915_priolist_types.h
> @@ -20,15 +20,14 @@ enum {
>   	I915_PRIORITY_INVALID = INT_MIN
>   };
>   
> -#define I915_USER_PRIORITY_SHIFT 3
> +#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 - 1)
>   
>   #define I915_PRIORITY_WAIT		((u8)BIT(0))
> -#define I915_PRIORITY_NEWCLIENT		((u8)BIT(1))
> -#define I915_PRIORITY_NOSEMAPHORE	((u8)BIT(2))
> +#define I915_PRIORITY_NOSEMAPHORE	((u8)BIT(1))
>   
>   #define __NO_PREEMPTION (I915_PRIORITY_WAIT)
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 4899195e58c2..2fca0b59578d 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1189,7 +1189,7 @@ struct i915_request *__i915_request_commit(struct i915_request *rq)
>   		 * the bulk clients. (FQ_CODEL)
>   		 */
>   		if (list_empty(&rq->sched.signalers_list))
> -			attr.priority |= I915_PRIORITY_NEWCLIENT;
> +			attr.priority |= I915_PRIORITY_WAIT;
>   
>   		engine->schedule(rq, &attr);
>   	}
>