Re: [PATCH 8/9] drm/i915/bdw: Implement a basic PM interrupt handler

["S, Deepak" <deepak.s@intel.com>]

On Wed, Jan 29, 2014 at 9:55 AM, Ben Widawsky
> <benjamin.widawsky@intel.com <mailto:benjamin.widawsky@intel.com>> wrote:
>
>     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.
>
>     Signed-off-by: Ben Widawsky <ben@bwidawsk.net <mailto:ben@bwidawsk.net>>
>     ---
>       drivers/gpu/drm/i915/i915_irq.c  | 80
>     ++++++++++++++++++++++++++++++++++++++--
>       drivers/gpu/drm/i915/i915_reg.h  |  1 +
>       drivers/gpu/drm/i915/intel_drv.h |  2 +
>       drivers/gpu/drm/i915/intel_pm.c  | 39 +++++++++++++++++++-
>       4 files changed, 117 insertions(+), 5 deletions(-)
>
>     diff --git a/drivers/gpu/drm/i915/i915_irq.c
>     b/drivers/gpu/drm/i915/i915_irq.c
>     index 72ade87..ab0c7ac 100644
>     --- a/drivers/gpu/drm/i915/i915_irq.c
>     +++ b/drivers/gpu/drm/i915/i915_irq.c
>     @@ -214,6 +214,53 @@ 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->pc8.irqs_disabled) {
>     +               WARN(1, "IRQs disabled\n");
>     +               dev_priv->pc8.regsave.gen6_pmimr &= ~interrupt_mask;
>     +               dev_priv->pc8.regsave.gen6_pmimr |= (~enabled_irq_mask &
>     +                                                    interrupt_mask);
>     +               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);
>     +}
>     +
>     +
>       static bool cpt_can_enable_serr_int(struct drm_device *dev)
>       {
>              struct drm_i915_private *dev_priv = dev->dev_private;
>     @@ -997,11 +1044,15 @@ static void gen6_pm_rps_work(struct
>     work_struct *work)
>              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, GEN6_PM_RPS_EVENTS);
>     +       if (IS_BROADWELL(dev_priv->dev))
>     +               bdw_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>     +       else {
>     +               /* Make sure we didn't queue anything we're not
>     going to process. */
>     +               snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>     +               WARN_ON(pm_iir & ~GEN6_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 & ~GEN6_PM_RPS_EVENTS);
>
>              if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
>                      return;
>     @@ -1192,6 +1243,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 & GEN6_PM_RPS_EVENTS) == 0)
>     +               return;
>     +
>     +       spin_lock(&dev_priv->irq_lock);
>     +       dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
>     +       bdw_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
>     +       spin_unlock(&dev_priv->irq_lock);
>     +
>     +       queue_work(dev_priv->wq, &dev_priv->rps.work);
>     +}

We can use the "gen6_rps_irq_handler" right?

>       static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>                                             struct drm_i915_private
>     *dev_priv,
>                                             u32 master_ctl)
>     @@ -1227,6 +1291,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 & GEN6_PM_RPS_EVENTS) {
>     +                       ret = IRQ_HANDLED;
>     +                       gen8_rps_irq_handler(dev_priv, tmp);
>     +                       I915_WRITE(GEN8_GT_IIR(1), tmp &
>     GEN6_PM_RPS_EVENTS);
>     +               } 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 d77720e..f9e582e 100644
>     --- a/drivers/gpu/drm/i915/i915_reg.h
>     +++ b/drivers/gpu/drm/i915/i915_reg.h
>     @@ -4024,6 +4024,7 @@
>       #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 44067bc..7b49779 100644
>     --- a/drivers/gpu/drm/i915/intel_drv.h
>     +++ b/drivers/gpu/drm/i915/intel_drv.h
>     @@ -608,6 +608,8 @@ 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 hsw_pc8_disable_interrupts(struct drm_device *dev);
>       void hsw_pc8_restore_interrupts(struct drm_device *dev);
>
>     diff --git a/drivers/gpu/drm/i915/intel_pm.c
>     b/drivers/gpu/drm/i915/intel_pm.c
>     index deaaaf2..a5c9142 100644
>     --- a/drivers/gpu/drm/i915/intel_pm.c
>     +++ b/drivers/gpu/drm/i915/intel_pm.c
>     @@ -3091,6 +3091,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)) &
>     +                                  ~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
>     +        * stale bits in PMIIR and PMIMR which gen6_enable_rps will
>     clean up. */
>     +
>     +       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), GEN6_PM_RPS_EVENTS);
>     +}
>     +
>       static void gen6_disable_rps_interrupts(struct drm_device *dev)
>       {
>              struct drm_i915_private *dev_priv = dev->dev_private;
>     @@ -3116,7 +3135,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)
>     @@ -3161,6 +3183,19 @@ 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, GEN6_PM_RPS_EVENTS);
>     +       I915_WRITE(GEN8_GT_IIR(2), GEN6_PM_RPS_EVENTS);
>     +       spin_unlock_irq(&dev_priv->irq_lock);
>     +
>     +       I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS);
>     +}

As per the spec, I think we need to unmask bit 31 also, if not i don't 
this we will get PM inetrrupts.

>       static void gen6_enable_rps_interrupts(struct drm_device *dev)
>       {
>              struct drm_i915_private *dev_priv = dev->dev_private;
>     @@ -3263,7 +3298,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);
>       }
>     --
>     1.8.5.3
>
>     _______________________________________________
>     Intel-gfx mailing list
>     Intel-gfx@lists.freedesktop.org <mailto:Intel-gfx@lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>