| Message ID | 1344980117-1993-2-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > Clear the cached instdone state to match what we expect from hardware > and prevent us from comparing stale values. > > Actually, clearing the state is not the same as setting idle state. > There would be a known state of idle (ie. all units are done), but since > it differs for every platform, we can just set 0, and let the hangcheck > progress as normal. > > By putting the clear into add_request we are essentially initializing > the cached instdone to a known state before we start the hangcheck > timer. > > v2: clear instdone in more place (Chris) > Rewrote the commit message > > References: https://bugs.freedesktop.org/show_bug.cgi?id=52429 > Tested-by: Guang A Yang <guang.a.yang@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> So if hangcheck is already running and we add a new request, we still clear the "stale" state. This means that we may take an extra tick after a new request for hangcheck to raise an error. Not a big deal and it keeps the code clean. Though possibly deserves a comment? Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 2012-08-15 02:14, Chris Wilson wrote: > On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> > wrote: >> Clear the cached instdone state to match what we expect from >> hardware >> and prevent us from comparing stale values. >> >> Actually, clearing the state is not the same as setting idle state. >> There would be a known state of idle (ie. all units are done), but >> since >> it differs for every platform, we can just set 0, and let the >> hangcheck >> progress as normal. >> >> By putting the clear into add_request we are essentially >> initializing >> the cached instdone to a known state before we start the hangcheck >> timer. >> >> v2: clear instdone in more place (Chris) >> Rewrote the commit message >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429 >> Tested-by: Guang A Yang <guang.a.yang@intel.com> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > So if hangcheck is already running and we add a new request, we still > clear the "stale" state. This means that we may take an extra tick > after > a new request for hangcheck to raise an error. Not a big deal and it > keeps the code clean. Though possibly deserves a comment? > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris I was thinking about this exactly. I rationalized it as comparing stale state and comparing 0 state should result in the same number of retries, unless of course stale state happens to be equal to the state at the first timeout. This thinking led me to start thinking that this patch wasn't even really necessary at all, so I started going lalala.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 0514593..0d992e6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1601,6 +1601,7 @@ i915_add_request(struct intel_ring_buffer *ring, if (!dev_priv->mm.suspended) { if (i915_enable_hangcheck) { + dev_priv->last_instdone = dev_priv->last_instdone1 = 0; mod_timer(&dev_priv->hangcheck_timer, jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));