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

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

[Imre Deak]

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.

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  | 13 +++++--------
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ce4139e..68f65ec 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);
+	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);
 	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 6d275c9..8a0583b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -791,6 +791,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 2c8bbfc..803676f 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);
 }
 
@@ -6234,6 +6226,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
+	gen6_reset_rps_interrupts(dev);
+
 	if (IS_CHERRYVIEW(dev)) {
 		cherryview_enable_rps(dev);
 	} else if (IS_VALLEYVIEW(dev)) {
@@ -6248,6 +6242,9 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 		__gen6_update_ring_freq(dev);
 	}
 	dev_priv->rps.enabled = true;
+
+	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

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

[Imre Deak]

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);
+	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);
 	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);
+
 	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

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

[Chris Wilson]

On Mon, Nov 10, 2014 at 03:41:05PM +0200, Imre Deak wrote:
> 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:
> 

0. disable left-over RPS
> 1. clear any pending RPS interrupts
> 2. initialize RPS
> 3. enable RPS interrupts
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Mon, 2014-11-10 at 13:45 +0000, Chris Wilson wrote:
> On Mon, Nov 10, 2014 at 03:41:05PM +0200, Imre Deak wrote:
> > 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:
> > 
> 
> 0. disable left-over RPS

Isn't enough relying on
intel_uncore_sanitize()->intel_disable_gt_powersave()?

> > 1. clear any pending RPS interrupts
> > 2. initialize RPS
> > 3. enable RPS interrupts
> -Chris
> 


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

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

[Chris Wilson]

On Mon, Nov 10, 2014 at 04:13:02PM +0200, Imre Deak wrote:
> On Mon, 2014-11-10 at 13:45 +0000, Chris Wilson wrote:
> > On Mon, Nov 10, 2014 at 03:41:05PM +0200, Imre Deak wrote:
> > > 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:
> > > 
> > 
> > 0. disable left-over RPS
> 
> Isn't enough relying on
> intel_uncore_sanitize()->intel_disable_gt_powersave()?

That should be enough. It's an important step to remember though :)

> > > 1. clear any pending RPS interrupts
> > > 2. initialize RPS
> > > 3. enable RPS interrupts
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Mon, 2014-11-10 at 14:15 +0000, Chris Wilson wrote:
> On Mon, Nov 10, 2014 at 04:13:02PM +0200, Imre Deak wrote:
> > On Mon, 2014-11-10 at 13:45 +0000, Chris Wilson wrote:
> > > On Mon, Nov 10, 2014 at 03:41:05PM +0200, Imre Deak wrote:
> > > > 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:
> > > > 
> > > 
> > > 0. disable left-over RPS
> > 
> > Isn't enough relying on
> > intel_uncore_sanitize()->intel_disable_gt_powersave()?
> 
> That should be enough. It's an important step to remember though :)

Ok. Btw, I also thought of clearing the interrupts right before enabling
them which would've made things simpler. I wasn't sure though if we
could lose some interrupt that the init step would trigger; though that
may not be an issue. In any case this looked like the more robust order.

> > > > 1. clear any pending RPS interrupts
> > > > 2. initialize RPS
> > > > 3. enable RPS interrupts
> -Chris
> 


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

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

[Paulo Zanoni]

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))

> +       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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

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

[Imre Deak]

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