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

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 05/19] drm/i915/icl: Interrupt handling
Date: Thu, 15 Feb 2018 09:59:04 -0800	[thread overview]
Message-ID: <47454ea5-6b99-0540-a96f-882b53e8ac3b@intel.com> (raw)
In-Reply-To: <20180215162729.16034-1-mika.kuoppala@linux.intel.com>

<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)));
> +	}
> +}
> +
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx