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

Daniele Ceraolo Spurio
2018-02-15T17:59:05Z

<snip>

> +
> +static void
> +gen11_gt_irq_handler(struct drm_i915_private * const i915,
> + const u32 master_ctl)
> +{
> + void __iomem * const regs = i915->regs;
> + unsigned int bank;
> +
> + for (bank = 0; bank < 2; bank++) {
> + unsigned long intr_dw;
> + unsigned int bit;
> +
> + if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
> + continue;
> +
> + intr_dw = readl(regs +
> + i915_mmio_reg_offset(GEN11_GT_INTR_DW(bank)));
> +

I think we should keep some kind of polling here, because there is going
to be some delay between writing the selector and getting the correct
value in the identity register and the CPU might also be running at a
higher frequency than the GPU. Spec does not specify a time but implies
that we have to wait for the valid bit to be set.

> + if (unlikely(!intr_dw))
> + DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> +
> + for_each_set_bit(bit, &intr_dw, 32) {
> + const u16 iir = gen11_gt_engine_intr(i915, bank, bit);
> +
> + if (unlikely(!iir))
> + continue;
> +
> + gen11_gt_engine_irq_handler(i915, bank, bit, iir);

The identity register contains the class (bits 16-18) and instance (bits
20-25), so we can potentially decode them and call directly:

gen11_cs_irq_handler(i915->engine_class[class][instance], iir);

instead of having a big switch. The only problem with that would be that
GuC and PM interrupts come out as class 4 instances 0 and 1, so once we
add support for those we'd have to do something like

if (class != OTHER_CLASS)
gen11_cs_irq_handler(...);
else {
switch (instance) {
case 0:
gen11_guc_irq_handler(...);
break;
case 1:
gen11_rps_irq_handler(...);
break;
default:
MISSING_CASE();
}
}

not sure if decoding class and instance and having a small switch wins
in the end against the big switch, so I won't complain whichever you pick ;)

Daniele

> + }
> +
> + /* Clear must be after shared has been served for engine */
> + writel(intr_dw,
> + regs + i915_mmio_reg_offset(GEN11_GT_INTR_DW(bank)));
> + }
> +}
> +