2014-11-10 11:41 GMT-02:00 Imre Deak <imre.deak@xxxxxxxxx>: > 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@xxxxxxxxx> > --- > 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)) > + 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))) ? > 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... > + > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx