Re: [PATCH 07/10] drm/i915: timeout parameter for seqno wait

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 20, 2012 at 06:23:29PM -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.
> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

Interface bikeshed: I think handling the wait_forever case in here would
be better, the do {} while (-ETIME) loops don't look to cute. That way we
can also ditch the rather arbitrary SEQNO_WAIT_DEFAULT.

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 920dbc1..7bfad04 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1847,25 +1847,36 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +#define SEQNO_WAIT_DEFAULT (3000000UL)
>  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;
> +	const long timeout = usecs_to_jiffies(*usecs);
> +	long end;
>  
>  #define EXIT_COND \
>  	(i915_seqno_passed(ring->get_seqno(ring), seqno) || \
>  	atomic_read(&dev_priv->mm.wedged))
>  
>  	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
>  
> -	return ret;
> +	switch (end) {
> +	case 0: /* Tiemout */
> +		return -ETIME;
> +	case -ERESTARTSYS: /* Signal */
> +		return -ERESTARTSYS;
> +	default: /* Completed */
> +		BUG_ON(end < 0); /* We're not aware of other errors */

WARN_ON, we won't die right away when this ever happens.

> +		*usecs = jiffies_to_usecs(timeout - end);

Safe when the ioctl restarting works different, you also need this for the
-ERESTARTSYS case. And it doesn't hurt for the timeout case.

> +		return 0;
> +	}
>  }
>  
>  /**
> @@ -1916,7 +1927,12 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  		if (WARN_ON(!ring->irq_get(ring)))
>  			return -EBUSY;
>  
> -		ret = __wait_seqno(ring, seqno, dev_priv->mm.interruptible);
> +		do {
> +			long time = SEQNO_WAIT_DEFAULT;
> +			ret  = __wait_seqno(ring, seqno,
> +					    dev_priv->mm.interruptible,
> +					    &time);
> +		} while (ret == -ETIME);
>  
>  		ring->irq_put(ring);
>  		trace_i915_gem_request_wait_end(ring, seqno);
> @@ -3012,13 +3028,16 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
>  		 * lockless.
>  		 */
>  		if (ring->irq_get(ring)) {
> -			ret = __wait_seqno(ring, seqno, true);
> +			do {
> +				long time = SEQNO_WAIT_DEFAULT;
> +				ret = __wait_seqno(ring, seqno, true, &time);
> +			} while (ret == -ETIME);
>  			ring->irq_put(ring);
> -
>  			if (ret == 0 && atomic_read(&dev_priv->mm.wedged))
>  				ret = -EIO;
> -		} else
> +		} else {
>  			ret = -EBUSY;
> +		}
>  	}
>  
>  	if (ret == 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