On Fri, 2015-02-20 at 19:40 +0000, Chris Wilson wrote:
> On Fri, Feb 20, 2015 at 09:09:02PM +0200, Imre Deak wrote:
> > +static void intel_irq_set_state(struct drm_i915_private *dev_priv,
> > + bool enabled)
> > +{
> > + dev_priv->pm.irqs_enabled = enabled;
> > + /*
> > + * Before we unmask the interrupt or synchronize against it, make sure
> > + * that a corresponding interrupt handler running on another CPU sees
> > + * the updated irqs_enabled value.
> > + */
> > + smp_mb();
>
> I would like a comment here to say something like: before powering down
> the hardware make sure that we have no further access via pending
> interrupt handlers. At the moment, I read the block comment above and
> thought you were trying to imply something about the serialisation from
> synchronize_irq() - but the only reason I can see you want sync_irq here
> is to be sure that once the irq is disabled it is not going to run again.
Right. With sync_irq I want to wait for any running handler on another
CPU to finish. Subsequent ones due to any shared/spurious interrupts
will also not run since the barrier guarantees that they see already the
updated value and do an early return. Will add a comment about this.
>
> > + if (!enabled)
> > + synchronize_irq(dev_priv->dev->irq);
> > +}
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
