| Message ID | 1371037046-3732-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/6/12 Daniel Vetter <daniel.vetter@ffwll.ch>: > By the time we write DEIER in the postinstall hook the interrupt > handler could run any time. And it does modify DEIER to handle > interrupts. > > Hence the DEIER read-modify-write cycle for enabling the PCU event > source is racy. Close this races the same way we handle vblank > interrupts: Unconditionally enable the interrupt in the IER register, > but conditionally mask it in IMR. The later poses no such race since > the interrupt handler does not touch DEIMR. > > Also update the comment, the clearing has already happened > unconditionally above. I don't see an "updated comment". I guess you wanted to s/Clear & enable/Enable/ ? Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 567945f..969da20 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2658,7 +2658,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > /* should always can generate irq */ > I915_WRITE(DEIIR, I915_READ(DEIIR)); > I915_WRITE(DEIMR, dev_priv->irq_mask); > - I915_WRITE(DEIER, display_mask | DE_PIPEA_VBLANK | DE_PIPEB_VBLANK); > + I915_WRITE(DEIER, display_mask | > + DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); > POSTING_READ(DEIER); > > dev_priv->gt_irq_mask = ~0; > @@ -2680,11 +2681,9 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > ibx_irq_postinstall(dev); > > if (IS_IRONLAKE_M(dev)) { > - /* Clear & enable PCU event interrupts */ > - I915_WRITE(DEIIR, DE_PCU_EVENT); > - I915_WRITE(DEIER, I915_READ(DEIER) | DE_PCU_EVENT); > - > - /* spinlocking not required here for correctness since interrupt > + /* Clear & enable PCU event interrupts > + * > + * spinlocking not required here for correctness since interrupt > * setup is guaranteed to run in single-threaded context. But we > * need it to make the assert_spin_locked happy. */ > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 567945f..969da20 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2658,7 +2658,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) /* should always can generate irq */ I915_WRITE(DEIIR, I915_READ(DEIIR)); I915_WRITE(DEIMR, dev_priv->irq_mask); - I915_WRITE(DEIER, display_mask | DE_PIPEA_VBLANK | DE_PIPEB_VBLANK); + I915_WRITE(DEIER, display_mask | + DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); POSTING_READ(DEIER); dev_priv->gt_irq_mask = ~0; @@ -2680,11 +2681,9 @@ static int ironlake_irq_postinstall(struct drm_device *dev) ibx_irq_postinstall(dev); if (IS_IRONLAKE_M(dev)) { - /* Clear & enable PCU event interrupts */ - I915_WRITE(DEIIR, DE_PCU_EVENT); - I915_WRITE(DEIER, I915_READ(DEIER) | DE_PCU_EVENT); - - /* spinlocking not required here for correctness since interrupt + /* Clear & enable PCU event interrupts + * + * spinlocking not required here for correctness since interrupt * setup is guaranteed to run in single-threaded context. But we * need it to make the assert_spin_locked happy. */ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
By the time we write DEIER in the postinstall hook the interrupt handler could run any time. And it does modify DEIER to handle interrupts. Hence the DEIER read-modify-write cycle for enabling the PCU event source is racy. Close this races the same way we handle vblank interrupts: Unconditionally enable the interrupt in the IER register, but conditionally mask it in IMR. The later poses no such race since the interrupt handler does not touch DEIMR. Also update the comment, the clearing has already happened unconditionally above. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)