Re: [PATCH v2] drm/i915: Cleanup i915_gem_request_add_to_client

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

On Wed, Sep 28, 2016 at 05:48:23PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Several things we can clean up in this function:
> 
>  * Request and file passed in cannot be NULL. There is
>    a single caller which makes it impossible so change
>    that condition to a GEM_BUG_ON instead of a WARN
>    with a return code.
> 
>  * Same is true for req->file_priv which is always
>    zeroed before this function is called.
> 
>  * With the above we can remove the error return
>    from the function making it void.
> 
>  * dev_private local variable was unused.
> 
>  * Call site in i915_gem_do_execbuffer can be
>    simplified since there is no error handling any
>    longer.
> 
> v2:
>  * Move next to the only caller. (Chris Wilson)
>  * Remove useless asserts. (Joonas Lahtinen)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v1)
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 +++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem_request.c    | 25 -------------------------
>  drivers/gpu/drm/i915/i915_gem_request.h    |  2 --
>  3 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 33c85227643d..20dc7d90cecf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1612,6 +1612,19 @@ eb_select_engine(struct drm_i915_private *dev_priv,
>  	return engine;
>  }
>  
> +static void i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> +					   struct drm_file *file)

Just add_to_client.

> +{
> +	struct drm_i915_file_private *file_priv = file->driver_priv;
> +
> +	GEM_BUG_ON(req->file_priv);

The error isn't associated with execbuf. The explosion will happen
elsewhere and will be from a path that doesn't go near execbuf.

> +	spin_lock(&file_priv->mm.lock);
> +	req->file_priv = file_priv;
> +	list_add_tail(&req->client_list, &file_priv->mm.request_list);
> +	spin_unlock(&file_priv->mm.lock);
> +}
> +
>  static int
>  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		       struct drm_file *file,
> @@ -1824,9 +1837,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	 */
>  	params->request->batch = params->batch;
>  
> -	ret = i915_gem_request_add_to_client(params->request, file);
> -	if (ret)
> -		goto err_request;
> +	i915_gem_request_add_to_client(params->request, file);
>  
>  	/*
>  	 * Save assorted stuff away to pass through to *_submission().
> @@ -1841,8 +1852,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	params->ctx                     = ctx;
>  
>  	ret = execbuf_submit(params, args, &eb->vmas);
> -err_request:
> -	__i915_add_request(params->request, ret == 0);
> +	__i915_add_request(params->request, true);

We don't want to force the flush if submit fails either.

* mutters about having to add back err_request: in the current series.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx