[Intel-gfx] [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS

[Paulo Zanoni]

2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak at intel.com>:
> 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?

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 at intel.com>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni