Re: [PATCH] drm/radeon/kms: fence cleanup + more reliable GPU lockup detection

[Jerome G. <gl...@fr...>]

On Mon, Mar 01, 2010 at 04:00:02PM +1000, Dave Airlie wrote:
> On Mon, Mar 1, 2010 at 2:47 AM, Jerome Glisse <jg...@re...> wrote:
> > On Sun, Feb 28, 2010 at 12:22:52PM +0000, Alan Swanson wrote:
> >> On Fri, 2010-02-26 at 15:49 +0100, Jerome Glisse wrote:
> >> > This patch cleanup the fence code, it drops the timeout field of
> >> > fence as the time to complete each IB is unpredictable and shouldn't
> >> > be bound.
> >> >
> >> > The fence cleanup lead to GPU lockup detection improvement, this
> >> > patch introduce a callback, allowing to do asic specific test for
> >> > lockup detection. In this patch the CP is use as a first indicator
> >> > of GPU lockup. If CP doesn't make progress during 1second we assume
> >> > we are facing a GPU lockup.
> >> >
> >> > To avoid overhead of testing GPU lockup frequently due to fence
> >> > taking time to be signaled we query the lockup callback every
> >> > 100msec. There is plenty code comment explaining the design & choise
> >> > inside the code.
> >>
> >> Every 100msec? Is this running all the time? If so, that's not very good
> >> for CPU power saving to lower C-states in an idle system. We could at
> >> least use one of the round_jiffies.
> >>
> >
> > This run only when userspace call bo wait thus it only happen when userspace
> > is waiting for something.
> 
> Why not just test when the old timeout code used to test? every second or so?
> 
> I'm not sure why with the old code instead of assuming a fence timeout implied
> a lockup you didn't just change it to test if it was a real lockup and
> continue waiting
> if the GPU was making progress. This seems simpler, though maybe the cleanups
> are worth it.
> 
> Dave.
> 

The old code was misleading we didn't test every second or so but
it was depending on fence timeout and it was quite small amount
of time don't remember of hand. Here on the fast r7xx with 2 quake3
bunch of gears and compiz the average time for fence is 20ms.
But i think i will switch back to half a second timeout some irq
will likely wake up us.

Note that if you remove the comment lines in my patch i am pretty
sure my patch is removing code rather than adding some :) I will
do a V3 today.

Cheers,
Jerome