Re: [PATCH 2/7] drm/i915: Keep a count of requests waiting for a slot on GPU

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2018-04-05 13:39:18)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Keep a per-engine number of runnable (waiting for GPU time) requests.
> 
> v2:
>  * Move queued increment from insert_request to execlist_submit_request to
>    avoid bumping when re-ordering for priority.
>  * Support the counter on the ringbuffer submission path as well, albeit
>    just notionally. (Chris Wilson)
> 
> v3:
>  * Rebase.
> 
> v4:
>  * Rename and move the stats into a container structure. (Chris Wilson)
> 
> v5:
>  * Re-order fields in struct intel_engine_cs. (Chris Wilson)
> 
> v6-v8:
>  * Rebases.
> 
> v9:
>  * Fix accounting during wedging.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 1 +
>  drivers/gpu/drm/i915/i915_request.c     | 7 +++++++
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 5 +++--
>  drivers/gpu/drm/i915/intel_lrc.c        | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++++++
>  5 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9650a7b10c5f..63f334d5f7fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3211,6 +3211,7 @@ static void nop_complete_submit_request(struct i915_request *request)
>         dma_fence_set_error(&request->fence, -EIO);
>  
>         spin_lock_irqsave(&request->engine->timeline->lock, flags);
> +       request->engine->request_stats.runnable++;
>         __i915_request_submit(request);
>         intel_engine_init_global_seqno(request->engine, request->global_seqno);
>         spin_unlock_irqrestore(&request->engine->timeline->lock, flags);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 585242831974..5c01291ad1cc 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -540,6 +540,9 @@ void __i915_request_submit(struct i915_request *request)
>         /* Transfer from per-context onto the global per-engine timeline */
>         move_to_timeline(request, engine->timeline);
>  
> +       GEM_BUG_ON(engine->request_stats.runnable == 0);
> +       engine->request_stats.runnable--;
> +
>         trace_i915_request_execute(request);
>  
>         wake_up_all(&request->execute);
> @@ -553,6 +556,8 @@ void i915_request_submit(astruct i915_request *request)
>         /* Will be called from irq-context when using foreign fences. */
>         spin_lock_irqsave(&engine->timeline->lock, flags);
>  
> +       engine->request_stats.runnable++;

Hmm, I was thinking this should be in submit_notify(), as you want to
count from when all fences are signaled.

But you are using the timeline lock as its guard?

The only downside is having to repeat the inc in each path. And with the
slight disparity for unsubmit. Not a blocker, just had to actually think
about what you were doing, so maybe discuss that upfront in the commit
msg.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx