[Intel-gfx] [PATCH 59/59] drm/i915: Remove the almost obsolete i915_gem_object_flush_active()

[Daniel Vetter]

On Tue, Mar 31, 2015 at 07:32:41PM +0100, Tomas Elf wrote:
> On 19/03/2015 12:31, John.C.Harrison at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch