diff mbox

[13/14] drm/i915: extract rps interrupt enable/disable helpers

Message ID 1372973734-7601-14-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 4, 2013, 9:35 p.m. UTC 
This just unifies the vlv code with the snb-hsw code which matched
exactly before the VECS enabling.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

Comments

Paulo Zanoni July 10, 2013, 9:12 p.m. UTC | #1 
2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This just unifies the vlv code with the snb-hsw code which matched
> exactly before the VECS enabling.

So now the VLV code is behaving differently. Is that intentional? You
could write about the implications here.


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 96f0872..787a528 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>         trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
>  }
>
> -
> -static void gen6_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps_interrupts(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       I915_WRITE(GEN6_RC_CONTROL, 0);
> -       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>         I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>         I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
>         /* Complete PM interrupt masking here doesn't race with the rps work
> @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
>         I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>  }
>
> -static void valleyview_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         I915_WRITE(GEN6_RC_CONTROL, 0);
> -       I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> -       I915_WRITE(GEN6_PMIER, 0);
> -       /* Complete PM interrupt masking here doesn't race with the rps work
> -        * item again unmasking PM interrupts because that is using a different
> -        * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> -        * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> +       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       dev_priv->rps.pm_iir = 0;
> -       spin_unlock_irq(&dev_priv->irq_lock);
> +       gen6_disable_rps_interrupts(dev);
> +}
> +
> +static void valleyview_disable_rps(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       I915_WRITE(GEN6_RC_CONTROL, 0);
>
> -       I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +       gen6_disable_rps_interrupts(dev);
>
>         if (dev_priv->vlv_pctx) {
>                 drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
>         return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
>  }
>
> +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       spin_lock_irq(&dev_priv->irq_lock);
> +       /* FIXME: Our interrupt enabling sequence is bonghits.
> +        * dev_priv->rps.pm_iir really should be 0 here. */
> +       dev_priv->rps.pm_iir = 0;
> +       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> +       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> +       spin_unlock_irq(&dev_priv->irq_lock);
> +       /* unmask all PM interrupts */
> +       I915_WRITE(GEN6_PMINTRMSK, 0);
> +}
> +
>  static void gen6_enable_rps(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>
>         gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       /* FIXME: Our interrupt enabling sequence is bonghits.
> -        * dev_priv->rps.pm_iir really should be 0 here. */
> -       dev_priv->rps.pm_iir = 0;
> -       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> -       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> -       spin_unlock_irq(&dev_priv->irq_lock);
> -       /* unmask all PM interrupts */
> -       I915_WRITE(GEN6_PMINTRMSK, 0);
> +       gen6_enable_rps_interrupts(dev);
>
>         rc6vids = 0;
>         ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>
>         valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       WARN_ON(dev_priv->rps.pm_iir != 0);
> -       I915_WRITE(GEN6_PMIMR, 0);
> -       spin_unlock_irq(&dev_priv->irq_lock);
> -       /* enable all PM interrupts */
> -       I915_WRITE(GEN6_PMINTRMSK, 0);
> +       gen6_enable_rps_interrupts(dev);
>
>         gen6_gt_force_wake_put(dev_priv);
>  }
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter July 11, 2013, 6:20 a.m. UTC | #2 
On Wed, Jul 10, 2013 at 06:12:13PM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > This just unifies the vlv code with the snb-hsw code which matched
> > exactly before the VECS enabling.
> 
> So now the VLV code is behaving differently. Is that intentional? You
> could write about the implications here.

It's just for consistency that we now also reset GEN6_PMINTRMSK for vlv
and that the additional clearing of the rps event bits in PMIIR now
consistently happens in enable_interrupts. Like I've said in the next
commit message, the PMIIR handling here still smells a bit funny - as long
as we properly clear them at irq install time and keep the actual rps
events masked we should never see PMIIR bits here.
-Daniel

> 
> 
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 59 ++++++++++++++++++++---------------------
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 96f0872..787a528 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3121,13 +3121,10 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> >         trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
> >  }
> >
> > -
> > -static void gen6_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps_interrupts(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > -       I915_WRITE(GEN6_RC_CONTROL, 0);
> > -       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >         I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> >         I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
> >         /* Complete PM interrupt masking here doesn't race with the rps work
> > @@ -3142,23 +3139,23 @@ static void gen6_disable_rps(struct drm_device *dev)
> >         I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> >  }
> >
> > -static void valleyview_disable_rps(struct drm_device *dev)
> > +static void gen6_disable_rps(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >         I915_WRITE(GEN6_RC_CONTROL, 0);
> > -       I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > -       I915_WRITE(GEN6_PMIER, 0);
> > -       /* Complete PM interrupt masking here doesn't race with the rps work
> > -        * item again unmasking PM interrupts because that is using a different
> > -        * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> > -        * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> > +       I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       dev_priv->rps.pm_iir = 0;
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > +       gen6_disable_rps_interrupts(dev);
> > +}
> > +
> > +static void valleyview_disable_rps(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       I915_WRITE(GEN6_RC_CONTROL, 0);
> >
> > -       I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +       gen6_disable_rps_interrupts(dev);
> >
> >         if (dev_priv->vlv_pctx) {
> >                 drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> > @@ -3191,6 +3188,21 @@ int intel_enable_rc6(const struct drm_device *dev)
> >         return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> >  }
> >
> > +static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       spin_lock_irq(&dev_priv->irq_lock);
> > +       /* FIXME: Our interrupt enabling sequence is bonghits.
> > +        * dev_priv->rps.pm_iir really should be 0 here. */
> > +       dev_priv->rps.pm_iir = 0;
> > +       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > +       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > +       spin_unlock_irq(&dev_priv->irq_lock);
> > +       /* unmask all PM interrupts */
> > +       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +}
> > +
> >  static void gen6_enable_rps(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3319,15 +3331,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >
> >         gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       /* FIXME: Our interrupt enabling sequence is bonghits.
> > -        * dev_priv->rps.pm_iir really should be 0 here. */
> > -       dev_priv->rps.pm_iir = 0;
> > -       I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> > -       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > -       /* unmask all PM interrupts */
> > -       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +       gen6_enable_rps_interrupts(dev);
> >
> >         rc6vids = 0;
> >         ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > @@ -3597,12 +3601,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >
> >         valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       WARN_ON(dev_priv->rps.pm_iir != 0);
> > -       I915_WRITE(GEN6_PMIMR, 0);
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> > -       /* enable all PM interrupts */
> > -       I915_WRITE(GEN6_PMINTRMSK, 0);
> > +       gen6_enable_rps_interrupts(dev);
> >
> >         gen6_gt_force_wake_put(dev_priv);
> >  }
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96f0872..787a528 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3121,13 +3121,10 @@  void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
 }
 
-
-static void gen6_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
 	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
 	/* Complete PM interrupt masking here doesn't race with the rps work
@@ -3142,23 +3139,23 @@  static void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 }
 
-static void valleyview_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	I915_WRITE(GEN6_RC_CONTROL, 0);
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-	I915_WRITE(GEN6_PMIER, 0);
-	/* Complete PM interrupt masking here doesn't race with the rps work
-	 * item again unmasking PM interrupts because that is using a different
-	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
-	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
+	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir = 0;
-	spin_unlock_irq(&dev_priv->irq_lock);
+	gen6_disable_rps_interrupts(dev);
+}
+
+static void valleyview_disable_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_RC_CONTROL, 0);
 
-	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
+	gen6_disable_rps_interrupts(dev);
 
 	if (dev_priv->vlv_pctx) {
 		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
@@ -3191,6 +3188,21 @@  int intel_enable_rc6(const struct drm_device *dev)
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
+static void gen6_enable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	/* FIXME: Our interrupt enabling sequence is bonghits.
+	 * dev_priv->rps.pm_iir really should be 0 here. */
+	dev_priv->rps.pm_iir = 0;
+	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
+	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
+	spin_unlock_irq(&dev_priv->irq_lock);
+	/* unmask all PM interrupts */
+	I915_WRITE(GEN6_PMINTRMSK, 0);
+}
+
 static void gen6_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3319,15 +3331,7 @@  static void gen6_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	/* FIXME: Our interrupt enabling sequence is bonghits.
-	 * dev_priv->rps.pm_iir really should be 0 here. */
-	dev_priv->rps.pm_iir = 0;
-	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
-	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* unmask all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -3597,12 +3601,7 @@  static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	WARN_ON(dev_priv->rps.pm_iir != 0);
-	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->irq_lock);
-	/* enable all PM interrupts */
-	I915_WRITE(GEN6_PMINTRMSK, 0);
+	gen6_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv);
 }