Re: [PATCH v2 2/5] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/04/16 19:03, 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.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++-----
>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++---
>   drivers/gpu/drm/i915/intel_lrc.c           |  9 +++------
>   3 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 42d2efa..66af5ce 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -451,10 +451,11 @@ 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)
> +int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>   {
> +	const size_t size = sizeof(struct guc_wq_item);

All the other variables passed to CIRC_SPACE are u32 so this is for the 
worse I think.

> +	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)
> @@ -537,11 +538,11 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
>    *
>    * Return:	0 if succeed
>    */
> -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 2b7e6bb..b8ec8c6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -708,9 +708,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;
>   	}
> @@ -776,7 +774,6 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
>   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);
> @@ -806,8 +803,8 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
>   		}
>   	}
>
> -	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);
>
>

With the datatype fix (revert) in i915_guc_wq_check_space:

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

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx