Re: [PATCH] drm/i915: tidy up request alloc

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Hong Liu <hong.liu@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: tidy up request alloc
Date: Thu, 30 Jun 2016 13:49:14 +0100	[thread overview]
Message-ID: <5775154A.9070005@linux.intel.com> (raw)
In-Reply-To: <20160630102241.GY28577@nuc-i3427.alporthouse.com>

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@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

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