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 at 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 at 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 at ffwll.ch Mobile: +41 (0)79 365 57 48
