| Message ID | 1375829114-9990-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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@bwidawsk.net> > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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.
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);
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@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@bwidawsk.net> Cc: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)