On Sun, Jul 14, 2013 at 02:06:28PM -0700, Ben Widawsky wrote:
> 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 <ben at 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 <ben at 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 <przanoni at gmail.com>
> > Cc: Ben Widawsky <ben at bwidawsk.net>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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.
I certainly won't hurt, but again I think a separate patch series. Atm
most of our uninstall code doesn't use POSTING_READs at all.
>
> > /* 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.
Next patch (i.e. the final one, which I haven't resent) does that. And
you've slapped your r-b onto it already.
>
> > + 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?
More forward declarations, which are my rough (and sometimes misguided
metric) to judge whether a function is at the right place. Hence I've
decided against moving it. Has the downside that not all code touching
PMIMR is at the same place, but alas. We can't share a commen
enable/disable_pm_interrupts like we have for display interrupt registers
and similar stuff since we want to do a bit more than just set up
interrupt registers under the spinlock protection.
Also, the other guy toching PMIMR at runtime lives in intel_ringbuffer.c,
so moving this to i915_irq.c won't bring them any closer.
-Daniel
>
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
