[Intel-gfx] [PATCH 2/3] drm/i915: close PM interrupt masking races in the rps work func

[Chris Wilson]

On Sun, 4 Sep 2011 23:51:52 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Mon, 5 Sep 2011 08:38:07 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Sun, Sep 04, 2011 at 09:38:56PM +0000, Ben Widawsky wrote:
> > > Oops, you're totally right, I think I meant:
> > > -       I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir);
> > > +       I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
> > 
> > Imo still racy without the irqsafe rps_lock around it. gcc is free to
> > compile that into a separate load and store which the irq handler can
> > get in between and change dev_priv->pm_iir and PMIMR. The race is now
> > only one instruction wide, though ;-)
> > -Daniel
> 
> You are absolutely correct. The modification to GEN6_PMIMR must be
> within the protection of rps_lock.

Right. However, I don't see why we need to hold the mutex though. Why
are we touching PMIMR in gen6_enable_rps()? Afaics, it is because
gen6_disable_rps() may leave a stale PMIMR (it sets dev_priv->pm_iir to
0, causing the existing work-handler to return early and not touch
PMIMR).

I believe everything is correct if we clear PMIMR on module load, remove
the setting of PMIMR from gen6_enable_rps() and clear PMIMR under the
rps lock at the top of the work handler (which both Daniel and I have
desired to do... ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre