| Message ID | 1467250532-23644-1-git-send-email-hong.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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 >
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
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 *
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(-)