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

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 05/19] drm/i915/icl: Interrupt handling
Date: Thu, 15 Feb 2018 18:24:34 +0200	[thread overview]
Message-ID: <87wozete4d.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <151861835826.31524.8463924294963797062@mail.alporthouse.com>

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-02-14 14:12:13)
>> 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.
>> 
>> v9:
>>   * squashed in selector and shared register handling (Daniele)
>>   * skip writing of irq if data is not valid (Daniele)
>>   * use time_after32 (Chris)
>>   * use I915_MAX_VCS and I915_MAX_VECS (Daniele)
>>   * remove fake pm interrupt handling for later patch (Mika)
>> 
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> 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>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 212 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_pm.c |   7 +-
>>  2 files changed, 218 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index b886bd459acc..9a2d12c8c44c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -408,6 +408,37 @@ void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>>         spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>>  
>> +static int gen11_service_shared_iir(struct drm_i915_private *dev_priv,
>> +                                   const unsigned int bank,
>> +                                   const unsigned int bit)
>> +{
>> +       u64 wait_end;
>> +       u32 ident;
>> +       int irq;
>> +
>> +       I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
>> +       /*
>> +        * NB: Specs do not specify how long to spin wait.
>> +        * Taking 100us as an educated guess
>> +        */
>> +       wait_end = (local_clock() >> 10) + 100;
>> +       do {
>> +               ident = I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
>> +       } while (!(ident & GEN11_INTR_DATA_VALID) &&
>> +                !time_after32(local_clock() >> 10, wait_end));
>
> Now you are just mixing types willy nilly :)
>
> No need for wait_end to be 64b when we are looking at a 100 interval.

I threw the poll away. We can add it later if someone objects
and/or evidence indicates that it is needed. But polling for 100us just
in case in irq handler should not be the first step.

>
>> +
>> +       if (!(ident & GEN11_INTR_DATA_VALID)) {
>> +               DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n", bank, bit);
>> +               return -ETIMEDOUT;
>> +       }
>> +
>> +       irq = ident & GEN11_INTR_ENGINE_MASK;
>> +
>> + I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident);

Bspec tells that valid bit write should be enough here.

>> +
>> +       return irq;
>
> return ident & GEN11_INTR_ENGINE_MASK;
>
> no need for irq, and why int return type?

I made it return zero on unvalid.

>
> Why is this gen11 specific helper so far away from the irq_handler?

Due to later patch in this series needing it, rc6 enabling
due to rps reset.

I moved those closer in both patches.

>
>> +static __always_inline void
>> +gen11_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>> +{
>> +       gen8_cs_irq_handler(engine, iir, 0);
>> +}
>> +
>> +static void
>> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, const u32 master_ctl)
>> +{
>> +       u16 irq[2][32];
>> +       unsigned int bank, engine;
>> +
>> +       memset(irq, 0, sizeof(irq));
>> +
>> +       for (bank = 0; bank < 2; bank++) {
>> +               unsigned long tmp;
>> +               unsigned int bit;
>> +               u32 dw;
>> +               int ret;
>> +
>> +               if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
>> +                       continue;
>> +
>> +               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) {
>
> tmp is not required here.

reading directly now to unsigned long dw_intr.

>
>> +                       ret = gen11_service_shared_iir(dev_priv, bank, bit);
>> +                       if (unlikely(ret < 0))
>> +                               continue;
>> +
>> +                       irq[bank][bit] = ret;
>> +               }
>> +
>> +               I915_WRITE_FW(GEN11_GT_INTR_DW(bank), dw);
>
> If we process the banks here, we won't need to memset 128 bytes and scan
> untouched cachelines!

Made it so. Bspec says that we need to serve the engine shared irq
before clearing the corresponding bank bit. I avoided trickery
and kept it so that we minimize writes by clearing them all.

>
>> +       }
>> +
>> +       if (irq[0][GEN11_RCS0])
>> +               gen11_cs_irq_handler(dev_priv->engine[RCS],
>> +                                    irq[0][GEN11_RCS0]);
>> +
>> +       if (irq[0][GEN11_BCS])
>> +               gen11_cs_irq_handler(dev_priv->engine[BCS],
>> +                                    irq[0][GEN11_BCS]);
>> +
>> +       for (engine = 0; engine < I915_MAX_VCS; engine++)
>> +               if (irq[1][GEN11_VCS(engine)])
>> +                       gen11_cs_irq_handler(dev_priv->engine[_VCS(engine)],
>> +                                            irq[1][GEN11_VCS(engine)]);
>> +
>> +       for (engine = 0; engine < I915_MAX_VECS; engine++)
>> +               if (irq[1][GEN11_VECS(engine)])
>> +                       gen11_cs_irq_handler(dev_priv->engine[_VECS(engine)],
>> +                                            irq[1][GEN11_VECS(engine)]);
>
> Keep reminding yourself that this is the hottest function in the entire
> i915.ko.

The serving of engine specific irq is now done using outer
and inner switches. Atleast it looks leaner :O

>
>> +}
>> +
>> +static irqreturn_t gen11_irq_handler(int irq, void *arg)
>> +{
>> +       struct drm_device *dev = arg;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>
> What?

	struct drm_i915_private * const dev_priv =
		to_i915((struct drm_device *)arg);
>
>> +       u32 master_ctl;
>> +       u32 disp_ctl;
>
> Why is this at top level scope?

It is no more at all.

>
>> +       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. */
>> +       gen11_gt_irq_handler(dev_priv, master_ctl);
>> +
>> +       if (master_ctl & GEN11_DISPLAY_IRQ) {
>> +               disp_ctl = I915_READ_FW(GEN11_DISPLAY_INT_CTL);
>> +               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);
>
> What happened to the onion? gen8 is broken as well, sure I sent patches
> to fix that. The posting read is ott, and you don't need to disable the
> asserts around the GT irq handler.

I moved the asserts inside display scope, like in gen8 version.

Thanks for comments,
-Mika

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx