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@xxxxxxxxx wrote:
> > From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> >
> > 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.
> > -
> > 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.
> > + } 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?
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
