[Intel-gfx] [PATCH v2] drm/i915: Cleanup i915_gem_request_add_to_client

[Tvrtko Ursulin]

On 28/09/2016 18:09, Chris Wilson wrote:
> On Wed, Sep 28, 2016 at 05:48:23PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at 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.

Ok.

>> +{
>> +	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.

I did not even spot it while looking. Will remove.

>> +	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.

Embarrassing. :I

Regards,

Tvrtko