Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Quoting Mika Kuoppala (2018-02-14 14:12:13)
>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
