Re: [PATCH 05/20] drm/i915/icl: Interrupt handling

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 05/20] drm/i915/icl: Interrupt handling
Date: Tue, 13 Feb 2018 17:06:16 +0000	[thread overview]
Message-ID: <151854157661.25823.4437847787620721248@mail.alporthouse.com> (raw)
In-Reply-To: <20180213163738.9055-6-mika.kuoppala@linux.intel.com>

Quoting Mika Kuoppala (2018-02-13 16:37:23)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> v2: Rebase.
> 
> v3:
>   * Remove DPF, it has been removed from SKL+.
>   * Fix -internal rebase wrt. execlists interrupt handling.
> 
> v4: Rebase.
> 
> v5:
>   * Updated for POR changes. (Daniele Ceraolo Spurio)
>   * Merged with irq handling fixes by Daniele Ceraolo Spurio:
>       * Simplify the code by using gen8_cs_irq_handler.
>       * Fix interrupt handling for the upstream kernel.
> 
> v6:
>   * Remove early bringup debug messages (Tvrtko)
>   * Add NB about arbitrary spin wait timeout (Tvrtko)
> 
> v7 (from Paulo):
>   * Don't try to write RO bits to registers.
>   * Don't check for PCH types that don't exist. PCH interrupts are not
>     here yet.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 210 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c |   7 +-
>  2 files changed, 216 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b886bd459acc..3a1de4e2a941 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -415,6 +415,9 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
>         if (READ_ONCE(rps->interrupts_enabled))
>                 return;
>  
> +       if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
> +               return;
> +
>         spin_lock_irq(&dev_priv->irq_lock);
>         WARN_ON_ONCE(rps->pm_iir);
>         WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
> @@ -431,6 +434,9 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>         if (!READ_ONCE(rps->interrupts_enabled))
>                 return;
>  
> +       if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
> +               return;
> +
>         spin_lock_irq(&dev_priv->irq_lock);
>         rps->interrupts_enabled = false;
>  
> @@ -2746,6 +2752,131 @@ static void __fini_wedge(struct wedge_me *w)
>              (W)->i915;                                                 \
>              __fini_wedge((W)))
>  
> +static __always_inline void
> +gen11_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> +{
> +       gen8_cs_irq_handler(engine, iir, 0);
> +}
> +
> +static irqreturn_t
> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> +{
> +       irqreturn_t ret = IRQ_NONE;
> +       u16 irq[2][32];
> +       u32 dw, ident;
> +       unsigned long tmp;
> +       unsigned int bank, bit, engine;
> +       unsigned long wait_start, wait_end;
> +
> +       memset(irq, 0, sizeof(irq));
> +
> +       for (bank = 0; bank < 2; bank++) {
> +               if (master_ctl & GEN11_GT_DW_IRQ(bank)) {
> +                       dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
> +                       if (!dw)
> +                               DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> +                       tmp = dw;
> +                       for_each_set_bit(bit, &tmp, 32) {
> +                               I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit);
> +                               wait_start = local_clock() >> 10;
> +                               /* NB: Specs do not specify how long to spin wait.
> +                                * Taking 100us as an educated guess */
> +                               wait_end = wait_start + 100;
> +                               do {
> +                                       ident = I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
> +                               } while (!(ident & GEN11_INTR_DATA_VALID) &&
> +                                        !time_after((unsigned long)local_clock() >> 10, wait_end));
> +
> +                               if (!(ident & GEN11_INTR_DATA_VALID))
> +                                       DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n",
> +                                                 bank, bit);
> +
> +                               irq[bank][bit] = ident & GEN11_INTR_ENGINE_MASK;
> +                               if (!irq[bank][bit])
> +                                       DRM_ERROR("INTR_IDENTITY_REG%u:%u blank!\n",
> +                                                 bank, bit);
> +                               I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident);
> +                       }
> +                       I915_WRITE_FW(GEN11_GT_INTR_DW(bank), dw);
> +               }
> +       }
> +
> +       if (irq[0][GEN11_RCS0]) {
> +               gen11_cs_irq_handler(dev_priv->engine[RCS],
> +                                    irq[0][GEN11_RCS0]);
> +               ret = IRQ_HANDLED;

If you implement the same reset policy as we have for earlier gen, you
need to set IRQ_HANDLED based solely on the master. In short, just make
this function return void.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx