[Intel-gfx] [PATCH v3 3/6] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[Tvrtko Ursulin]

On 06/05/16 20:31, Dave Gordon wrote:
> The knowledge of how to derive the relevant client from the request
> should be localised within i915_guc_submission.c; the LRC code shouldn't
> have to know about the internal details of the GuC submission process.
> And all the information the GuC code needs should be encapsulated in (or
> reachable from) the request.
>
> v3:
>      GEM_BUG_ON() for bad GuC client (Tvrtko Ursulin).
>      Add/update kerneldoc explaining check_space/submit protocol
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 46 ++++++++++++++++++++++++------
>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++--
>   drivers/gpu/drm/i915/intel_lrc.c           |  9 ++----
>   3 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e6af672..9e30762 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -451,14 +451,30 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>   			     sizeof(desc) * client->ctx_index);
>   }
>
> -int i915_guc_wq_check_space(struct i915_guc_client *gc)
> +/**
> + * i915_guc_wq_check_space() - check that the GuC can accept a request
> + * @request:	request associated with the commands
> + *
> + * Return:	0 if space is available
> + *		-EAGAIN if space is not currently available
> + *
> + * This function must be called (and must return 0) before a request
> + * is submitted to the GuC via i915_guc_submit() below. Once a result
> + * of 0 has been returned, it remains valid until (but only until)
> + * the next call to submit().
> + *
> + * This precheck allows the caller to determine in advance that space
> + * will be available for the next submission before committing resources
> + * to it, and helps avoid late failures with complicated recovery paths.
> + */
> +int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>   {
> +	const size_t size = sizeof(struct guc_wq_item);
> +	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
>   	struct guc_process_desc *desc;
> -	u32 size = sizeof(struct guc_wq_item);
>   	int ret = -ETIMEDOUT, timeout_counter = 200;
>
> -	if (!gc)
> -		return 0;
> +	GEM_BUG_ON(gc == NULL);
>
>   	desc = gc->client_base + gc->proc_desc_offset;
>
> @@ -532,16 +548,28 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>
>   /**
>    * i915_guc_submit() - Submit commands through GuC
> - * @client:	the guc client where commands will go through
>    * @rq:		request associated with the commands
>    *
> - * Return:	0 if succeed
> + * Return:	0 on success, otherwise an errno.
> + * 		(Note: nonzero really shouldn't happen!)
> + *
> + * The caller must have already called i915_guc_wq_check_space() above
> + * with a result of 0 (success) since the last request submission. This
> + * guarantees that there is space in the work queue for the new request,
> + * so enqueuing the item cannot fail.
> + *
> + * Bad Things Will Happen if the caller violates this protocol e.g. calls
> + * submit() when check() says there's no space, or calls submit() multiple
> + * times with no intervening check().
> + *
> + * The only error here arises if the doorbell hardware isn't functioning
> + * as expected, which really shouln't happen.
>    */
> -int i915_guc_submit(struct i915_guc_client *client,
> -		    struct drm_i915_gem_request *rq)
> +int i915_guc_submit(struct drm_i915_gem_request *rq)
>   {
> -	struct intel_guc *guc = client->guc;
>   	unsigned int engine_id = rq->engine->guc_id;
> +	struct intel_guc *guc = &rq->i915->guc;
> +	struct i915_guc_client *client = guc->execbuf_client;
>   	int q_ret, b_ret;
>
>   	q_ret = guc_add_workqueue_item(client, rq);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 9d79c4c..b37c731 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev);
>   /* i915_guc_submission.c */
>   int i915_guc_submission_init(struct drm_device *dev);
>   int i915_guc_submission_enable(struct drm_device *dev);
> -int i915_guc_submit(struct i915_guc_client *client,
> -		    struct drm_i915_gem_request *rq);
> +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
> +int i915_guc_submit(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_device *dev);
>   void i915_guc_submission_fini(struct drm_device *dev);
> -int i915_guc_wq_check_space(struct i915_guc_client *client);
>
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d876352..79302e1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -698,9 +698,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   		 * going any further, as the i915_add_request() call
>   		 * later on mustn't fail ...
>   		 */
> -		struct intel_guc *guc = &request->i915->guc;
> -
> -		ret = i915_guc_wq_check_space(guc->execbuf_client);
> +		ret = i915_guc_wq_check_space(request);
>   		if (ret)
>   			return ret;
>   	}
> @@ -749,7 +747,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>   {
>   	struct intel_ringbuffer *ringbuf = request->ringbuf;
> -	struct drm_i915_private *dev_priv = request->i915;
>   	struct intel_engine_cs *engine = request->engine;
>
>   	intel_logical_ring_advance(ringbuf);
> @@ -777,8 +774,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   	request->previous_context = engine->last_context;
>   	engine->last_context = request->ctx;
>
> -	if (dev_priv->guc.execbuf_client)
> -		i915_guc_submit(dev_priv->guc.execbuf_client, request);
> +	if (i915.enable_guc_submission)
> +		i915_guc_submit(request);
>   	else
>   		execlists_context_queue(request);
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko