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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: tidy up request alloc
Date: Thu, 30 Jun 2016 11:22:41 +0100	[thread overview]
Message-ID: <20160630102241.GY28577@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <5774DD4C.6030702@linux.intel.com>

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