[PATCH] drm/i915: tidy up request alloc

[PATCH] drm/i915: tidy up request alloc

[Hong Liu]

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 *
-- 
2.7.4

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

✓ Ro.CI.BAT: success for drm/i915: tidy up request alloc

[Patchwork]

== Series Details ==

Series: drm/i915: tidy up request alloc
URL   : https://patchwork.freedesktop.org/series/9300/
State : success

== Summary ==

Series 9300v1 drm/i915: tidy up request alloc
http://patchwork.freedesktop.org/api/1.0/series/9300/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-kbl-qkkr      total:229  pass:161  dwarn:27  dfail:0   fail:0   skip:41 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1336/

8a6521c drm-intel-nightly: 2016y-06m-29d-16h-08m-16s UTC integration manifest
0c7486c7 drm/i915: tidy up request alloc

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

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

[Tvrtko Ursulin]

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.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

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

[Chris Wilson]

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

Re: [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@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

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

[Dave Gordon]

On 30/06/16 13:49, Tvrtko Ursulin wrote:
>
> 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

It ought to make almost no difference, because the *only* place the 
inner function is called is from the outer one, which passes a pointer 
to a local for the returned object; and the inner one is then inlined, 
so the compiler doesn't actually put it on the stack and call to the 
inner allocator anyway.

Strangely, however, with this change the code becomes ~400 bytes bigger!

Disassembly reveals that while the code for the externally-callable 
outer function is indeed almost identical, a second copy of it has also 
been inlined at the one callsite in this file:

__i915_gem_object_sync() ...
	req = i915_gem_request_alloc(to, NULL);

I don't think that's a critical path and would rather have 400 bytes 
smaller codespace. We can get that back by adding /noinline/ to the 
outer function i915_gem_request_alloc() (not, of course, to the inner 
one, that definitely *should* be inline).

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

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

[Chris Wilson]

On Fri, Jul 01, 2016 at 05:58:18PM +0100, Dave Gordon wrote:
> On 30/06/16 13:49, Tvrtko Ursulin wrote:
> >
> >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
> 
> It ought to make almost no difference, because the *only* place the
> inner function is called is from the outer one, which passes a
> pointer to a local for the returned object; and the inner one is
> then inlined, so the compiler doesn't actually put it on the stack
> and call to the inner allocator anyway.
> 
> Strangely, however, with this change the code becomes ~400 bytes bigger!
> 
> Disassembly reveals that while the code for the externally-callable
> outer function is indeed almost identical, a second copy of it has
> also been inlined at the one callsite in this file:
> 
> __i915_gem_object_sync() ...
> 	req = i915_gem_request_alloc(to, NULL);
> 
> I don't think that's a critical path and would rather have 400 bytes
> smaller codespace. We can get that back by adding /noinline/ to the
> outer function i915_gem_request_alloc() (not, of course, to the
> inner one, that definitely *should* be inline).

__i915_gem_object_sync() should not be calling i915_gem_request_alloc().

That's the issue with this patch, your patch and John's patch.
-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

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

[Liu, Hong]

On Fri, 2016-07-01 at 19:34 +0100, Chris Wilson wrote:
> On Fri, Jul 01, 2016 at 05:58:18PM +0100, Dave Gordon wrote:
> > On 30/06/16 13:49, Tvrtko Ursulin wrote:
> > > 
> > > 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
> > 
> > It ought to make almost no difference, because the *only* place the
> > inner function is called is from the outer one, which passes a
> > pointer to a local for the returned object; and the inner one is
> > then inlined, so the compiler doesn't actually put it on the stack
> > and call to the inner allocator anyway.
> > 
> > Strangely, however, with this change the code becomes ~400 bytes
> > bigger!
> > 
> > Disassembly reveals that while the code for the externally-callable
> > outer function is indeed almost identical, a second copy of it has
> > also been inlined at the one callsite in this file:
> > 
> > __i915_gem_object_sync() ...
> > 	req = i915_gem_request_alloc(to, NULL);
> > 
> > I don't think that's a critical path and would rather have 400
> > bytes
> > smaller codespace. We can get that back by adding /noinline/ to the
> > outer function i915_gem_request_alloc() (not, of course, to the
> > inner one, that definitely *should* be inline).
> 
> __i915_gem_object_sync() should not be calling
> i915_gem_request_alloc().
> 
> That's the issue with this patch, your patch and John's patch.

So we wrote the i915_gem_request_alloc() this way is to avoid being
inlined into callers like __i915_gem_object_sync()?

I checked the file with GCC 4.8.5 on my centos environment, it is like
what Dave found. With the patch, i915_gem_object_sync() is 368 bytes
bigger.

But when I checked it with GCC 6.1.1 on Fedora 24, it seems it inlines
the i915_gem_request_alloc() even with the current implementation.
With the patch, the i915_gem_object_sync() is 80 bytes smaller.


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

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

[Dave Gordon]

On 04/07/16 05:08, Liu, Hong wrote:
> On Fri, 2016-07-01 at 19:34 +0100, Chris Wilson wrote:
>> On Fri, Jul 01, 2016 at 05:58:18PM +0100, Dave Gordon wrote:
>>> On 30/06/16 13:49, Tvrtko Ursulin wrote:
>>>>
>>>> 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
>>>
>>> It ought to make almost no difference, because the *only* place the
>>> inner function is called is from the outer one, which passes a
>>> pointer to a local for the returned object; and the inner one is
>>> then inlined, so the compiler doesn't actually put it on the stack
>>> and call to the inner allocator anyway.
>>>
>>> Strangely, however, with this change the code becomes ~400 bytes
>>> bigger!
>>>
>>> Disassembly reveals that while the code for the externally-callable
>>> outer function is indeed almost identical, a second copy of it has
>>> also been inlined at the one callsite in this file:
>>>
>>> __i915_gem_object_sync() ...
>>> 	req = i915_gem_request_alloc(to, NULL);
>>>
>>> I don't think that's a critical path and would rather have 400
>>> bytes
>>> smaller codespace. We can get that back by adding /noinline/ to the
>>> outer function i915_gem_request_alloc() (not, of course, to the
>>> inner one, that definitely *should* be inline).
>>
>> __i915_gem_object_sync() should not be calling
>> i915_gem_request_alloc().
>>
>> That's the issue with this patch, your patch and John's patch.
>
> So we wrote the i915_gem_request_alloc() this way is to avoid being
> inlined into callers like __i915_gem_object_sync()?

Not specifically, as the description of commit 268270883 says,

"... this patch renames the existing i915_gem_request_alloc(), and
  makes it local (static inline), and replaces it with a wrapper that
  provides a default if the context is NULL, and also has a nicer
  calling convention (doesn't require a pointer to an output parameter).
  Then we change all callers to use the new convention:
     OLD:
         err = i915_gem_request_alloc(ring, user_ctx, &req);
         if (err) ...
     NEW:
         req = i915_gem_request_alloc(ring, user_ctx);
         if (IS_ERR(req)) ...
     OLD:
         err = i915_gem_request_alloc(ring, ring->default_context, &req);
         if (err) ...
     NEW:
         req = i915_gem_request_alloc(ring, NULL);
         if (IS_ERR(req)) ..."

At the time, it was more a matter of simplifying review and minimising 
lines-of-change by adding the wrapper to implement the new convention 
rather than rewriting the real function at the same time as changing all 
the callers. So I have no objection to continuing the process of making 
this nicer by now flattening the two layers into a single function; I 
just noted the oddity that the flattening made the codespace larger.

Chris, do you have a patch for *not* calling i915_gem_request_alloc() 
from inside __i915_gem_object_sync() ? I don't really know why John's 
original conversion to passing requests everywhere needed this.

.Dave.

> I checked the file with GCC 4.8.5 on my centos environment, it is like
> what Dave found. With the patch, i915_gem_object_sync() is 368 bytes
> bigger.
>
> But when I checked it with GCC 6.1.1 on Fedora 24, it seems it inlines
> the i915_gem_request_alloc() even with the current implementation.
> With the patch, the i915_gem_object_sync() is 80 bytes smaller.
>
>> -Chris

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