On Tue, Mar 31, 2015 at 07:32:41PM +0100, Tomas Elf wrote:
> On 19/03/2015 12:31, John.C.Harrison@xxxxxxxxx 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
