On Sun, 4 Sep 2011 23:51:52 -0700, Ben Widawsky <b...@bwidawsk.net> wrote: > On Mon, 5 Sep 2011 08:38:07 +0200 > Daniel Vetter <dan...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx