Re: [PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Apr 22, 2011 at 09:20:17AM -0700, Ben Widawsky wrote:
> On Thu, Apr 21, 2011 at 07:18:24AM +0100, Chris Wilson wrote:
> > On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Just to annoy you, this needs to be split up into the various categories
> > of fixes. Because...
> > 
> > >  static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
> > > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> > >  	intel_disable_pll(dev_priv, pipe);
> > >  
> > >  	intel_crtc->active = false;
> > > +
> > > +	mutex_lock(&dev->struct_mutex);
> > >  	intel_update_fbc(dev);
> > >  	intel_update_watermarks(dev);
> > >  	intel_clear_scanline_wait(dev);
> > > +	mutex_unlock(&dev->struct_mutex);
> > >  }
> > 
> > This is overly correct. You can put a comment here to say that we will
> > never attempt to use FORCEWAKE here and that these registers are protected
> > by the mode_config lock. Except for intel_clear_scanline_wait, but that
> > itself is is longing to be killed now. If we haven't fixed the underlying
> > bug that we were working around by now, we have been too lax.
> > -Chris
> 
> I don't understand what you're asking for. I'm pretty convinced I need
> the mutex protected intel_update_fbc, because the call trace could be:
> 
> intel_update_fbc()
> intel_enable_fbc()
> ironlake_enable_fbc()
> sandybridge_blit_fbc_update()
> gen6_gt_force_wake_get()
> 
> 
> Could you elaborate?
> 
> Ben

Crap. I wasn't paying attention. You're right, I did this to be
symmetric with the other code. I can remove it if you prefer.