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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> Cc: Paulo Zanoni <przanoni@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
