[Intel-xe] [PATCH 15/26] drm/xe: Interrupts are delivered per-tile, not per-GT

[Lucas De Marchi]

On Wed, May 10, 2023 at 08:47:11PM -0700, Matt Roper wrote:
>IRQ delivery and handling needs to be handled on a per-tile basis.  Note
>that this is true even for the "GT interrupts" relating to engines and
>GuCs --- the interrupts relating to both GTs get raised through a single
>set of registers in the tile's sgunit range.
>
>The (mis)use of struct xe_gt as a target for MMIO operations in the
>driver makes the code somewhat confusing since we wind up needing a GT
>pointer to handle programming that's unrelated to the GT.  To mitigate
>this confusion, all of the xe_gt structures used solely as an MMIO
>target in interrupt code are renamed to 'mmio.'  Reworking the driver's
>MMIO handling to not be dependent on xe_gt is planned as a future
>update.
>
>Note that GT initialization code currently calls xe_gt_irq_postinstall()
>in an attempt to enable the HWE interrupts for the GT being initialized.
>Unfortunately xe_gt_irq_postinstall() doesn't really match its name and
>does a bunch of other stuff unrelated to the GT interrupts (such as
>enabling the top-level device interrupts).  That will be addressed in
>future patches.
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt.c  |   2 +-
> drivers/gpu/drm/xe/xe_irq.c | 334 ++++++++++++++++++++----------------
> drivers/gpu/drm/xe/xe_irq.h |   4 +-
> 3 files changed, 187 insertions(+), 153 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>index 20663cd0ddaf..e00d260dff00 100644
>--- a/drivers/gpu/drm/xe/xe_gt.c
>+++ b/drivers/gpu/drm/xe/xe_gt.c
>@@ -303,7 +303,7 @@ static int gt_fw_domain_init(struct xe_gt *gt)
> 	gt->info.engine_mask = gt->info.__engine_mask;
>
> 	/* Enables per hw engine IRQs */
>-	xe_gt_irq_postinstall(gt);
>+	xe_gt_irq_postinstall(gt_to_tile(gt));
>
> 	/* Rerun MCR init as we now have hw engine list */
> 	xe_gt_mcr_init(gt);
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index 494ec5567e50..fa7d04ba23c0 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -27,60 +27,66 @@
> #define IIR(offset)				XE_REG(offset + 0x8)
> #define IER(offset)				XE_REG(offset + 0xc)
>
>-static void assert_iir_is_zero(struct xe_gt *gt, struct xe_reg reg)
>+static void assert_iir_is_zero(struct xe_gt *mmio, struct xe_reg reg)
> {
>-	u32 val = xe_mmio_read32(gt, reg);
>+	u32 val = xe_mmio_read32(mmio, reg);
>
> 	if (val == 0)
> 		return;
>
>-	drm_WARN(&gt_to_xe(gt)->drm, 1,
>+	drm_WARN(&gt_to_xe(mmio)->drm, 1,
> 		 "Interrupt register 0x%x is not zero: 0x%08x\n",
> 		 reg.addr, val);
>-	xe_mmio_write32(gt, reg, 0xffffffff);
>-	xe_mmio_read32(gt, reg);
>-	xe_mmio_write32(gt, reg, 0xffffffff);
>-	xe_mmio_read32(gt, reg);
>+	xe_mmio_write32(mmio, reg, 0xffffffff);
>+	xe_mmio_read32(mmio, reg);
>+	xe_mmio_write32(mmio, reg, 0xffffffff);
>+	xe_mmio_read32(mmio, reg);
> }
>
> /*
>  * Unmask and enable the specified interrupts.  Does not check current state,
>  * so any bits not specified here will become masked and disabled.
>  */
>-static void unmask_and_enable(struct xe_gt *gt, u32 irqregs, u32 bits)
>+static void unmask_and_enable(struct xe_tile *tile, u32 irqregs, u32 bits)
> {
>+	struct xe_gt *mmio = tile->primary_gt;


AFAICS mmio is always assigned to primary_gt. Why not call it primary_gt
and avoid the confusion and name mismatch with the type?

If I see a function assert_iir_is_zero(struct xe_gt *primary_gt, struct xe_reg reg)
there's a hint that function is only supposed to be called with the
primary_gt. If it can be any type of gt, then simply calling it gt
rather than mmio would be more straightforward.

Lucas De Marchi