[Intel-gfx] [PATCH v6 1/3] drm/i915: Get active pending request for given context

[Tvrtko Ursulin]

On 26/11/2019 04:51, Ankit Navik wrote:
> This patch gives us the active pending request count which is yet
> to be submitted to the GPU.
> 
> V2:
>   * Change 64-bit to atomic for request count. (Tvrtko Ursulin)
> 
> V3:
>   * Remove mutex for request count.
>   * Rebase.
>   * Fixes hitting underflow for predictive request. (Tvrtko Ursulin)
> 
> V4:
>   * Rebase.
> 
> V5:
>   * Rebase.
> 
> V6
>   * Rebase.
> 
> Cc: Vipin Anand <vipin.anand at intel.com>
> Signed-off-by: Ankit Navik <ankit.p.navik at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c       | 1 +
>   drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 5 +++++
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c    | 2 ++
>   drivers/gpu/drm/i915/gt/intel_lrc.c               | 3 +++
>   4 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index c94ac83..8288fb9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -712,6 +712,7 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags)
>   	}
>   
>   	trace_i915_context_create(ctx);
> +	atomic_set(&ctx->req_cnt, 0);
>   
>   	return ctx;
>   }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index c060bc4..3931c06 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -168,6 +168,11 @@ struct i915_gem_context {
>   	 */
>   	struct radix_tree_root handles_vma;
>   
> +	/** req_cnt: tracks the pending commands, based on which we decide to
> +	 * go for low/medium/high load configuration of the GPU.
> +	 */
> +	atomic_t req_cnt;
> +
>   	/** jump_whitelist: Bit array for tracking cmds during cmdparsing
>   	 *  Guarded by struct_mutex
>   	 */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 7a87e82..83f4392 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2700,6 +2700,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	if (eb.batch->private)
>   		intel_engine_pool_mark_active(eb.batch->private, eb.request);
>   
> +	atomic_inc(&eb.gem_context->req_cnt);
> +
>   	trace_i915_request_queue(eb.request, eb.batch_flags);
>   	err = eb_submit(&eb);
>   err_request:
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 4cd0d46..511d5a1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1956,6 +1956,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   
>   				submit = true;
>   				last = rq;
> +
> +				if (atomic_read(&rq->gem_context->req_cnt) > 0)

If you need this check (underflow is real) we need to understand when it 
can happen and solve it properly. Until then the code is telling me that 
in some unexplained circumstances feature will not work for this context.

Perhaps put a GEM_BUG_ON there and send it to CI so we see if it's real 
or not. :)

On the overall it is still an interesting question what request states 
should be looked at by the heuristics. Submitted or submitted + 
runnable. Long time ago I had patches which implement these counters 
correctly so that needs to be dug out and rebased on drm-tip. Then you 
can read and add the counts you want.

Regards,

Tvrtko

> +					atomic_dec(&rq->gem_context->req_cnt);
>   			}
>   		}
>   
>