On Sun, Sep 04, 2011 at 09:17:24PM +0200, Daniel Vetter wrote: > > > 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
I still fail to see the inconsitency. The value of PMR no longer matters once IER is cleared. What the wq or irq handler does after this point should be fine so long as everything is setup up properly at enable time. This is a minor detail. So I would rework some of your comments a bit, and I also think it's about time we create a central place to cancel workqueue items. Mostly because I think this patch is subject to a deadlock with struct_mutex (you're holding it when you call gen6_disable_rps(), but you're doing cancel_work_sync on a workqueue which attempts to acquire struct_mutex. Ben _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
