Re: [PATCH 3/3] drm/i915: close rps work vs. rps disable races

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Sep 04, 2011 at 05:23:30PM +0000, Ben Widawsky wrote:
> On Sun, Sep 04, 2011 at 05:35:02PM +0200, Daniel Vetter wrote:
> > The rps disabling code wasn't properly cancelling outstanding work
> > items. Also add a comment that explains why we're not racing with
> > the work item, that could again unmask interrupts.
> > 
> > This also fixes a bug on module unload because nothing was properly
> > syncing up with that work item, possibly leading to it being run after
> > freeing dev_priv or removing the module code.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 56a8554..ccd4600 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7483,10 +7483,16 @@ void gen6_disable_rps(struct drm_device *dev)
> >  
> >  	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >  	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > -	I915_WRITE(GEN6_PMIER, 0);
> > +	/* Complete PM interrupt masking here doesn't race with the rps work
> > +	 * item again unmasking PM interrupts because that is using PMIMR
> > +	 * (logically sitting atop of PMINTRMSK) to mask interrupts. */
> > +	cancel_work_sync(&dev_priv->rps_work);
> >  
> > +	/* Clear PMIMR and dev_priv->pm_iir in case outstanding work gets
> > +	 * cancelled before having run. */
> 
> This comment is wrong, you are clearing PMIER, not IMR. IMR gets cleared
> in enable() iirc

Bleh, now it's my turn to completely screw it up ;-) Still, I think some
comment that we muck around with different registers than the work item
would be good. And clearing PMIMR under the lock just for paranoia so
that setting dev_priv->pm_iir doesn't leave a strange interrupt mask
lingering around

> >  	spin_lock_irq(&dev_priv->rps_lock);
> >  	dev_priv->pm_iir = 0;
> > +	I915_WRITE(GEN6_PMIER, 0);
> >  	spin_unlock_irq(&dev_priv->rps_lock);
> >  
> >  	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> I'm not sure this actually fixes a problem. The existing code:
> 1. disables all interrupts (no more can occur).
> 2. sets pm_iir to 0 safe in rps lock
> <workqueues can run at this point, but IMR has no effect with IER = 0>
> 
> I think you should do the cancel work sync somewhere in the code before
> module unload (to be correct). I just don't think this fixes a race.

Well, we definitely need the cancel_work_sync in the unload path
somewhere. I prefer it in the rps disable function. In the context of the
other patches my patch description is a bit lousy because I've really only
wanted to fix the unload race (and the PMIMR inconsistency) with this
patch.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48