Re: [PATCH 3/3] drm/i915: extract rps interrupt enable/disable helpers

[Ben Widawsky <ben@bwidawsk.net>]

On Sun, Jul 14, 2013 at 11:35:46PM +0200, Daniel Vetter wrote:
> 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@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@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@gmail.com>
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > 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 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
> 

FWIW, I'd still prefer all the non interrupt handling code to be in
i915_irq.c (with intel_ringbuffer.c as the only exception), but I think
you make a fair point. 
As long as my other comments are not forgotten..
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center