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

Daniele Ceraolo Spurio
2018-02-13T19:18:33Z

On 13/02/18 08:37, Mika Kuoppala wrote:
> 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));

Patch 18/20 changes this to time_after64 and wait_end to u64, which
looks like the correct way to handle this. Since splitting part of this
loop out also helps making things cleaner, would it make sense to squash
patch 18/20 into this one?

> +
> + if (!(ident & GEN11_INTR_DATA_VALID))
> + DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n",
> + bank, bit);

If the data is not valid we should probably skip writing it to the
irq[bank][bit] field as we might be writing an incorrect value.

> +
> + irq[bank][bit] = ident & GEN11_INTR_ENGINE_MASK;
> + if (!irq[bank][bit])
> + DRM_ERROR("INTR_IDENTITY_REG%u:%u blank!\n",
> + bank, bit);

As mentioned on the previous review, I believe it is actually possible
to get a blank identity register due to double buffering of interrupts
if a second interrupt arrives between reading GT_INTR_DW and reading
GEN11_INTR_IDENTITY, as both interrupts will be serviced on the first
iteration but GT_INTR_DW will still report the second interrupt after
being cleared once. We can just drop the error here as nothing bas
should happen afterwards

> + 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 (irq[0][GEN11_BCS]) {
> + gen11_cs_irq_handler(dev_priv->engine[BCS],
> + irq[0][GEN11_BCS]);
> + ret = IRQ_HANDLED;
> + }
> +
> + for (engine = 0; engine < 4; engine++) {

Can we use I915_MAX_VCS here?

> + if (irq[1][GEN11_VCS(engine)]) {
> + gen11_cs_irq_handler(dev_priv->engine[_VCS(engine)],
> + irq[1][GEN11_VCS(engine)]);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + for (engine = 0; engine < 2; engine++) {

same as above but with I915_MAX_VECS

Thanks,
Daniele

> + if (irq[1][GEN11_VECS(engine)]) {
> + gen11_cs_irq_handler(dev_priv->engine[_VECS(engine)],
> + irq[1][GEN11_VECS(engine)]);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + if (irq[0][GEN11_GTPM] & dev_priv->pm_rps_events) {
> + ret = IRQ_HANDLED;
> + gen6_rps_irq_handler(dev_priv, tmp);
> + }
> +
> + return ret;
> +}
> +
> +static irqreturn_t gen11_irq_handler(int irq, void *arg)
> +{
> + struct drm_device *dev = arg;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 master_ctl;
> + u32 disp_ctl;
> + irqreturn_t ret;
> +
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> + master_ctl = I915_READ_FW(GEN11_GFX_MSTR_IRQ);
> +
> + master_ctl &= ~GEN11_MASTER_IRQ;
> + if (!master_ctl)
> + return IRQ_NONE;
> +
> + /* Disable interrupts. */
> + I915_WRITE_FW(GEN11_GFX_MSTR_IRQ, 0);
> +
> + /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> + disable_rpm_wakeref_asserts(dev_priv);
> +
> + /* Find, clear, then process each source of interrupt. */
> + ret = gen11_gt_irq_handler(dev_priv, master_ctl);
> +
> + if (master_ctl & GEN11_DISPLAY_IRQ) {
> + disp_ctl = I915_READ_FW(GEN11_DISPLAY_INT_CTL);
> + ret |= gen8_de_irq_handler(dev_priv, disp_ctl);
> + }
> +
> + /* Acknowledge and enable interrupts. */
> + I915_WRITE_FW(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ | master_ctl);
> + POSTING_READ_FW(GEN11_GFX_MSTR_IRQ);
> +
> + enable_rpm_wakeref_asserts(dev_priv);
> +
> + return ret;
> +}
> +
> /**
> * i915_reset_device - do process context error handling work
> * @dev_priv: i915 device private
> @@ -3159,6 +3290,42 @@ static void gen8_irq_reset(struct drm_device *dev)
> ibx_irq_reset(dev_priv);
> }
>
> +static void gen11_gt_irq_reset(struct drm_i915_private *dev_priv)
> +{
> + /* Disable RCS, BCS, VCS and VECS class engines. */
> + I915_WRITE(GEN11_RENDER_COPY_INTR_ENABLE, 0);
> + I915_WRITE(GEN11_VCS_VECS_INTR_ENABLE, 0);
> +
> + /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */
> + I915_WRITE(GEN11_RCS0_RSVD_INTR_MASK, ~0);
> + I915_WRITE(GEN11_BCS_RSVD_INTR_MASK, ~0);
> + I915_WRITE(GEN11_VCS0_VCS1_INTR_MASK, ~0);
> + I915_WRITE(GEN11_VCS2_VCS3_INTR_MASK, ~0);
> + I915_WRITE(GEN11_VECS0_VECS1_INTR_MASK, ~0);
> +}
> +
> +static void gen11_irq_reset(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int pipe;
> +
> + I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> + POSTING_READ(GEN11_GFX_MSTR_IRQ);
> +
> + gen11_gt_irq_reset(dev_priv);
> +
> + I915_WRITE(GEN11_DISPLAY_INT_CTL, 0);
> +
> + for_each_pipe(dev_priv, pipe)
> + if (intel_display_power_is_enabled(dev_priv,
> + POWER_DOMAIN_PIPE(pipe)))
> + GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> +
> + GEN3_IRQ_RESET(GEN8_DE_PORT_);
> + GEN3_IRQ_RESET(GEN8_DE_MISC_);
> + GEN3_IRQ_RESET(GEN8_PCU_);
> +}
> +
> void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> u8 pipe_mask)
> {
> @@ -3656,6 +3823,41 @@ static int gen8_irq_postinstall(struct drm_device *dev)
> return 0;
> }
>
> +static void gen11_gt_irq_postinstall(struct drm_i915_private *dev_priv)
> +{
> + const u32 irqs = GT_RENDER_USER_INTERRUPT | GT_CONTEXT_SWITCH_INTERRUPT;
> +
> + BUILD_BUG_ON(irqs & 0xffff0000);
> +
> + /* Enable RCS, BCS, VCS and VECS class interrupts. */
> + I915_WRITE(GEN11_RENDER_COPY_INTR_ENABLE, irqs << 16 | irqs);
> + I915_WRITE(GEN11_VCS_VECS_INTR_ENABLE, irqs << 16 | irqs);
> +
> + /* Unmask irqs on RCS, BCS, VCS and VECS engines. */
> + I915_WRITE(GEN11_RCS0_RSVD_INTR_MASK, ~(irqs << 16));
> + I915_WRITE(GEN11_BCS_RSVD_INTR_MASK, ~(irqs << 16));
> + I915_WRITE(GEN11_VCS0_VCS1_INTR_MASK, ~(irqs | irqs << 16));
> + I915_WRITE(GEN11_VCS2_VCS3_INTR_MASK, ~(irqs | irqs << 16));
> + I915_WRITE(GEN11_VECS0_VECS1_INTR_MASK, ~(irqs | irqs << 16));
> +
> + dev_priv->pm_imr = 0xffffffff; /* TODO */
> +}
> +
> +static int gen11_irq_postinstall(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + gen11_gt_irq_postinstall(dev_priv);
> + gen8_de_irq_postinstall(dev_priv);
> +
> + I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE);
> +
> + I915_WRITE(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
> + POSTING_READ(GEN11_GFX_MSTR_IRQ);
> +
> + return 0;
> +}
> +
> static int cherryview_irq_postinstall(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -4104,6 +4306,14 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> dev->driver->enable_vblank = i965_enable_vblank;
> dev->driver->disable_vblank = i965_disable_vblank;
> dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> + } else if (INTEL_GEN(dev_priv) >= 11) {
> + dev->driver->irq_handler = gen11_irq_handler;
> + dev->driver->irq_preinstall = gen11_irq_reset;
> + dev->driver->irq_postinstall = gen11_irq_postinstall;
> + dev->driver->irq_uninstall = gen11_irq_reset;
> + dev->driver->enable_vblank = gen8_enable_vblank;
> + dev->driver->disable_vblank = gen8_disable_vblank;
> + dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> } else if (INTEL_GEN(dev_priv) >= 8) {
> dev->driver->irq_handler = gen8_irq_handler;
> dev->driver->irq_preinstall = gen8_irq_reset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d88f107b0d50..087f981461e7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8027,7 +8027,10 @@ void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
> dev_priv->gt_pm.rc6.enabled = true; /* force RC6 disabling */
> intel_disable_gt_powersave(dev_priv);
>
> - gen6_reset_rps_interrupts(dev_priv);
> + if (INTEL_GEN(dev_priv) < 11)
> + gen6_reset_rps_interrupts(dev_priv);
> + else
> + WARN_ON_ONCE(1);
> }
>
> static inline void intel_disable_llc_pstate(struct drm_i915_private *i915)
> @@ -8140,6 +8143,8 @@ static void intel_enable_rps(struct drm_i915_private *dev_priv)
> cherryview_enable_rps(dev_priv);
> } else if (IS_VALLEYVIEW(dev_priv)) {
> valleyview_enable_rps(dev_priv);
> + } else if (WARN_ON_ONCE(INTEL_GEN(dev_priv) >= 11)) {
> + /* TODO */
> } else if (INTEL_GEN(dev_priv) >= 9) {
> gen9_enable_rps(dev_priv);
> } else if (IS_BROADWELL(dev_priv)) {
>