Re: [PATCH 2/3] drm/i915: unify GT/PM irq postinstall code

[Ben Widawsky <ben@bwidawsk.net>]

On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote:
> > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote:
> > > Again extract a common helper. For the postinstall hook things are a
> > > bit more complicated since we have more cases on ilk-hsw/vlv here.
> > > 
> > > But since vlv was clearly broken by failing to initialize
> > > dev_priv->gt_irq_mask correctly the shared code is clearly justified.
> > > 
> > > Also kill the PMIER setting in the async rps enable work. I should
> > > have been save, but also clearly looked rather fragile. PMIER setup is
> > Can you fix up this sentence. It's a bit weird, and I'm not positive
> > what you're trying to say.
> 
> Stab at your VECS enabling patches which touch PMIER in the rps enabling
> work. After this patch PMIER is only set up (once) in the postinstall hook
> with all interrupts we ever want already enabled. We only update the PMIMR
> register when we enable rps, like with all other interrupts enabled after
> irq install time.
> 
> > > not all down in the irq pre/postinstall hooks.
> 
> s/not/now/ here probably clarifies it?
> 
> > > 
> > > With this we now have the usual interrupt register sequence for GT/PM
> > > irq registers:
> > > 
> > > - IER is setup once with all the interrupts we ever need in the
> > >   postinstall hook and never touched again. Exceptions are SDEIER,
> > >   which is touched in the preinstall hook (when the irq handler isn't
> > >   enabled) and then only from the irq handler. And DEIER/VLV_IER with
> > >   is used in the irq handler but also written to once in the
> > >   postinstall hook. But since that write is essentially what enables
> > >   the interrupt and we should always have MSI interrupts we should be
> > >   save. In case we ever have non-MSI interrupts we'd be screwed.
> > > 
> > > - IIR is cleared in the postinstall hook before we enable/unmask the
> > >   respective interrupt sources. Hence we can't steal an interrupt
> > >   event an accidentally trigger the spurious interrupt logic in the
> > >   core kernel. Note that after some discussion with Ben Widawsky we
> > >   think that we actually should clear the IIR registers in the
> > >   preinstall hook. But doing that is a much larger patch series.
> > > 
> > > - IMR regs are (usually) all masked off. Those are the only regs
> > >   changed at runtime, which is all protected by dev_priv->irq_lock.
> > > 
> > 
> > This comment block is really nice. Maybe supplement it with explanation
> > about how the ring interrupts work, and shove it in i915_irq.c?
> 
> I'm not a fan of big overview comments. The get outdated really fast and
> no one ever bothers to update them. In the commit message they're
> annotated to a clear point in our history with the implication that they
> have been reviewed to be correct at that point.
> 
> > 
> > > This unification also kills the cargo-culted read-modify-write PM
> > > register setup for VECS. Interrupt setup is done without userspace
> > > being able to interfere, so we better know what values we want to put
> > > into those registers. RMW cycles otoh are really good at papering over
> > > races, until stuff magically blows up and no one has a clue why.
> > > 
> > > v2: Touch the gen6+ PM interrupt registers only on gen6+.
> > > 
> > > v3: Improve the commit message to more clearly spell out why we want
> > > to unify the code and what exactly changes.
> > > 
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > Cc: Paulo Zanoni <przanoni@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++----------------------
> > >  drivers/gpu/drm/i915/intel_pm.c |  4 --
> > >  2 files changed, 42 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index d5c3bef..ba61bc7 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> > >  	I915_WRITE(SDEIMR, ~mask);
> > >  }
> > >  
> > > +static void gen5_gt_irq_postinstall(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	u32 pm_irqs, gt_irqs;
> > > +
> > > +	pm_irqs = gt_irqs = 0;
> > > +
> > > +	dev_priv->gt_irq_mask = ~0;
> > > +	if (HAS_L3_GPU_CACHE(dev)) {
> > > +		dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > +		gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > +	}
> > 
> > Maybe while you're doing this, explain why the L3 parity interrupt is
> > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask
> 
> I'll add 
> 		/* L3 parity interrupt is always unmasked. */
> 
> before the gt_irq_mask assignemnt.
> 
> > 
> > > +
> > > +	gt_irqs |= GT_RENDER_USER_INTERRUPT;
> > > +	if (IS_GEN5(dev)) {
> > > +		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > > +			   ILK_BSD_USER_INTERRUPT;
> > > +	} else {
> > > +		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > > +	}
> > > +
> > > +	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > +	I915_WRITE(GTIER, gt_irqs);
> > > +	POSTING_READ(GTIER);
> > > +
> > > +	if (INTEL_INFO(dev)->gen >= 6) {
> > > +		pm_irqs |= GEN6_PM_RPS_EVENTS;
> > > +
> > > +		if (HAS_VEBOX(dev))
> > > +			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > > +
> > > +		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > > +		I915_WRITE(GEN6_PMIMR, 0xffffffff);
> > > +		I915_WRITE(GEN6_PMIER, pm_irqs);
> > > +		POSTING_READ(GEN6_PMIER);
> > > +	}
> > > +}
> > > +
> > 
> > The ordering still looks funky here to me and your comment in the commit
> > message hasn't convinced me otherwise. The order should be:
> > MASK
> > CLEAR
> > ENABLE
> > 
> > In your order there is a theoretical race after you've cleared, but
> > before you've masked. This is trivial to fix, if you want to avoid
> > reposting, it's fine with me.
> 
> Imo fixing that up is a separate patch series, I'm sticking to being
> consistent whats there already. In any case I vote for a bit of
> refactoring so that all the I.R register sequences use the same code, i.e.
> something like the DISABLE_AND_CLEAR_INTERRUPT macro I've proposed in
> another thread.
> 
> > 
> > >  static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  {
> > >  	unsigned long irqflags;
> > > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> > >  			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> > >  			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
> > > -	u32 gt_irqs;
> > >  
> > >  	dev_priv->irq_mask = ~display_mask;
> > >  
> > > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > >  			  DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT);
> > >  	POSTING_READ(DEIER);
> > >  
> > > -	dev_priv->gt_irq_mask = ~0;
> > > -
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT;
> > > -
> > > -	if (IS_GEN6(dev))
> > > -		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> > > -	else
> > > -		gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT |
> > > -			   ILK_BSD_USER_INTERRUPT;
> > > -
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	ibx_irq_postinstall(dev);
> > >  
> > > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  		DE_PLANEA_FLIP_DONE_IVB |
> > >  		DE_AUX_CHANNEL_A_IVB |
> > >  		DE_ERR_INT_IVB;
> > > -	u32 pm_irqs = GEN6_PM_RPS_EVENTS;
> > > -	u32 gt_irqs;
> > >  
> > >  	dev_priv->irq_mask = ~display_mask;
> > >  
> > > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  		   DE_PIPEA_VBLANK_IVB);
> > >  	POSTING_READ(DEIER);
> > >  
> > > -	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > -
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > > -		  GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > -
> > > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > > -	if (HAS_VEBOX(dev))
> > > -		pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> > > -
> > > -	/* Our enable/disable rps functions may touch these registers so
> > > -	 * make sure to set a known state for only the non-RPS bits.
> > > -	 * The RMW is extra paranoia since this should be called after being set
> > > -	 * to a known state in preinstall.
> > > -	 * */
> > > -	I915_WRITE(GEN6_PMIMR,
> > > -		   (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs);
> > > -	I915_WRITE(GEN6_PMIER,
> > > -		   (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs);
> > > -	POSTING_READ(GEN6_PMIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	ibx_irq_postinstall(dev);
> > >  
> > > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
> > >  static int valleyview_irq_postinstall(struct drm_device *dev)
> > >  {
> > >  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > > -	u32 gt_irqs;
> > >  	u32 enable_mask;
> > >  	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
> > >  	unsigned long irqflags;
> > > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> > >  	I915_WRITE(VLV_IIR, 0xffffffff);
> > >  	I915_WRITE(VLV_IIR, 0xffffffff);
> > >  
> > > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> > > -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > > -
> > > -	gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT |
> > > -		GT_BLT_USER_INTERRUPT;
> > > -	I915_WRITE(GTIER, gt_irqs);
> > > -	POSTING_READ(GTIER);
> > > +	gen5_gt_irq_postinstall(dev);
> > >  
> > >  	/* ack & enable invalid PTE error interrupts */
> > >  #if 0 /* FIXME: add support to irq handler for checking these bits */
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index fb4afaa..e609232 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev)
> > >  
> > >  	gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
> > >  
> > > -	/* requires MSI enabled */
> > > -	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
> > >  	spin_lock_irq(&dev_priv->irq_lock);
> > >  	/* FIXME: Our interrupt enabling sequence is bonghits.
> > >  	 * dev_priv->rps.pm_iir really should be 0 here. */
> > > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  
> > >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > >  
> > > -	/* requires MSI enabled */
> > > -	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
> > >  	spin_lock_irq(&dev_priv->irq_lock);
> > >  	WARN_ON(dev_priv->rps.pm_iir != 0);
> > >  	I915_WRITE(GEN6_PMIMR, 0);
> > 
> > With my comments in 1 & 2 address, they're both:
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> If you don't object to my comments above I'll bikeshed the patch while
> applying and add the missing comment and fix up the commit message.
> -Daniel

ack
-- 
Ben Widawsky, Intel Open Source Technology Center