Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, 15 Mar 2011 00:12:51 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, Mar 14, 2011 at 10:00:20PM -0700, Ben Widawsky wrote:
> > On Mon, Mar 14, 2011 at 09:55:01PM -0700, Ben Widawsky wrote:
> > > This fixes a race condition with MI_SET_CONTEXT and setting of the
> > > PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
> > > completes, it's possible that the system will enter rc6, and try to
> > > return to the default render context, which if unset, could cause a GPU
> > > hang
> > > 
> > > Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582
> > 
> > I'm still waiting for feedback on bugzilla if this patch works like the
> > previous. Just submitting it here for review while we wait...
> 
> It appears that I've jumped the gun on this fix. I can sort of reason
> that the LOAD_REGISTER_IMM doesn't work, perhaps because somehow the
> MI_SET_CONTEXT is really slow, and the GPU executes the register load
> out of order, thus invoking the original potential race.

If in doubt add a second MI_FLUSH, that is guaranteed to block on the
retirement of the first. But I suspect the actual bug is the misuse of
LOAD_REGISTER_IMM.
 
> But I followed up with the i915_gpu_idle(), and that too did not work...

Yeah, because the rings were idle (no outstanding requests and no active
buffers) it was a no-op. So there is no shortcut, you have to add a
request and then wait (or then idle). The alternative is to add the
HEAD!=TAIL polling to i915_gpu_idle as well, but i915_gpu_idle() is also
used during normal ops via evict_everything, so I'd rather keep it slim.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre