On Sun, Sep 04, 2011 at 09:26:48PM +0200, Daniel Vetter wrote: > On Sun, Sep 04, 2011 at 10:08:17AM -0700, Ben Widawsky wrote: > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 55518e3..3bc1479 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -415,12 +415,7 @@ static void gen6_pm_rps_work(struct work_struct *work) > > gen6_set_rps(dev_priv->dev, new_delay); > > dev_priv->cur_delay = new_delay; > > > > - /* > > - * rps_lock not held here because clearing is non-destructive. > > There is > > - * an *extremely* unlikely race with gen6_rps_enable() that is > > prevented > > - * by holding struct_mutex for the duration of the write. > > - */ > > - I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); > > + I915_WRITE(GEN6_PMIMR, pm_imr & dev_priv->pm_iir); > > mutex_unlock(&dev_priv->dev->struct_mutex); > > } > > For this to work we'd need to hold the rps_lock (to avoid racing with the > irq handler). But imo my approach is conceptually simpler: The work func > grabs all oustanding PM interrupts and then enables them again in one go > (protected by rps_lock).
I agree your approach is similar, but I think we should really consider whether my approach actually requires the lock. I *think* it doesn't. At least in my head my patch should fix the error you spotted. I don't know, maybe I need to think some more. The reason I worked so hard to avoid doing it the way you did in my original implementation is I was trying really hard to not break the cardinal rule about minimizing time holding spinlock_irqs. To go with the other patch, you probably want a POSTING_READ also before releasing the spin_lock (though I think this is being a bit paranoid). Again I need to think on this some more, but I'm also not terribly fond of ever unconditionally setting a value to 0 as it tends to complicate things when adding new readers or writers to the system. In summary, let me think about whether my solution actually won't work (feel free to contribute to my thinking), and if it doesn't then the decision is easy. Ben _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
