Re: [Intel-gfx] [PATCH 1/5] drm/i915: Cleanup instdone state

2012-08-15 Thread Chris Wilson
On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky  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 
> Signed-off-by: Ben Widawsky 

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, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 1/5] drm/i915: Cleanup instdone state

2012-08-15 Thread Ben Widawsky

On 2012-08-15 02:14, Chris Wilson wrote:
On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky  
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 
Signed-off-by: Ben Widawsky 


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


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.


--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx