[Intel-gfx] [PATCH] drm/i915: tidy up request alloc

[Tvrtko Ursulin]

On 30/06/16 11:22, Chris Wilson wrote:
> On Thu, Jun 30, 2016 at 09:50:20AM +0100, Tvrtko Ursulin wrote:
>>
>> On 30/06/16 02:35, Hong Liu wrote:
>>> Return the allocated request pointer directly to remove
>>> the double pointer parameter.
>>>
>>> Signed-off-by: Hong Liu <hong.liu at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c | 25 +++++++------------------
>>>   1 file changed, 7 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 1d98782..9881455 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2988,32 +2988,26 @@ void i915_gem_request_free(struct kref *req_ref)
>>>   	kmem_cache_free(req->i915->requests, req);
>>>   }
>>>
>>> -static inline int
>>> +static inline struct drm_i915_gem_request *
>>>   __i915_gem_request_alloc(struct intel_engine_cs *engine,
>>> -			 struct i915_gem_context *ctx,
>>> -			 struct drm_i915_gem_request **req_out)
>>> +			 struct i915_gem_context *ctx)
>>>   {
>>>   	struct drm_i915_private *dev_priv = engine->i915;
>>>   	unsigned reset_counter = i915_reset_counter(&dev_priv->gpu_error);
>>>   	struct drm_i915_gem_request *req;
>>>   	int ret;
>>>
>>> -	if (!req_out)
>>> -		return -EINVAL;
>>> -
>>> -	*req_out = NULL;
>>> -
>>>   	/* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
>>>   	 * EIO if the GPU is already wedged, or EAGAIN to drop the struct_mutex
>>>   	 * and restart.
>>>   	 */
>>>   	ret = i915_gem_check_wedge(reset_counter, dev_priv->mm.interruptible);
>>>   	if (ret)
>>> -		return ret;
>>> +		return ERR_PTR(ret);
>>>
>>>   	req = kmem_cache_zalloc(dev_priv->requests, GFP_KERNEL);
>>>   	if (req == NULL)
>>> -		return -ENOMEM;
>>> +		return ERR_PTR(-ENOMEM);
>>>
>>>   	ret = i915_gem_get_seqno(engine->i915, &req->seqno);
>>>   	if (ret)
>>> @@ -3041,14 +3035,13 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>>>   	if (ret)
>>>   		goto err_ctx;
>>>
>>> -	*req_out = req;
>>> -	return 0;
>>> +	return req;
>>>
>>>   err_ctx:
>>>   	i915_gem_context_unreference(ctx);
>>>   err:
>>>   	kmem_cache_free(dev_priv->requests, req);
>>> -	return ret;
>>> +	return ERR_PTR(ret);
>>>   }
>>>
>>>   /**
>>> @@ -3067,13 +3060,9 @@ struct drm_i915_gem_request *
>>>   i915_gem_request_alloc(struct intel_engine_cs *engine,
>>>   		       struct i915_gem_context *ctx)
>>>   {
>>> -	struct drm_i915_gem_request *req;
>>> -	int err;
>>> -
>>>   	if (ctx == NULL)
>>>   		ctx = engine->i915->kernel_context;
>>> -	err = __i915_gem_request_alloc(engine, ctx, &req);
>>> -	return err ? ERR_PTR(err) : req;
>>> +	return __i915_gem_request_alloc(engine, ctx);
>>>   }
>>>
>>>   struct drm_i915_gem_request *
>>>
>>
>> Looks good to me. And have this feeling I've seen this somewhere before.
>
> Several times. This is not the full tidy, nor does it realise the
> ramifactions of request alloc through the stack.

Hm I can't spot that it is doing anything wrong or making anything 
worse. You don't want to let the small cleanup in?

Regards,

Tvrtko