[Intel-xe] [PATCH 2/8] drm/xe/irq: Add helpers to find ISR/IIR/IMR/IER registers

[Lucas De Marchi]

On Thu, Mar 30, 2023 at 11:23:59AM -0700, Matt Roper wrote:
>For cases where IRQ_INIT and IRQ_RESET are used, the relevant interrupt
>registers are always consecutive and ordered ISR, IMR, IIR, IER.  Adding
>helpers to look these up from a base offset will let us eliminate some
>of the CPP pasting and simplify other upcoming patches.
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/regs/xe_reg_defs.h |  8 ++++++++
> drivers/gpu/drm/xe/regs/xe_regs.h     | 11 ++---------
> drivers/gpu/drm/xe/xe_irq.c           | 24 ++++++++++++------------
> 3 files changed, 22 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>index b5c25e31b889..7ff3aa9322af 100644
>--- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>+++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>@@ -8,4 +8,12 @@
>
> #include "compat-i915-headers/i915_reg_defs.h"
>
>+/*
>+ * Interrupt registers for a unit are always consecutive and ordered
>+ * ISR, IMR, IIR, IER.
>+ */
>+#define IMR(offset)				_MMIO(offset + 0x4)
>+#define IIR(offset)				_MMIO(offset + 0x8)
>+#define IER(offset)				_MMIO(offset + 0xc)

since this is only to ever be used while handling the irqs in xe_irq.c,
wouldn't it be better o make it local to them local to that file?

My worry is that xe_reg_defs.h is the one file included by the world,
directly or indirectly and these IMR/IIR/IER short macros, although nice
would be poluting the "namespace".


>+
> #endif
>diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h b/drivers/gpu/drm/xe/regs/xe_regs.h
>index c1c829c23df1..ffe5d726e196 100644
>--- a/drivers/gpu/drm/xe/regs/xe_regs.h
>+++ b/drivers/gpu/drm/xe/regs/xe_regs.h
>@@ -76,15 +76,8 @@
>
> #define SOFTWARE_FLAGS_SPR33			_MMIO(0x4f084)
>
>-#define GEN8_PCU_ISR				_MMIO(0x444e0)
>-#define GEN8_PCU_IMR				_MMIO(0x444e4)
>-#define GEN8_PCU_IIR				_MMIO(0x444e8)
>-#define GEN8_PCU_IER				_MMIO(0x444ec)
>-
>-#define GEN11_GU_MISC_ISR			_MMIO(0x444f0)
>-#define GEN11_GU_MISC_IMR			_MMIO(0x444f4)
>-#define GEN11_GU_MISC_IIR			_MMIO(0x444f8)
>-#define GEN11_GU_MISC_IER			_MMIO(0x444fc)
>+#define PCU_IRQ_REGS				0x444e0
>+#define GU_MISC_IRQ_REGS			0x444f0

we now have:

	*_BASE	
	*_OFFSET
	*_REGS

to refer to the plain u32 value. Should we consolidate in just one?
Maybe _OFFSET  would be one that could be applied both when it's a
"base", in the sense of HW unit, and when it's "the start of similar
registers".

otherwise the simplication and avoiding the macro pasting looks pretty
good and a little bit more grep-able.

thanks
Lucas De Marchi

> #define   GEN11_GU_MISC_GSE			(1 << 27)
>
> #define GEN11_GFX_MSTR_IRQ			_MMIO(0x190010)
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index d8fde8caff1e..74d7d999a383 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -48,9 +48,9 @@ static void irq_init(struct xe_gt *gt,
> }
> #define IRQ_INIT(gt, type, imr_val, ier_val) \
> 	irq_init((gt), \
>-		 type##IMR, imr_val, \
>-		 type##IER, ier_val, \
>-		 type##IIR)
>+		 IMR(type), imr_val, \
>+		 IER(type), ier_val, \
>+		 IIR(type))
>
> static void irq_reset(struct xe_gt *gt, i915_reg_t imr, i915_reg_t iir,
> 			   i915_reg_t ier)
>@@ -67,7 +67,7 @@ static void irq_reset(struct xe_gt *gt, i915_reg_t imr, i915_reg_t iir,
> 	xe_mmio_read32(gt, iir.reg);
> }
> #define IRQ_RESET(gt, type) \
>-	irq_reset((gt), type##IMR, type##IIR, type##IER)
>+	irq_reset((gt), IMR(type), IIR(type), IER(type))
>
> static u32 gen11_intr_disable(struct xe_gt *gt)
> {
>@@ -90,9 +90,9 @@ gen11_gu_misc_irq_ack(struct xe_gt *gt, const u32 master_ctl)
> 	if (!(master_ctl & GEN11_GU_MISC_IRQ))
> 		return 0;
>
>-	iir = xe_mmio_read32(gt, GEN11_GU_MISC_IIR.reg);
>+	iir = xe_mmio_read32(gt, IIR(GU_MISC_IRQ_REGS).reg);
> 	if (likely(iir))
>-		xe_mmio_write32(gt, GEN11_GU_MISC_IIR.reg, iir);
>+		xe_mmio_write32(gt, IIR(GU_MISC_IRQ_REGS).reg, iir);
>
> 	return iir;
> }
>@@ -173,7 +173,7 @@ static void gen11_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
>
> 	gen11_gt_irq_postinstall(xe, gt);
>
>-	IRQ_INIT(gt, GEN11_GU_MISC_, ~GEN11_GU_MISC_GSE, GEN11_GU_MISC_GSE);
>+	IRQ_INIT(gt, GU_MISC_IRQ_REGS, ~GEN11_GU_MISC_GSE, GEN11_GU_MISC_GSE);
>
> 	gen11_intr_enable(gt, true);
> }
>@@ -336,7 +336,7 @@ static void dg1_irq_postinstall(struct xe_device *xe, struct xe_gt *gt)
> {
> 	gen11_gt_irq_postinstall(xe, gt);
>
>-	IRQ_INIT(gt, GEN11_GU_MISC_, ~GEN11_GU_MISC_GSE, GEN11_GU_MISC_GSE);
>+	IRQ_INIT(gt, GU_MISC_IRQ_REGS, ~GEN11_GU_MISC_GSE, GEN11_GU_MISC_GSE);
>
> 	if (gt->info.id == XE_GT0)
> 		dg1_intr_enable(xe, true);
>@@ -441,8 +441,8 @@ static void gen11_irq_reset(struct xe_gt *gt)
>
> 	gen11_gt_irq_reset(gt);
>
>-	IRQ_RESET(gt, GEN11_GU_MISC_);
>-	IRQ_RESET(gt, GEN8_PCU_);
>+	IRQ_RESET(gt, GU_MISC_IRQ_REGS);
>+	IRQ_RESET(gt, PCU_IRQ_REGS);
> }
>
> static void dg1_irq_reset(struct xe_gt *gt)
>@@ -452,8 +452,8 @@ static void dg1_irq_reset(struct xe_gt *gt)
>
> 	gen11_gt_irq_reset(gt);
>
>-	IRQ_RESET(gt, GEN11_GU_MISC_);
>-	IRQ_RESET(gt, GEN8_PCU_);
>+	IRQ_RESET(gt, GU_MISC_IRQ_REGS);
>+	IRQ_RESET(gt, PCU_IRQ_REGS);
> }
>
> static void xe_irq_reset(struct xe_device *xe)
>-- 
>2.39.2
>