[Intel-gfx] [PATCH] drm/i915: increase the default timeout in wait_seqno to 60s

[Ben Widawsky]

On Wed, Aug 07, 2013 at 12:45:14AM +0200, Daniel Vetter wrote:
> We'll loop forever, but if we wake up before the hangcheck time has a
> chance to complain that we're stuck waiting for the interrupt we'll
> loose that valuable debug tool. And silently blocking for 1 second is
> just not the right approach.
> 
> Noticed since Paulo reported a suspicious 2 fps when running glxgears
> with pc8+ enabled. Since mesa stalls for the second-last frame before
> rendering a new one this is perfectly explained by hitting the 1
> second timeout.
> 
> This regression has been introduced in Ben's wait with timeout ioctl
> work in:
> 
> commit 5c81fe85dad3c8c2bcec03e3fc2bfd4ea198763c
> Author: Ben Widawsky <ben at bwidawsk.net>
> Date:   Thu May 24 15:03:08 2012 -0700
> 
>     drm/i915: timeout parameter for seqno wait
> 
> Also fix whitespace in that line while at it and add an explicit
> comment.
> 
> Cc: Ben Widawsky <ben at bwidawsk.net>
> Cc: Paulo Zanoni <przanoni at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8508b3d..84170fe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -986,7 +986,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>  			bool interruptible, struct timespec *timeout)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	struct timespec before, now, wait_time={1,0};
> +	struct timespec before, now, wait_time;
>  	unsigned long timeout_jiffies;
>  	long end;
>  	bool wait_forever = true;
> @@ -1000,6 +1000,14 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
>  	if (timeout != NULL) {
>  		wait_time = *timeout;
>  		wait_forever = false;
> +	} else {
> +		/*
> +		 * The timeout here must be longer than the hangcheck timeout,
> +		 * otherwise we'll no longer detect missed interrupts but end up
> +		 * with dead-slow rendering.
> +		 */
> +		wait_time.tv_sec = 60;
> +		wait_time.tv_nsec = 0;
>  	}
>  
>  	timeout_jiffies = timespec_to_jiffies_timeout(&wait_time);

If you do this as two patches, 1 to add the else, and 1 two change the
timeout value, you have my r-b on the "else" patch. I also wouldn't say
it's a regression, but that's semantics.

I don't want to touch the timeout change since it scares me, and I don't
have time to investigate the implications. Acked-by if you want to add
it. Please test simulation before you merge it.

-- 
Ben Widawsky, Intel Open Source Technology Center