Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 13, 2012 at 04:50:39PM -0800, Keith Packard wrote:
> On Sat, 14 Jan 2012 01:31:40 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote:
> > > On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > acc101d drm/i915: Hold gt_lock across forcewake register reads
> > > > 
> > > > Imo this is a simple cleanup (reading forcewake-protected registers isn't
> > > > really a fast-path for us), so material for -next.
> > > 
> > > The 'optimization' is just a side benefit. The fix is to prevent reads
> > > From happening without forcewake being set.
> > 
> > I still fail to see how you can sneak a read in there without forcewake
> > being asserted. And assuming I haven't understood you the last time around
> > we've discussed this, you've agreed.
> 
> Yes, except during reset, where forcewake is cleared even if the lock
> count is non-zero. Any reads happening while reset is going on will
> return garbage. None of this is 'required' given the structure of the
> code today, it just makes it all evident without having to go through
> yet another long sequence of explanations.

I think that race is air-tight with your patch to rework the reset code
already. But better safe than sorry. And as I've said a good cleanup
anyway.

> I had patches to hold the spinlock across register writes too, as using
> different locking for reading and writing seems like a bad plan, but I
> didn't put those in because writes involve spinning to wait for the fifo
> to drain, and that seemed like a bad thing to do while holding the
> spinlock.

One of the reasons Chris originally shot down Ben's forcewake patches
which protected everything with a spinlock (i.e. also writes) is the
overhead. And writes to advance the ring are actually rather common. Iirc
Chris even wrote a patch to cut down on the overhead by caching the fifo
count. So I think we actually want this asymmetry in locking for
performance reasons.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48