Re: [Intel-gfx] [PATCH 08/11] drm/i915/xehp: Make IRQ reset and postinstall multi-tile aware

[Andi Shyti]

Hi Matt,

> > > - dg1_master_intr_enable(uncore->regs);
> > > - intel_uncore_posting_read(uncore, DG1_MSTR_TILE_INTR);
> > > + dg1_master_intr_enable(dev_priv->gt.uncore->regs);
> > > + intel_uncore_posting_read(dev_priv->gt.uncore, DG1_MSTR_TILE_INTR);
> > 
> > I guess this should also go under a for_each_gt()
> 
> DG1_MSTR_TILE_INTR (0x190008) is the top-level, one-per-PCI device
> interrupt register; we always access it via tile0's MMIO .  So in this
> case we do want to do this outside the loop since it's not a per-tile
> operation.

yes of course... we are writing to the master interrupt.

> We could probably simplify the dev_priv->gt.uncore parameter to just
> dev_priv->uncore to make this more obvious.

... it would be a nitpick, but nice to have.

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [Intel-gfx] [PATCH 08/11] drm/i915/xehp: Make IRQ reset and postinstall multi-tile aware

[Matt Roper]

On Thu, Oct 28, 2021 at 06:30:09PM +0200, Andi Shyti wrote:
> Hi Paulo and Matt,
> 
> [...]
> 
> > @@ -3190,14 +3190,19 @@ static void dg1_irq_reset(struct drm_i915_private 
> > *dev_priv)
> 
> mmmhhh... bad naming :/

Even though dg1 wasn't a multi-tile platform, it was the platform that
introduced the singleton "master tile interrupt" register that is
responsible for telling us which tile(s) had interrupts; we then proceed
to read the per-tile master register to find out what those interrupts
are.  So I think the name is accurate since the hardware introduced the
extra level of indirection, and we do need to use this handler on DG1
(we'll just never have more than a single GT to loop over in that case).

> 
> [...]
> 
> > -   dg1_master_intr_enable(uncore->regs);
> > -   intel_uncore_posting_read(uncore, DG1_MSTR_TILE_INTR);
> > +   dg1_master_intr_enable(dev_priv->gt.uncore->regs);
> > +   intel_uncore_posting_read(dev_priv->gt.uncore, DG1_MSTR_TILE_INTR);
> 
> I guess this should also go under a for_each_gt()

DG1_MSTR_TILE_INTR (0x190008) is the top-level, one-per-PCI device
interrupt register; we always access it via tile0's MMIO .  So in this
case we do want to do this outside the loop since it's not a per-tile
operation.

We could probably simplify the dev_priv->gt.uncore parameter to just
dev_priv->uncore to make this more obvious.


Matt

> 
> Andi

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795

[Intel-gfx] [PATCH 08/11] drm/i915/xehp: Make IRQ reset and postinstall multi-tile aware

[Matt Roper]

From: Paulo Zanoni 

Loop through all the tiles when initializing and resetting interrupts.

Signed-off-by: Paulo Zanoni 
Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Matt Roper 
---
 drivers/gpu/drm/i915/i915_irq.c | 28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9f99ad56cde6..e788e283d4a8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3190,14 +3190,19 @@ static void dg1_irq_reset(struct drm_i915_private 
*dev_priv)
 {
struct intel_gt *gt = _priv->gt;
struct intel_uncore *uncore = gt->uncore;
+   unsigned int i;
 
dg1_master_intr_disable(dev_priv->uncore.regs);
 
-   gen11_gt_irq_reset(gt);
-   gen11_display_irq_reset(dev_priv);
+   for_each_gt(dev_priv, i, gt) {
+   gen11_gt_irq_reset(gt);
 
-   GEN3_IRQ_RESET(uncore, GEN11_GU_MISC_);
-   GEN3_IRQ_RESET(uncore, GEN8_PCU_);
+   uncore = gt->uncore;
+   GEN3_IRQ_RESET(uncore, GEN11_GU_MISC_);
+   GEN3_IRQ_RESET(uncore, GEN8_PCU_);
+   }
+
+   gen11_display_irq_reset(dev_priv);
 }
 
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
@@ -3890,13 +3895,16 @@ static void gen11_irq_postinstall(struct 
drm_i915_private *dev_priv)
 
 static void dg1_irq_postinstall(struct drm_i915_private *dev_priv)
 {
-   struct intel_gt *gt = _priv->gt;
-   struct intel_uncore *uncore = gt->uncore;
+   struct intel_gt *gt;
u32 gu_misc_masked = GEN11_GU_MISC_GSE;
+   unsigned int i;
 
-   gen11_gt_irq_postinstall(gt);
+   for_each_gt(dev_priv, i, gt) {
+   gen11_gt_irq_postinstall(gt);
 
-   GEN3_IRQ_INIT(uncore, GEN11_GU_MISC_, ~gu_misc_masked, gu_misc_masked);
+   GEN3_IRQ_INIT(gt->uncore, GEN11_GU_MISC_, ~gu_misc_masked,
+ gu_misc_masked);
+   }
 
if (HAS_DISPLAY(dev_priv)) {
icp_irq_postinstall(dev_priv);
@@ -3905,8 +3913,8 @@ static void dg1_irq_postinstall(struct drm_i915_private 
*dev_priv)
   GEN11_DISPLAY_IRQ_ENABLE);
}
 
-   dg1_master_intr_enable(uncore->regs);
-   intel_uncore_posting_read(uncore, DG1_MSTR_TILE_INTR);
+   dg1_master_intr_enable(dev_priv->gt.uncore->regs);
+   intel_uncore_posting_read(dev_priv->gt.uncore, DG1_MSTR_TILE_INTR);
 }
 
 static void cherryview_irq_postinstall(struct drm_i915_private *dev_priv)
-- 
2.33.0