| Message ID | 1352219142-1395-10-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d5aef6c..5533e55 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -372,10 +372,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); @@ -536,17 +537,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"); } @@ -619,7 +620,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS) blc_event = true; - 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); @@ -759,7 +760,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; @@ -841,7 +842,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); /* should clear PCH hotplug event before clear CPU irq */ diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 21f5d61..70f9ad7 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4269,7 +4269,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 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);
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. v2: Shouldn't destroy PMIIR or PMIMR VEBOX interrupt state in enable/disable rps functions (Haihao) CC: Xiang, Haihao <haihao.xiang@intel.com> 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 | 8 ++++---- 3 files changed, 14 insertions(+), 13 deletions(-)