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

[Tvrtko Ursulin]

On 06/11/2018 04:13, Ankit Navik wrote:
> From: Praveen Diwakar <praveen.diwakar at intel.com>
> 
> This patch gives us the active pending request count which is yet
> to be submitted to the GPU
> 
> Signed-off-by: Praveen Diwakar <praveen.diwakar at intel.com>
> Signed-off-by: Yogesh Marathe <yogesh.marathe at intel.com>
> Signed-off-by: Aravindan Muthukumar <aravindan.muthukumar at intel.com>
> Signed-off-by: Kedar J Karanje <kedar.j.karanje at intel.com>
> Signed-off-by: Ankit Navik <ankit.p.navik at intel.com>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c            | 1 +
>   drivers/gpu/drm/i915/i915_drv.h            | 5 +++++
>   drivers/gpu/drm/i915/i915_gem_context.c    | 1 +
>   drivers/gpu/drm/i915/i915_gem_context.h    | 6 ++++++
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++++
>   drivers/gpu/drm/i915/intel_lrc.c           | 6 ++++++
>   6 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f8cfd16..d37c46e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -903,6 +903,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>   	mutex_init(&dev_priv->av_mutex);
>   	mutex_init(&dev_priv->wm.wm_mutex);
>   	mutex_init(&dev_priv->pps_mutex);
> +	mutex_init(&dev_priv->pred_mutex);
>   
>   	i915_memcpy_init_early(dev_priv);
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4aca534..137ec33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1609,6 +1609,11 @@ struct drm_i915_private {
>   	 * controller on different i2c buses. */
>   	struct mutex gmbus_mutex;
>   
> +	/** pred_mutex protects against councurrent usage of pending
> +	 * request counter for multiple contexts
> +	 */
> +	struct mutex pred_mutex;
> +
>   	/**
>   	 * Base address of the gmbus and gpio block.
>   	 */
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b10770c..0bcbe32 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -387,6 +387,7 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
>   	}
>   
>   	trace_i915_context_create(ctx);
> +	atomic_set(&ctx->req_cnt, 0);
>   
>   	return ctx;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index b116e49..04e3ff7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -194,6 +194,12 @@ struct i915_gem_context {
>   	 * context close.
>   	 */
>   	struct list_head handles_list;
> +
> +	/** req_cnt: tracks the pending commands, based on which we decide to
> +	 * go for low/medium/high load configuration of the GPU, this is
> +	 * controlled via a mutex
> +	 */
> +	atomic_t req_cnt;
>   };
>   
>   static inline bool i915_gem_context_is_closed(const struct i915_gem_context *ctx)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3f0c612..8afa2a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2178,6 +2178,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   		       struct drm_syncobj **fences)
>   {
>   	struct i915_execbuffer eb;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>   	struct dma_fence *in_fence = NULL;
>   	struct sync_file *out_fence = NULL;
>   	int out_fence_fd = -1;
> @@ -2390,6 +2391,10 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	 */
>   	eb.request->batch = eb.batch;
>   
> +	mutex_lock(&dev_priv->pred_mutex);
> +	atomic_inc(&eb.ctx->req_cnt);

Point of going to atomic_t was to remove need for the mutex.

> +	mutex_unlock(&dev_priv->pred_mutex);
> +
>   	trace_i915_request_queue(eb.request, eb.batch_flags);
>   	err = eb_submit(&eb);
>   err_request:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 1744792..bcbb66b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -728,6 +728,12 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   			trace_i915_request_in(rq, port_index(port, execlists));
>   			last = rq;
>   			submit = true;
> +
> +			mutex_lock(&rq->i915->pred_mutex);
> +			if (atomic_read(&rq->gem_context->req_cnt) > 0)
> +				atomic_dec(&rq->gem_context->req_cnt);

Hitting underflow is a hint accounting does not work as expected. I 
really think you need to fix it by gathering some ideas from the patches 
I've pointed at in the previous round.

And there is also GuC to think about.

Regards,

Tvrtko

> +
> +			mutex_unlock(&rq->i915->pred_mutex);
>   		}
>   
>   		rb_erase_cached(&p->node, &execlists->queue);
>