On Fri, Jul 12, 2013 at 10:43:27PM +0200, Daniel Vetter wrote:
> The VECS enabling required some changes to how rps interrupts are
> enabled/disabled since VECS interrupts are handling with the PM
> interrupt registers.
> 
> But now that the pre/postinstall sequences is identical for all
> platforms with rps support (snb, ivb, hsw, vlv) we can also use the
> exact same sequence to actually enable the rps interrupts. Strictly
> speaking using spinlocks is overkill on snb/ivb & vlv since they have
> no VECS ring, but imo that's more than made up by the common code.
> 
> Hence this just unifies the vlv code with the snb-hsw code which
> matched exactly before the VECS enabling. See
> 
> commit eda63ffb906c2fb3b609a0e87aeb63c0f25b9e6b
> Author: Ben Widawsky <b...@bwidawsk.net>
> Date:   Tue May 28 19:22:26 2013 -0700
> 
>     drm/i915: Add PM regs to pre/post install
> 
> and
> 
> commit 4848405cced3b46f4ec7d404b8ed5873171ae10a
> Author: Ben Widawsky <b...@bwidawsk.net>
> Date:   Tue May 28 19:22:27 2013 -0700
> 
>     drm/i915: make PM interrupt writes non-destructive
> 
> for why the gen6 code (shared between snb, ivb and hsw) needed to be
> changed originally.
> 
> v3: Improve the commit message to more clearly spell out why we want
> to unify the code and what exactly changes.
> 
> Cc: Paulo Zanoni <przan...@gmail.com>
> Cc: Ben Widawsky <b...@bwidawsk.net>
> Signed-off-by: Daniel Vetter <daniel.vet...@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 e609232..190ab96 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);

I think actually, we need a POSTING_READ here before clearing
dev_priv->rps.pm_iir. Or maybe just one after disabling RPS is enough.
Not sure. Maybe just a hypothetical race.

>       /* 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);

I thought for enable, we want to modify PMIER? If it's not required, I
want a comment when not. Also, with your changes, it should be safe to
remove the FIXME if I am not mistaken.

> +     I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> +     spin_unlock_irq(&dev_priv->irq_lock);
> +     /* unmask all PM interrupts */
> +     I915_WRITE(GEN6_PMINTRMSK, 0);
> +}
> +

Also, maybe move this (with disable interrupts) to i915_irq.c?

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

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to