diff mbox

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

Message ID 1367110769-1306-10-git-send-email-ben@bwidawsk.net (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky April 28, 2013, 12:59 a.m. UTC
PM interrupts have an expanded role on HSW. It helps route the EBOX
interrupts. This patch is necessary to make the existing code which
touches the mask, and enable registers more friendly to other code paths
that also will need these registers.

To be more explicit:
At preinstall all interrupts are masked and disabled. This implies that
preinstall should always happen before any enabling/disabling of RPS or
other interrupts.

The PMIMR is touched by the workqueue, so enable/disable touch IER and
IIR. Similarly, the code currently expects IMR has no use outside of the
RPS related interrupts so they unconditionally set 0, or ~0. We could
use IER in the workqueue, and IMR elsewhere, but since the workqueue
use-case is more transient the existing usage makes sense.

Disable RPS events:
IER := IER & ~GEN6_PM_RPS_EVENTS // Disable RPS related interrupts
IIR := GEN6_PM_RPS_EVENTS // Disable any outstanding interrupts

Enable RPS events:
IER := IER | GEN6_PM_RPS_EVENTS // Enable the RPS related interrupts
IIR := GEN6_PM_RPS_EVENTS // Make sure there were no leftover events
(really shouldn't happen)

v2: Shouldn't destroy PMIIR or PMIMR VEBOX interrupt state in
enable/disable rps functions (Haihao)

v3: Bug found by Chris where we were clearing the wrong bits at rps
disable.
    expanded commit message

v4: v3 was based off the wrong branch

v5: Updated for VLV

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++--------
 drivers/gpu/drm/i915/i915_reg.h |  2 +-
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
 3 files changed, 16 insertions(+), 15 deletions(-)

Comments

Lespiau, Damien May 28, 2013, 1:30 p.m. UTC | #1
On Sat, Apr 27, 2013 at 05:59:20PM -0700, Ben Widawsky wrote:
> @@ -2720,12 +2720,12 @@ 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, GEN6_PM_RPS_EVENTS);

You're not unmasking the RPS interrupts in PMIMR here now. I'm missing
how they are enabled now.

>  	spin_unlock_irq(&dev_priv->rps.lock);
> -	/* enable all PM interrupts */
> +	/* unmask all PM interrupts */
>  	I915_WRITE(GEN6_PMINTRMSK, 0);
>
>  	rc6vids = 0;
Ben Widawsky May 28, 2013, 6:02 p.m. UTC | #2
On Tue, May 28, 2013 at 02:30:56PM +0100, Damien Lespiau wrote:
> On Sat, Apr 27, 2013 at 05:59:20PM -0700, Ben Widawsky wrote:
> > @@ -2720,12 +2720,12 @@ 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, GEN6_PM_RPS_EVENTS);
> 
> You're not unmasking the RPS interrupts in PMIMR here now. I'm missing
> how they are enabled now.

You are right. It's fixed in a later patch IFF we have VEBOX, but would
regress on IVB and HSW. Both this patch, and that are easily fixed. BRB.

> 
> >  	spin_unlock_irq(&dev_priv->rps.lock); -	/* enable all PM
> >  	interrupts */ +	/* unmask all PM interrupts */
> >  	I915_WRITE(GEN6_PMINTRMSK, 0);
> >
> >  	rc6vids = 0;
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98af4fe..13ea6c2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -701,10 +701,11 @@  static void gen6_pm_rps_work(struct work_struct *work)
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
 	pm_imr = I915_READ(GEN6_PMIMR);
-	I915_WRITE(GEN6_PMIMR, 0);
+	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
+	I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->rps.lock);
 
-	if ((pm_iir & GEN6_PM_DEFERRED_EVENTS) == 0)
+	if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
 		return;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -934,17 +935,17 @@  static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->rps.lock, flags);
-	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_DEFERRED_EVENTS;
+	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 	if (dev_priv->rps.pm_iir) {
 		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 		/* We never want to mask useful interrupts. (also posting read) */
-		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_DEFERRED_EVENTS);
+		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
 		/* TODO: if queue_work is slow, move it out of the spinlock */
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
 	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
 
-	if (pm_iir & ~GEN6_PM_DEFERRED_EVENTS)
+	if (pm_iir & ~GEN6_PM_RPS_EVENTS)
 		DRM_ERROR("Unexpected PM interrupted\n");
 }
 
@@ -1019,7 +1020,7 @@  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
 			gmbus_irq_handler(dev);
 
-		if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
+		if (pm_iir & GEN6_PM_RPS_EVENTS)
 			gen6_queue_rps_work(dev_priv, pm_iir);
 
 		I915_WRITE(GTIIR, gt_iir);
@@ -1260,7 +1261,7 @@  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 	if (pm_iir) {
 		if (IS_HASWELL(dev))
 			hsw_pm_irq_handler(dev_priv, pm_iir);
-		else if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
+		else if (pm_iir & GEN6_PM_RPS_EVENTS)
 			gen6_queue_rps_work(dev_priv, pm_iir);
 		I915_WRITE(GEN6_PMIIR, pm_iir);
 		ret = IRQ_HANDLED;
@@ -1375,7 +1376,7 @@  static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (IS_GEN5(dev) &&  de_iir & DE_PCU_EVENT)
 		ironlake_handle_rps_change(dev);
 
-	if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS)
+	if (IS_GEN6(dev) && pm_iir & GEN6_PM_RPS_EVENTS)
 		gen6_queue_rps_work(dev_priv, pm_iir);
 
 	I915_WRITE(GTIIR, gt_iir);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5dae1d9..4485dfa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4518,7 +4518,7 @@ 
 #define  GEN6_PM_RP_DOWN_THRESHOLD		(1<<4)
 #define  GEN6_PM_RP_UP_EI_EXPIRED		(1<<2)
 #define  GEN6_PM_RP_DOWN_EI_EXPIRED		(1<<1)
-#define  GEN6_PM_DEFERRED_EVENTS		(GEN6_PM_RP_UP_THRESHOLD | \
+#define  GEN6_PM_RPS_EVENTS			(GEN6_PM_RP_UP_THRESHOLD | \
 						 GEN6_PM_RP_DOWN_THRESHOLD | \
 						 GEN6_PM_RP_DOWN_TIMEOUT)
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3534a71..d3dd043 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2534,7 +2534,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
@@ -2563,7 +2563,7 @@  static void valleyview_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, GEN6_PM_RPS_EVENTS);
 }
 
 int intel_enable_rc6(const struct drm_device *dev)
@@ -2720,12 +2720,12 @@  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, GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->rps.lock);
-	/* enable all PM interrupts */
+	/* unmask all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
 	rc6vids = 0;
@@ -2937,7 +2937,7 @@  static void valleyview_enable_rps(struct drm_device *dev)
 	valleyview_set_rps(dev_priv->dev, rpe);
 
 	/* requires MSI enabled */
-	I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
+	I915_WRITE(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);