Re: [PATCH v2 6/8] drm/i915: sanitize rps irq enabling

[Imre Deak <imre.deak@intel.com>]

On Mon, 2014-11-17 at 16:49 -0200, Paulo Zanoni wrote:
> 2014-11-10 11:41 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > Atm we first enable the RPS interrupts then we clear any pending ones.
> > By this we could lose an interrupt arriving after we unmasked it. This
> > may not be a problem as the caller should handle such a race, but logic
> > still calls for the opposite order. Also we can delay enabling the
> > interrupts until after all the RPS initialization is ready with the
> > following order:
> >
> > 1. clear any pending RPS interrupts
> > 2. initialize RPS
> > 3. enable RPS interrupts
> >
> > This also allows us to do the 1. and 3. step the same way for all
> > platforms, so let's follow this order to simplifying things.
> >
> > Also make sure any queued interrupts are also cleared.
> >
> > v2:
> > - rebase on the GEN9 patches where we don't support RPS yet, so we
> >   musn't enable RPS interrupts on it (Paulo)
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c  | 11 ++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c  | 19 +++++++++++--------
> >  3 files changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 729e9a3..89a7be1 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -255,6 +255,16 @@ void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> >         snb_update_pm_irq(dev_priv, mask, 0);
> >  }
> >
> > +void gen6_reset_rps_interrupts(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       spin_lock_irq(&dev_priv->irq_lock);
> > +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> > +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> 
> What about adding POSTING_READ(gen6_pm_iir(dev_priv)) ?
> 
> (also maybe uint32_t reg = gen6_pm_iir(dev_priv))

Ok, will do.

> 
> > +       spin_unlock_irq(&dev_priv->irq_lock);
> > +}
> > +
> >  void gen6_enable_rps_interrupts(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -262,7 +272,6 @@ void gen6_enable_rps_interrupts(struct drm_device *dev)
> >         spin_lock_irq(&dev_priv->irq_lock);
> >         WARN_ON(dev_priv->rps.pm_iir);
> >         gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > -       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> 
> What about adding WARN_ON(I915_READ(gen6_pm_iir(dev_priv))) ?

This is more complicated, since the PM_IIR register has bits other than
RPS. So the WARN would makes sense, but we'd have to calculate a gen
specific mask. I could do this as a follow-up.

> 
> >         spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 2499348..fb2cf27 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -784,6 +784,7 @@ void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > +void gen6_reset_rps_interrupts(struct drm_device *dev);
> >  void gen6_enable_rps_interrupts(struct drm_device *dev);
> >  void gen6_disable_rps_interrupts(struct drm_device *dev);
> >  void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8d164bc..f555810 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4753,8 +4753,6 @@ static void gen8_enable_rps(struct drm_device *dev)
> >
> >         gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
> >
> > -       gen6_enable_rps_interrupts(dev);
> > -
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >
> > @@ -4851,8 +4849,6 @@ static void gen6_enable_rps(struct drm_device *dev)
> >         dev_priv->rps.power = HIGH_POWER; /* force a reset */
> >         gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >
> > -       gen6_enable_rps_interrupts(dev);
> > -
> >         rc6vids = 0;
> >         ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> >         if (IS_GEN6(dev) && ret) {
> > @@ -5344,8 +5340,6 @@ static void cherryview_enable_rps(struct drm_device *dev)
> >
> >         valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >
> > -       gen6_enable_rps_interrupts(dev);
> > -
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >
> > @@ -5424,8 +5418,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >
> >         valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >
> > -       gen6_enable_rps_interrupts(dev);
> > -
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >
> > @@ -6239,6 +6231,13 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >
> >         mutex_lock(&dev_priv->rps.hw_lock);
> >
> > +       /*
> > +        * TODO: reset/enable RPS interrupts on GEN9 too, once RPS support is
> > +        * added for it.
> > +        */
> > +       if (INTEL_INFO(dev)->gen != 9)
> > +               gen6_reset_rps_interrupts(dev);
> 
> I see that in this series you're using "gen != 9" checks, but I'd
> change them to "gen < 9", just in case...

Yep, makes sense. I'll update then the WARN in the IRQ handler too
accordingly.

> 
> > +
> >         if (IS_CHERRYVIEW(dev)) {
> >                 cherryview_enable_rps(dev);
> >         } else if (IS_VALLEYVIEW(dev)) {
> > @@ -6253,6 +6252,10 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >                 __gen6_update_ring_freq(dev);
> >         }
> >         dev_priv->rps.enabled = true;
> > +
> > +       if (INTEL_INFO(dev)->gen != 9)
> > +               gen6_enable_rps_interrupts(dev);
> > +
> >         mutex_unlock(&dev_priv->rps.hw_lock);
> >
> >         intel_runtime_pm_put(dev_priv);
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx