Re: [PATCH 09/18] drm/i915: make PM interrupt writes non-destructive

From: Ben Widawsky <ben@bwidawsk.net>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/18] drm/i915: make PM interrupt writes non-destructive
Date: Wed, 7 Nov 2012 11:53:25 +0000	[thread overview]
Message-ID: <20121107115325.4d0308a8@bwidawsk.net> (raw)
In-Reply-To: <eeac1e$4tp0pn@AZSMGA002.ch.intel.com>

On Wed, 07 Nov 2012 10:17:09 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue,  6 Nov 2012 16:25:33 +0000, Ben Widawsky <ben@bwidawsk.net> wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8f15616..5477454 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2354,7 +2354,7 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >  	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >  	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > -	I915_WRITE(GEN6_PMIER, 0);
> > +	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) & ~GEN6_PM_RPS_EVENTS);
> >  	/* 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
> > @@ -2364,7 +2364,7 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  	dev_priv->rps.pm_iir = 0;
> >  	spin_unlock_irq(&dev_priv->rps.lock);
> >  
> > -	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR) & ~GEN6_PM_RPS_EVENTS);
> >  }
> >  
> >  int intel_enable_rc6(const struct drm_device *dev)
> > @@ -2518,10 +2518,10 @@ 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, GEN6_PM_DEFERRED_EVENTS);
> > +	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
> >  	spin_lock_irq(&dev_priv->rps.lock);
> >  	WARN_ON(dev_priv->rps.pm_iir != 0);
> > -	I915_WRITE(GEN6_PMIMR, 0);
> > +	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR) & GEN6_PM_RPS_EVENTS);
> >  	spin_unlock_irq(&dev_priv->rps.lock);
> >  	/* enable all PM interrupts */
> >  	I915_WRITE(GEN6_PMINTRMSK, 0);
> 
> Totally confused.
> 
> IER = IER & ~RPS // only disable the RPS events
> IIR = IIR & RPS // clear the asserted RPS bits
> 
> IMR != IIR.
> -Chris
> 

So yes, there is a bug here I think. To make sure you aren't referring
to something else though I'll elaborate, and add this to the commit
message in v2.

At preinstall all interrupts are masked and disabled. That will only
happen at driver load time. Then the above code enable/disable rc6
happens also at resume, so a little more care is taken.

The IMR is only touched by the workqueue, so enable/disable touch IER
and IIR.

Disable (there is a bug here you found):
Disable RPS events, optionally clear IIR RPS interrupts.

+	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR) & ~GEN6_PM_RPS_EVENTS);
should be either nothing at all (since we clear on enable anyway), or:
+	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);

Enable (this is okay as it stands, I think):
Enable RPS events, clear IIR RPS interrupts.

+	I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
+	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR) & GEN6_PM_RPS_EVENTS);

-- 
Ben Widawsky, Intel Open Source Technology Center