| Message ID | 1475135611-18548-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 29, 2016 at 08:53:31AM +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) > > v3: > * Shorten name, remove last standing assert and > fix flushing after failed submit. (Chris Wilson) > > 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 | 16 ++++++++++++---- > drivers/gpu/drm/i915/i915_gem_request.c | 25 ------------------------- > drivers/gpu/drm/i915/i915_gem_request.h | 2 -- > 3 files changed, 12 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 33c85227643d..f7b96391ff66 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1612,6 +1612,17 @@ eb_select_engine(struct drm_i915_private *dev_priv, > return engine; > } > > +static void > +add_to_client(struct drm_i915_gem_request *req, struct drm_file *file) > +{ > + struct drm_i915_file_private *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); > +} > + > static int > i915_gem_do_execbuffer(struct drm_device *dev, void *data, > struct drm_file *file, > @@ -1824,9 +1835,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; > + add_to_client(params->request, file); So we add to the client list even if we fail to submit the batch? For that reason I put the call to add_to_client() in execbuf_submit. -Chris
On 29/09/2016 09:08, Chris Wilson wrote: > On Thu, Sep 29, 2016 at 08:53:31AM +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) >> >> v3: >> * Shorten name, remove last standing assert and >> fix flushing after failed submit. (Chris Wilson) >> >> 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 | 16 ++++++++++++---- >> drivers/gpu/drm/i915/i915_gem_request.c | 25 ------------------------- >> drivers/gpu/drm/i915/i915_gem_request.h | 2 -- >> 3 files changed, 12 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 33c85227643d..f7b96391ff66 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1612,6 +1612,17 @@ eb_select_engine(struct drm_i915_private *dev_priv, >> return engine; >> } >> >> +static void >> +add_to_client(struct drm_i915_gem_request *req, struct drm_file *file) >> +{ >> + struct drm_i915_file_private *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); >> +} >> + >> static int >> i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> struct drm_file *file, >> @@ -1824,9 +1835,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; >> + add_to_client(params->request, file); > So we add to the client list even if we fail to submit the batch? For > that reason I put the call to add_to_client() in execbuf_submit. Same as existing code, that was just trimming the useless bits. If you are reworking all this shortly lets drop this then. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 33c85227643d..f7b96391ff66 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1612,6 +1612,17 @@ eb_select_engine(struct drm_i915_private *dev_priv, return engine; } +static void +add_to_client(struct drm_i915_gem_request *req, struct drm_file *file) +{ + struct drm_i915_file_private *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); +} + static int i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_file *file, @@ -1824,9 +1835,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; + add_to_client(params->request, file); /* * Save assorted stuff away to pass through to *_submission(). @@ -1841,7 +1850,6 @@ 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); 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