[Intel-gfx] [PATCH] drm/i915/gt: Flush submission tasklet before waiting/retiring

[Summers, Stuart]

On Tue, 2019-10-08 at 11:56 +0100, Chris Wilson wrote:
> A common bane of ours is arbitrary delays in ksoftirqd processing our
> submission tasklet. Give the submission tasklet a kick before we wait
> to
> avoid those delays eating into a tight timeout.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine.h      |  3 +-
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 33 +++++++++++++----
> ----
>  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 12 ++++++++
>  3 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> b/drivers/gpu/drm/i915/gt/intel_engine.h
> index c9e8c8ccbd47..d624752f2a92 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -407,8 +407,9 @@ static inline void __intel_engine_reset(struct
> intel_engine_cs *engine,
>  	engine->serial++; /* contexts lost */
>  }
>  
> -bool intel_engine_is_idle(struct intel_engine_cs *engine);
>  bool intel_engines_are_idle(struct intel_gt *gt);
> +bool intel_engine_is_idle(struct intel_engine_cs *engine);
> +void intel_engine_flush_submission(struct intel_engine_cs *engine);
>  
>  void intel_engines_reset_default_submission(struct intel_gt *gt);
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 6220b7151bb9..7e2aa7a6bef0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1040,6 +1040,25 @@ static bool ring_is_idle(struct
> intel_engine_cs *engine)
>  	return idle;
>  }
>  
> +void intel_engine_flush_submission(struct intel_engine_cs *engine)
> +{
> +	struct tasklet_struct *t = &engine->execlists.tasklet;
> +
> +	if (__tasklet_is_scheduled(t)) {
> +		local_bh_disable();
> +		if (tasklet_trylock(t)) {
> +			/* Must wait for any GPU reset in progress. */
> +			if (__tasklet_is_enabled(t))
> +				t->func(t->data);
> +			tasklet_unlock(t);
> +		}
> +		local_bh_enable();
> +	}
> +
> +	/* Otherwise flush the tasklet if it was running on another cpu
> */
> +	tasklet_unlock_wait(t);
> +}
> +
>  /**
>   * intel_engine_is_idle() - Report if the engine has finished
> process all work
>   * @engine: the intel_engine_cs
> @@ -1058,21 +1077,9 @@ bool intel_engine_is_idle(struct
> intel_engine_cs *engine)
>  
>  	/* Waiting to drain ELSP? */
>  	if (execlists_active(&engine->execlists)) {
> -		struct tasklet_struct *t = &engine->execlists.tasklet;
> -
>  		synchronize_hardirq(engine->i915->drm.pdev->irq);
>  
> -		local_bh_disable();
> -		if (tasklet_trylock(t)) {
> -			/* Must wait for any GPU reset in progress. */
> -			if (__tasklet_is_enabled(t))
> -				t->func(t->data);
> -			tasklet_unlock(t);
> -		}
> -		local_bh_enable();
> -
> -		/* Otherwise flush the tasklet if it was on another cpu
> */
> -		tasklet_unlock_wait(t);
> +		intel_engine_flush_submission(engine);
>  
>  		if (execlists_active(&engine->execlists))
>  			return false;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index ca606b79fd5e..cbb4069b11e1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -4,6 +4,7 @@
>   * Copyright © 2019 Intel Corporation
>   */
>  
> +#include "i915_drv.h" /* for_each_engine() */
>  #include "i915_request.h"
>  #include "intel_gt.h"
>  #include "intel_gt_pm.h"
> @@ -19,6 +20,15 @@ static void retire_requests(struct intel_timeline
> *tl)
>  			break;
>  }
>  
> +static void flush_submission(struct intel_gt *gt)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	for_each_engine(engine, gt->i915, id)
> +		intel_engine_flush_submission(engine);
> +}
> +
>  long intel_gt_retire_requests_timeout(struct intel_gt *gt, long
> timeout)
>  {
>  	struct intel_gt_timelines *timelines = &gt->timelines;
> @@ -32,6 +42,8 @@ long intel_gt_retire_requests_timeout(struct
> intel_gt *gt, long timeout)
>  	if (unlikely(timeout < 0))
>  		timeout = -timeout, interruptible = false;
>  
> +	flush_submission(gt); /* kick the ksoftirqd tasklets */

Won't this add a performance hit if we are doing this across all
engines? Is there a way we can isolate this a bit more?

Thanks,
Stuart

> +
>  	spin_lock_irqsave(&timelines->lock, flags);
>  	list_for_each_entry_safe(tl, tn, &timelines->active_list, link)
> {
>  		if (!mutex_trylock(&tl->mutex)) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20191008/bdc23d43/attachment.bin>