| Message ID | 1475081303-10821-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 28, 2016 at 05:48:23PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Several things we can clean up in this function: > > * Request and file passed in cannot be NULL. There is > a single caller which makes it impossible so change > that condition to a GEM_BUG_ON instead of a WARN > with a return code. > > * Same is true for req->file_priv which is always > zeroed before this function is called. > > * With the above we can remove the error return > from the function making it void. > > * dev_private local variable was unused. > > * Call site in i915_gem_do_execbuffer can be > simplified since there is no error handling any > longer. > > v2: > * Move next to the only caller. (Chris Wilson) > * Remove useless asserts. (Joonas Lahtinen) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v1) > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 +++++++++++++++----- > drivers/gpu/drm/i915/i915_gem_request.c | 25 ------------------------- > drivers/gpu/drm/i915/i915_gem_request.h | 2 -- > 3 files changed, 15 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 33c85227643d..20dc7d90cecf 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1612,6 +1612,19 @@ eb_select_engine(struct drm_i915_private *dev_priv, > return engine; > } > > +static void i915_gem_request_add_to_client(struct drm_i915_gem_request *req, > + struct drm_file *file) Just add_to_client. > +{ > + struct drm_i915_file_private *file_priv = file->driver_priv; > + > + GEM_BUG_ON(req->file_priv); The error isn't associated with execbuf. The explosion will happen elsewhere and will be from a path that doesn't go near execbuf. > + spin_lock(&file_priv->mm.lock); > + req->file_priv = file_priv; > + list_add_tail(&req->client_list, &file_priv->mm.request_list); > + spin_unlock(&file_priv->mm.lock); > +} > + > static int > i915_gem_do_execbuffer(struct drm_device *dev, void *data, > struct drm_file *file, > @@ -1824,9 +1837,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > */ > params->request->batch = params->batch; > > - ret = i915_gem_request_add_to_client(params->request, file); > - if (ret) > - goto err_request; > + i915_gem_request_add_to_client(params->request, file); > > /* > * Save assorted stuff away to pass through to *_submission(). > @@ -1841,8 +1852,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > params->ctx = ctx; > > ret = execbuf_submit(params, args, &eb->vmas); > -err_request: > - __i915_add_request(params->request, ret == 0); > + __i915_add_request(params->request, true); We don't want to force the flush if submit fails either. * mutters about having to add back err_request: in the current series. -Chris
On 28/09/2016 18:09, Chris Wilson wrote: > On Wed, Sep 28, 2016 at 05:48:23PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Several things we can clean up in this function: >> >> * Request and file passed in cannot be NULL. There is >> a single caller which makes it impossible so change >> that condition to a GEM_BUG_ON instead of a WARN >> with a return code. >> >> * Same is true for req->file_priv which is always >> zeroed before this function is called. >> >> * With the above we can remove the error return >> from the function making it void. >> >> * dev_private local variable was unused. >> >> * Call site in i915_gem_do_execbuffer can be >> simplified since there is no error handling any >> longer. >> >> v2: >> * Move next to the only caller. (Chris Wilson) >> * Remove useless asserts. (Joonas Lahtinen) >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v1) >> --- >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 +++++++++++++++----- >> drivers/gpu/drm/i915/i915_gem_request.c | 25 ------------------------- >> drivers/gpu/drm/i915/i915_gem_request.h | 2 -- >> 3 files changed, 15 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 33c85227643d..20dc7d90cecf 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1612,6 +1612,19 @@ eb_select_engine(struct drm_i915_private *dev_priv, >> return engine; >> } >> >> +static void i915_gem_request_add_to_client(struct drm_i915_gem_request *req, >> + struct drm_file *file) > Just add_to_client. Ok. >> +{ >> + struct drm_i915_file_private *file_priv = file->driver_priv; >> + >> + GEM_BUG_ON(req->file_priv); > The error isn't associated with execbuf. The explosion will happen > elsewhere and will be from a path that doesn't go near execbuf. I did not even spot it while looking. Will remove. >> + spin_lock(&file_priv->mm.lock); >> + req->file_priv = file_priv; >> + list_add_tail(&req->client_list, &file_priv->mm.request_list); >> + spin_unlock(&file_priv->mm.lock); >> +} >> + >> static int >> i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> struct drm_file *file, >> @@ -1824,9 +1837,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> */ >> params->request->batch = params->batch; >> >> - ret = i915_gem_request_add_to_client(params->request, file); >> - if (ret) >> - goto err_request; >> + i915_gem_request_add_to_client(params->request, file); >> >> /* >> * Save assorted stuff away to pass through to *_submission(). >> @@ -1841,8 +1852,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> params->ctx = ctx; >> >> ret = execbuf_submit(params, args, &eb->vmas); >> -err_request: >> - __i915_add_request(params->request, ret == 0); >> + __i915_add_request(params->request, true); > We don't want to force the flush if submit fails either. Embarrassing. :I Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 33c85227643d..20dc7d90cecf 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1612,6 +1612,19 @@ eb_select_engine(struct drm_i915_private *dev_priv, return engine; } +static void i915_gem_request_add_to_client(struct drm_i915_gem_request *req, + struct drm_file *file) +{ + struct drm_i915_file_private *file_priv = file->driver_priv; + + GEM_BUG_ON(req->file_priv); + + spin_lock(&file_priv->mm.lock); + req->file_priv = file_priv; + list_add_tail(&req->client_list, &file_priv->mm.request_list); + spin_unlock(&file_priv->mm.lock); +} + static int i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_file *file, @@ -1824,9 +1837,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, */ params->request->batch = params->batch; - ret = i915_gem_request_add_to_client(params->request, file); - if (ret) - goto err_request; + i915_gem_request_add_to_client(params->request, file); /* * Save assorted stuff away to pass through to *_submission(). @@ -1841,8 +1852,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, params->ctx = ctx; ret = execbuf_submit(params, args, &eb->vmas); -err_request: - __i915_add_request(params->request, ret == 0); + __i915_add_request(params->request, true); err_batch_unpin: /* diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index 40978bc12ceb..490d3a40271a 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -115,31 +115,6 @@ const struct fence_ops i915_fence_ops = { .timeline_value_str = i915_fence_timeline_value_str, }; -int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, - struct drm_file *file) -{ - struct drm_i915_private *dev_private; - struct drm_i915_file_private *file_priv; - - WARN_ON(!req || !file || req->file_priv); - - if (!req || !file) - return -EINVAL; - - if (req->file_priv) - return -EINVAL; - - dev_private = req->i915; - file_priv = file->driver_priv; - - spin_lock(&file_priv->mm.lock); - req->file_priv = file_priv; - list_add_tail(&req->client_list, &file_priv->mm.request_list); - spin_unlock(&file_priv->mm.lock); - - return 0; -} - static inline void i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) { diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h index 974bd7bcc801..df1a41bbf645 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.h +++ b/drivers/gpu/drm/i915/i915_gem_request.h @@ -155,8 +155,6 @@ static inline bool fence_is_i915(struct fence *fence) struct drm_i915_gem_request * __must_check i915_gem_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx); -int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, - struct drm_file *file); void i915_gem_request_retire_upto(struct drm_i915_gem_request *req); static inline u32