| Message ID | 1426768264-16996-60-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The i915_gem_object_flush_active() call used to do lots. Over time it has done > less and less. Now all it does call i915_gem_retire_requests_ring(). Hence it is > pretty much redundant as the two callers could just call retire directly. This > patch makes that change. > This commit message tells us that we're removing the i915_gem_object_flush_active() function and inlining its remaining code at its former call sites. What it does not mention is that we also replace obj->active with obj->last_read_req because this is functionally equivalent and is preferably in this case for whatever reason. If you don't already know the relationship between obj->active and obj->last_read_req and that they are always equivalent without exception and that obj->active == 0 always corresponds to obj->last_read_req == NULL and not just some dead request or whatever, then this inlined, undocumented change is less than trivial. How about mentioning it in the commit message and point out that this is perfectly fine. > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++---------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index acb824c..aef4748 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2825,25 +2825,6 @@ i915_gem_idle_work_handler(struct work_struct *work) > } > > /** > - * Ensures that an object will eventually get non-busy by flushing any required > - * write domains, emitting any outstanding lazy request and retiring and > - * completed requests. > - */ > -static int > -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > -{ > - struct intel_engine_cs *ring; > - > - if (obj->active) { > - ring = i915_gem_request_get_ring(obj->last_read_req); > - > - i915_gem_retire_requests_ring(ring); > - } > - > - return 0; > -} > - > -/** > * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT > * @DRM_IOCTL_ARGS: standard ioctl arguments > * > @@ -2888,10 +2869,12 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > return -ENOENT; > } > > - /* Need to make sure the object gets inactive eventually. */ > - ret = i915_gem_object_flush_active(obj); > - if (ret) > - goto out; > + /* Make sure the object is not pending cleanup. */ > + if (obj->last_read_req) { > + struct intel_engine_cs *ring; > + ring = i915_gem_request_get_ring(obj->last_read_req); > + i915_gem_retire_requests_ring(ring); > + } > > if (!obj->active || !obj->last_read_req) > goto out; > @@ -4335,19 +4318,17 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > - /* Count all active objects as busy, even if they are currently not used > - * by the gpu. Users of this interface expect objects to eventually > - * become non-busy without any further actions, therefore emit any > - * necessary flushes here. > - */ > - ret = i915_gem_object_flush_active(obj); > - > args->busy = obj->active; > if (obj->last_read_req) { > struct intel_engine_cs *ring; > BUILD_BUG_ON(I915_NUM_RINGS > 16); > ring = i915_gem_request_get_ring(obj->last_read_req); > - args->busy |= intel_ring_flag(ring) << 16; > + > + /* Check that the object wasn't simply pending cleanup */ > + i915_gem_retire_requests_ring(ring); > + > + if (obj->last_read_req) > + args->busy |= intel_ring_flag(ring) << 16; > } > Having an if case like: if (expression) { ... if (expression) { ... } ... } Doesn't sit super-well with me since it's not entirely obvious how the state changed inside the if statement. Obviously there must have been a side-effect at some point but it would be nicer to have that spelled out explicitly instead of having it implied. I'm not saying that you should restructure anything but how about just embellishing the comment before the i915_gem_retire_requests_ring a bit saying something like: "Check that the object wasn't simply pending cleanup, in which case obj->last_read_req is cleared" or add a comment before if (obj->last_read_req) saying "if object was pending cleanup don't update args->busy" or whatever? Or am I being overly lazy now? Is this really trivial? Thanks, Tomas > drm_gem_object_unreference(&obj->base); >
On Tue, Mar 31, 2015 at 07:32:41PM +0100, Tomas Elf wrote: > On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote: > >@@ -4335,19 +4318,17 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > > goto unlock; > > } > > > >- /* Count all active objects as busy, even if they are currently not used > >- * by the gpu. Users of this interface expect objects to eventually > >- * become non-busy without any further actions, therefore emit any > >- * necessary flushes here. > >- */ > >- ret = i915_gem_object_flush_active(obj); > >- > > args->busy = obj->active; > > if (obj->last_read_req) { > > struct intel_engine_cs *ring; > > BUILD_BUG_ON(I915_NUM_RINGS > 16); > > ring = i915_gem_request_get_ring(obj->last_read_req); > >- args->busy |= intel_ring_flag(ring) << 16; > >+ > >+ /* Check that the object wasn't simply pending cleanup */ > >+ i915_gem_retire_requests_ring(ring); > >+ > >+ if (obj->last_read_req) > >+ args->busy |= intel_ring_flag(ring) << 16; > > } > > > > Having an if case like: > > if (expression) { > ... > if (expression) { > ... > } > ... > } > > Doesn't sit super-well with me since it's not entirely obvious how the state > changed inside the if statement. Obviously there must have been a > side-effect at some point but it would be nicer to have that spelled out > explicitly instead of having it implied. I'm not saying that you should > restructure anything but how about just embellishing the comment before the > i915_gem_retire_requests_ring a bit saying something like: > > "Check that the object wasn't simply pending cleanup, in which case > obj->last_read_req is cleared" > > or add a comment before if (obj->last_read_req) saying > > "if object was pending cleanup don't update args->busy" or whatever? > > Or am I being overly lazy now? Is this really trivial? Maybe: + /* Check that the object wasn't simply pending cleanup */ + i915_gem_retire_requests_ring(ring); + + /* ... and only set busy if it really is so. */ + if (obj->last_read_req) + args->busy |= intel_ring_flag(ring) << 16; Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index acb824c..aef4748 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2825,25 +2825,6 @@ i915_gem_idle_work_handler(struct work_struct *work) } /** - * Ensures that an object will eventually get non-busy by flushing any required - * write domains, emitting any outstanding lazy request and retiring and - * completed requests. - */ -static int -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) -{ - struct intel_engine_cs *ring; - - if (obj->active) { - ring = i915_gem_request_get_ring(obj->last_read_req); - - i915_gem_retire_requests_ring(ring); - } - - return 0; -} - -/** * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT * @DRM_IOCTL_ARGS: standard ioctl arguments * @@ -2888,10 +2869,12 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) return -ENOENT; } - /* Need to make sure the object gets inactive eventually. */ - ret = i915_gem_object_flush_active(obj); - if (ret) - goto out; + /* Make sure the object is not pending cleanup. */ + if (obj->last_read_req) { + struct intel_engine_cs *ring; + ring = i915_gem_request_get_ring(obj->last_read_req); + i915_gem_retire_requests_ring(ring); + } if (!obj->active || !obj->last_read_req) goto out; @@ -4335,19 +4318,17 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, goto unlock; } - /* Count all active objects as busy, even if they are currently not used - * by the gpu. Users of this interface expect objects to eventually - * become non-busy without any further actions, therefore emit any - * necessary flushes here. - */ - ret = i915_gem_object_flush_active(obj); - args->busy = obj->active; if (obj->last_read_req) { struct intel_engine_cs *ring; BUILD_BUG_ON(I915_NUM_RINGS > 16); ring = i915_gem_request_get_ring(obj->last_read_req); - args->busy |= intel_ring_flag(ring) << 16; + + /* Check that the object wasn't simply pending cleanup */ + i915_gem_retire_requests_ring(ring); + + if (obj->last_read_req) + args->busy |= intel_ring_flag(ring) << 16; } drm_gem_object_unreference(&obj->base);