Re: [PATCH 09/12] drm/i915: timeout parameter for seqno wait

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 26, 2012 at 04:03:06PM -0700, Ben Widawsky wrote:
> Insert a wait parameter in the code so we can possibly timeout on a
> seqno wait if need be. The code should be functionally the same as
> before because all the callers will continue to retry if an arbitrary
> timeout elapses.
> 
> We'd like to have nanosecond granularity, but the only way to do this is
> with hrtimer, and that doesn't fit well with the needs of this code.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I have to admit, I'm a bit unhappy with this swiss-army-tool wait_seqno
and what it looks like. What about copy&pasting __wait_seqno_timeout,
which is always interruptible?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   58 +++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0ae1a73..f054439 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1820,10 +1820,23 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  }
>  
>  static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
> -			bool interruptible)
> +			bool interruptible, long *usecs)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	int ret = 0;
> +	bool wait_forever = false;
> +	long timeout, end;
> +
> +	if (usecs == NULL || ((*usecs) < 0)) {
> +		wait_forever = true;
> +		timeout = msecs_to_jiffies(100);
> +	} else
> +		timeout = usecs_to_jiffies(*usecs);
> +
> +	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
> +		return 0;
> +
> +	if (WARN_ON(!ring->irq_get(ring)))
> +		return -ENODEV;
>  
>  	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
>  		return 0;
> @@ -1836,17 +1849,40 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>  	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
>  	atomic_read(&dev_priv->mm.wedged))
>  
> +	trace_i915_gem_request_wait_begin(ring, seqno);
> +again:
>  	if (interruptible)
> -		ret = wait_event_interruptible(ring->irq_queue,
> -					       EXIT_COND);
> +		end = wait_event_interruptible_timeout(ring->irq_queue,
> +						       EXIT_COND,
> +						       timeout);
>  	else
> -		wait_event(ring->irq_queue, EXIT_COND);
> +		end = wait_event_timeout(ring->irq_queue, EXIT_COND, timeout);
> +#undef EXIT_COND
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		end = -EAGAIN;
> +
> +	if (end == 0 && wait_forever)
> +		goto again;
>  
> -	ring->irq_put(ring);
>  	trace_i915_gem_request_wait_end(ring, seqno);
> -#undef EXIT_COND
> +	ring->irq_put(ring);
>  
> -	return ret;
> +	if (!wait_forever) {
> +		BUG_ON(end == 0);
> +		*usecs = jiffies_to_usecs(timeout - end);
> +	}
> +
> +	switch (end) {
> +	case -EAGAIN: /* Wedged */
> +	case -ERESTARTSYS: /* Signal */
> +		return (int)end;
> +	case 0: /* Tiemout */
> +		return -ETIME;
> +	default: /* Completed */
> +		WARN_ON(end < 0); /* We're not aware of other errors */
> +		return 0;
> +	}
>  }
>  
>  /**
> @@ -1891,9 +1927,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  		seqno = request->seqno;
>  	}
>  
> -	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
> -	if (atomic_read(&dev_priv->mm.wedged))
> -		ret = -EAGAIN;
> +	ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible, NULL);
>  
>  	return ret;
>  }
> @@ -2981,7 +3015,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>  	if (seqno == 0)
>  		return 0;
>  
> -	ret = __wait_seqno(ring, seqno, true);
> +	ret = __wait_seqno(ring, seqno, true, NULL);
>  	if (ret == 0)
>  		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
>  
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48