Re: [PATCH 07/27] drm/i915/icl: Interrupt handling

Daniele Ceraolo Spurio
2018-02-09T22:34:41Z

<snip>

> +
> +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);

I think having the identity reg valid but empty is a possible case and
we shouldn't error on it. The scenario I'm thinking of is:

- Interrupt on engine, we read master IRQ and disable interrupts
- we read GEN11_GT_INTR_DW. the register is now locked until a write occurs
- a second interrupt arrives on the same engine, gets double buffered
behind the current one in GEN11_GT_INTR_DW
- we read the irq vector (which will now contain both interrupts),
process it and clear it
- we write to GEN11_GT_INTR_DW to clear it and the buffered value is now
propagated to it
- GEN11_GT_INTR_DW now has the bit set, but we already serviced the
interrupt so the identity register will be zero.

Just dropping the error message should be enough since we have correctly
serviced everything.

Daniele