On Mon, 2014-12-01 at 14:29 -0200, Paulo Zanoni wrote:
> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@xxxxxxxxx>:
> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> > reasons for this:
> > 1. we call intel_enable_gt_powersave() without a preceeding
> > intel_disable_gt_powersave()
> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
>
> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> this be enough to prevent IIR from changing?
Note also, that during a subsequent RPS enabling, interrupts will be
unmasked in GEN6_PMINTRMSK, before calling gen6_enable_rps_interrupts().
So not masking the interrupts in PM_IMR here would lead to triggering
RPS interrupts before we explicitly enable them.
>
> Chris?
>
> >
> > 1. means RPS interrupts will remain enabled and can be serviced during
> > the HW initialization after a GPU reset. 2. means even if we called
> > gen6_disable_rps_interrupts() any new RPS interrupt during RPS
> > initialization would still propagate to PM_IIR too early (though
> > wouldn't be serviced).
> >
> > This patch solves the 2. issue by also masking interrupts in PM_IMR, the
> > following patch fixes 1. getting rid of the WARN. This also makes
> > intel_enable_gt_powersave() and intel_disable_gt_powersave() more
> > symmetric.
> >
> > Since gen6_disable_rps_interrupts() is called during driver loading with
> > i915 interrupts disabled add a new version of gen6_disable_pm_irq() that
> > doesn't WARN for this.
> >
> > Also while at it, get the irq_lock around the whole PM_IMR/IER/IIR
> > programming sequence and make sure that any queued PM_IIR bit is also
> > cleared.
> >
> > The WARN was caught by PRTS after I sent my previous RPS sanitizing
> > patchset and I could easily reproduce it on HSW. To actually fix it we
> > also need the next patch.
> >
> > Reported-by: He, Shuang <shuang.he@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++++++++++-------
> > 1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8d169e1..598e9689 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -231,9 +231,6 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> >
> > assert_spin_locked(&dev_priv->irq_lock);
> >
> > - if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > - return;
> > -
> > new_val = dev_priv->pm_irq_mask;
> > new_val &= ~interrupt_mask;
> > new_val |= (~enabled_irq_mask & interrupt_mask);
> > @@ -247,14 +244,26 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> >
> > void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > {
> > + if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > + return;
> > +
> > snb_update_pm_irq(dev_priv, mask, mask);
> > }
> >
> > -void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
> > + uint32_t mask)
> > {
> > snb_update_pm_irq(dev_priv, mask, 0);
> > }
> >
> > +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > + if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > + return;
> > +
> > + __gen6_disable_pm_irq(dev_priv, mask);
> > +}
> > +
> > void gen6_reset_rps_interrupts(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -289,16 +298,20 @@ void gen6_disable_rps_interrupts(struct drm_device *dev)
> >
> > cancel_work_sync(&dev_priv->rps.work);
> >
> > + spin_lock_irq(&dev_priv->irq_lock);
> > +
> > I915_WRITE(GEN6_PMINTRMSK, INTEL_INFO(dev_priv)->gen >= 8 ?
> > ~GEN8_PMINTR_REDIRECT_TO_NON_DISP : ~0);
> > +
> > + __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
> > ~dev_priv->pm_rps_events);
> > + I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> > + I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> >
> > - spin_lock_irq(&dev_priv->irq_lock);
> > dev_priv->rps.pm_iir = 0;
> > +
> > spin_unlock_irq(&dev_priv->irq_lock);
> > -
> > - I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> > }
> >
> > /**
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
