Re: [PATCH 59/59] drm/i915: Remove the almost obsolete i915_gem_object_flush_active()

[Tomas Elf <tomas.elf@intel.com>]

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);
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx