[Intel-gfx] [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Apr 16 10:41:33 CEST 2014
On Tue, Apr 15, 2014 at 07:43:07PM -0700, Ben Widawsky wrote:
> On Mon, Apr 14, 2014 at 10:55:53PM +0300, Ville Syrjälä wrote:
> > On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepak.s at intel.com wrote:
> > > From: Ben Widawsky <benjamin.widawsky at intel.com>
> > >
> > > Almost all of it is reusable from the existing code. The primary
> > > difference is we need to do even less in the interrupt handler, since
> > > interrupts are not shared in the same way.
> > >
> > > The patch is mostly a copy-paste of the existing snb+ code, with updates
> > > to the relevant parts requiring changes to the interrupt handling. As
> > > such it /should/ be relatively trivial. It's highly likely that I missed
> > > some places where I need a gen8 version of the PM interrupts, but it has
> > > become invisible to me by now.
> > >
> > > This patch could probably be split into adding the new functions,
> > > followed by actually handling the interrupts. Since the code is
> > > currently disabled (and broken) I think the patch stands better by
> > > itself.
> > >
> > > v2: Move the commit about not touching the ringbuffer interrupt to the
> > > snb_* function where it belongs (Rodrigo)
> > >
> > > v3: Rebased on Paulo's runtime PM changes
> > >
> > > v4: Not well validated, but rebase on
> > > commit 730488b2eddded4497f63f70867b1256cd9e117c
> > > Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Date: Fri Mar 7 20:12:32 2014 -0300
> > >
> > > drm/i915: kill dev_priv->pm.regsave
> > >
> > > v5: Rebased on latest code base. (Deepak)
> > >
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > >
> > > Conflicts:
> > > drivers/gpu/drm/i915/i915_irq.c
> >
> > IIRC Daniel doesn't like these conflict markers. So should be dropped.
> >
>
> I like the conflict markers generally. Daniel can kill it if he likes,
> but thanks for the input. I've killed it this time around, but I don't
> plan on it for the future.
>
> > > ---
> > > drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++---
> > > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++-
> > > 4 files changed, 115 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 7a4d3ae..96c459a 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> > > return true;
> > > }
> > >
> > > +/**
> > > + * bdw_update_pm_irq - update GT interrupt 2
> > > + * @dev_priv: driver private
> > > + * @interrupt_mask: mask of interrupt bits to update
> > > + * @enabled_irq_mask: mask of interrupt bits to enable
> > > + *
> > > + * Copied from the snb function, updated with relevant register offsets
> > > + */
> > > +static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
> > > + uint32_t interrupt_mask,
> > > + uint32_t enabled_irq_mask)
> > > +{
> > > + uint32_t new_val;
> > > +
> > > + assert_spin_locked(&dev_priv->irq_lock);
> > > +
> > > + if (dev_priv->pm.irqs_disabled) {
> > > + WARN(1, "IRQs disabled\n");
> > > + return;
> > > + }
> > > +
> > > + new_val = dev_priv->pm_irq_mask;
> > > + new_val &= ~interrupt_mask;
> > > + new_val |= (~enabled_irq_mask & interrupt_mask);
> > > +
> > > + if (new_val != dev_priv->pm_irq_mask) {
> > > + dev_priv->pm_irq_mask = new_val;
> > > + I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) |
> > > + dev_priv->pm_irq_mask);
> > > + POSTING_READ(GEN8_GT_IMR(2));
> > > + }
> > > +}
> > > +
> > > +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > > +{
> > > + bdw_update_pm_irq(dev_priv, mask, mask);
> > > +}
> > > +
> > > +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > > +{
> > > + bdw_update_pm_irq(dev_priv, mask, 0);
> > > +}
> > > +
> > > +
> >
> > Unnecessary empty line.
> >
>
> Got it, thanks.
>
> > > static bool cpt_can_enable_serr_int(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
> > > spin_lock_irq(&dev_priv->irq_lock);
> > > pm_iir = dev_priv->rps.pm_iir;
> > > dev_priv->rps.pm_iir = 0;
> > > - /* Make sure not to corrupt PMIMR state used by ringbuffer code */
> > > - snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + if (IS_BROADWELL(dev_priv->dev))
> > > + bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + else {
> > > + /* Make sure not to corrupt PMIMR state used by ringbuffer */
> > > + snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + /* Make sure we didn't queue anything we're not going to
> > > + * process. */
> > > + WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
> > > + }
> > > spin_unlock_irq(&dev_priv->irq_lock);
> > >
> > > - /* Make sure we didn't queue anything we're not going to process. */
> > > - WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
> >
> > Isn't this WARN equally valid for bdw?
> >
>
> So first, let me just mention, this has moved slightly in my latest
> version of this patch, but it's still a valid question.
>
> The answer is ambiguous actually. The WARN is always valid technically
> The distinction in BDW (at least for the time being) is that unlike
> pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right
> now, DRM_ERROR (or BUG) is more appropriate.
I don't see a reason for bdw to be different here. There are other bits
in the relevant IIR registers, so the same WARN should be fine.
>
>
> > > -
> > > if ((pm_iir & dev_priv->pm_rps_events) == 0)
> > > return;
> > >
> > > @@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,
> > > ivybridge_parity_error_irq_handler(dev, gt_iir);
> > > }
> > >
> > > +static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > > +{
> > > + if ((pm_iir & dev_priv->pm_rps_events) == 0)
> > > + return;
> > > +
> > > + spin_lock(&dev_priv->irq_lock);
> > > + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
> > > + bdw_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
> > > + spin_unlock(&dev_priv->irq_lock);
> > > +
> > > + queue_work(dev_priv->wq, &dev_priv->rps.work);
> > > +}
> > > +
> > > static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
> > > struct drm_i915_private *dev_priv,
> > > u32 master_ctl)
> > > @@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
> > > DRM_ERROR("The master control interrupt lied (GT1)!\n");
> > > }
> > >
> > > + if (master_ctl & GEN8_GT_PM_IRQ) {
> > > + tmp = I915_READ(GEN8_GT_IIR(2));
> > > + if (tmp & dev_priv->pm_rps_events) {
> > > + ret = IRQ_HANDLED;
> > > + gen8_rps_irq_handler(dev_priv, tmp);
> > > + I915_WRITE(GEN8_GT_IIR(1), tmp & dev_priv->pm_rps_events);
> > ^^^^^^^^^^^^^^
> >
> > GEN8_GT_IIR(2)
> >
>
> Nice catch, I guess I need to retest after this one.
I'm actually wondering how/if you managed to test this before Deepak's
PMINTRMSK interrupt redirect bit fix. W/o that you shouldn't be
getting any interrupt AFAICS.
>
> > > + } else
> > > + DRM_ERROR("The master control interrupt lied (PM)!\n");
> > > + }
> > > +
> > > if (master_ctl & GEN8_GT_VECS_IRQ) {
> > > tmp = I915_READ(GEN8_GT_IIR(3));
> > > if (tmp) {
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 8f84555..c2dd436 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4193,6 +4193,7 @@ enum punit_power_well {
> > > #define GEN8_DE_PIPE_A_IRQ (1<<16)
> > > #define GEN8_DE_PIPE_IRQ(pipe) (1<<(16+pipe))
> > > #define GEN8_GT_VECS_IRQ (1<<6)
> > > +#define GEN8_GT_PM_IRQ (1<<4)
> > > #define GEN8_GT_VCS2_IRQ (1<<3)
> > > #define GEN8_GT_VCS1_IRQ (1<<2)
> > > #define GEN8_GT_BCS_IRQ (1<<1)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index c551472..68a078c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > > void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
> > > void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
> > >
> > > -
> >
> > Unrelated whitespace change. IIRC someone left these two empty lines
> > here on purpose.
> >
>
> Got it, thanks.
>
> > > /* intel_crt.c */
> > > void intel_crt_init(struct drm_device *dev);
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 75c1c76..27b64ab 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> > > trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv, val));
> > > }
> > >
> > > +static void gen8_disable_rps_interrupts(struct drm_device *dev)
> > > +{
> > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > + I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > > + I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
> > > + ~dev_priv->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
> > > + * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> >
> > This comment would need a bit of update for gen8.
> >
>
> I've killed this comment locally. Somewhere I had written that we can
> rewrite some of this for gen8, but I seem to have lost that in the sands
> of time.
>
> > > +
> > > + spin_lock_irq(&dev_priv->irq_lock);
> > > + dev_priv->rps.pm_iir = 0;
> > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> > > + I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> > > +}
> > > +
> > > static void gen6_disable_rps_interrupts(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)
> > > I915_WRITE(GEN6_RC_CONTROL, 0);
> > > I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> > >
> > > - gen6_disable_rps_interrupts(dev);
> > > + if (IS_BROADWELL(dev))
> > > + gen8_disable_rps_interrupts(dev);
> > > + else
> > > + gen6_disable_rps_interrupts(dev);
> > > }
> > >
> > > static void valleyview_disable_rps(struct drm_device *dev)
> > > @@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)
> > > return INTEL_RC6_ENABLE;
> > > }
> > >
> > > +static void gen8_enable_rps_interrupts(struct drm_device *dev)
> > > +{
> > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > + spin_lock_irq(&dev_priv->irq_lock);
> > > + WARN_ON(dev_priv->rps.pm_iir);
> > > + bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > > + I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> >
> > The order of "unmask first then clear" seems just wrong. The same issues is
> > already present in the gen6 code however. So this should be fixed in
> > both places, or if it's like that on purpose it needs a comment.
> >
>
> Well, if you want to fix it, we can do it in a separate patch. Although
> I don't think I agree. Why does it seem wrong to you?
If you enable before clearing and there's already an IIR bit high the
interrupt should trigger immediately before you get to clear it. And
then on a SMP machine the write to clear it would even race with the
interrupt handler if there's no locking. So the clearing after
enabling+unmasking is pointless. The normal approach for enabling
interrupts is to clear first and then enable.
Hmm. Actually here we alreay have the interrupts enabled in IER, so
if any relevant IIR bit would be high, we should have already gotten
the interrupt. But since we always clear the IIR after masking the
interrupts in IMR, we should never have any relevant IIR bit set here.
So looks to me like we can just drop the IIR writes here. They serve no
purpose other than racing with the interrupt handler if a real interrupt
happens just after we've unmasked it.
>
> > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> >
> > Useless empty line.
> >
>
> If I was in an arguing mood, I would do, but fine.
>
> > > +}
> > > +
> > > static void gen6_enable_rps_interrupts(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)
> > >
> > > gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
> > >
> > > - gen6_enable_rps_interrupts(dev);
> > > + gen8_enable_rps_interrupts(dev);
> > >
> > > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > > }
>
> Thanks for the review.
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list