[Intel-gfx] [PATCH] drm/i915: extract object unplug code from busy/wait_timeout ioctl

[Ben Widawsky]

On Wed, 30 May 2012 20:21:33 +0200
Daniel Vetter <daniel.vetter at ffwll.ch> wrote:

> Both need to do the same dance (or at least should). Some slight
> changes:
> - busy_ioctl now unconditionally checks for olr. Before emitting a
>   require flush would have prevent the olr check and hence required a
>   second call to the busy ioctl to really emit the request.
> - the timeout wait now also retires request. Not really required for
>   abi-reasons, but makes a notch more sense imo.
> 

The one thing I dislike about the retiring behavior is it is still not
guaranteed. After some coffee, I've remembered a bit about this.  Since
we do an non-blocking wait, it's possible for userspace to race against
itself and rebusy the object. Therefore, the client should never assume
the call to wait_ioctl is the same as busy. (Busy has a similar, shorter
race; but also has a little smarter semantics). Therefore, I don't think
this buys us much, but I also don't think it hurts. So I'd vote on
letting Eric decide if he wants this or not.

> I've tested this by pimping the i-g-t test some more and also checking
> the polling behviour of the wait_rendering_timeout ioctl versus what
> busy_ioctl returns.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   61 ++++++++++++++++++--------------------
>  1 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d2eaa00..521e294 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2000,6 +2000,31 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
>  }
>  
>  /**
> + * 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. The unplug moniker is stolen from the linux block layer.
> + */
I'd prefer something like, "unbusy" but whatever.

> +static int
> +i915_gem_unplug_object(struct drm_i915_gem_object *obj)
> +{
> +	int ret;
> +
> +	if (obj->active) {
> +		ret = i915_gem_object_flush_gpu_write_domain(obj);
> +		if (ret)
> +			return ret;
> +
> +		ret = i915_gem_check_olr(obj->ring,
> +					 obj->last_rendering_seqno);
> +		if (ret)
> +			return ret;
> +		i915_gem_retire_requests_ring(obj->ring);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
>   * @DRM_IOCTL_ARGS: standard ioctl arguments
>   *
> @@ -2043,11 +2068,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		return -ENOENT;
>  	}
>  
> -	/* Need to make sure the object is flushed first. This non-obvious
> -	 * flush is required to enforce that (active && !olr) == no wait
> -	 * necessary.
> -	 */
> -	ret = i915_gem_object_flush_gpu_write_domain(obj);
> +	/* Need to make sure the object gets un-active eventually. */
> +	ret = i915_gem_unplug_object(obj);
>  	if (ret)
>  		goto out;

I can't remember the reason offhand, but I vaguely recall Chris demanded
we flush the write domain even if the object isn't active. Perhaps you
can look up the email, or he can chime in. I forget the exact reason.

>  
> @@ -2059,10 +2081,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	if (seqno == 0)
>  		 goto out;
>  
> -	ret = i915_gem_check_olr(ring, seqno);
> -	if (ret)
> -		goto out;
> -
>  	/* Do this after OLR check to make sure we make forward progress polling
>  	 * on this IOCTL with a 0 timeout (like busy ioctl)
>  	 */
> @@ -3302,30 +3320,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	 * become non-busy without any further actions, therefore emit any
>  	 * necessary flushes here.
>  	 */
> -	args->busy = obj->active;
> -	if (args->busy) {
> -		/* Unconditionally flush objects, even when the gpu still uses this
> -		 * object. Userspace calling this function indicates that it wants to
> -		 * use this buffer rather sooner than later, so issuing the required
> -		 * flush earlier is beneficial.
> -		 */
> -		if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
> -			ret = i915_gem_flush_ring(obj->ring,
> -						  0, obj->base.write_domain);
> -		} else {
> -			ret = i915_gem_check_olr(obj->ring,
> -						 obj->last_rendering_seqno);
> -		}
> +	ret = i915_gem_unplug_object(obj);
>  
> -		/* Update the active list for the hardware's current position.
> -		 * Otherwise this only updates on a delayed timer or when irqs
> -		 * are actually unmasked, and our working set ends up being
> -		 * larger than required.
> -		 */
> -		i915_gem_retire_requests_ring(obj->ring);
> -
> -		args->busy = obj->active;
> -	}
> +	args->busy = obj->active;
>  
>  	drm_gem_object_unreference(&obj->base);
>  unlock:



-- 
Ben Widawsky, Intel Open Source Technology Center