On 19/03/2015 12:31, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>
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@xxxxxxxxx>
---
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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
