| Message ID | 20211008215635.2026385-9-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915: Initial multi-tile support | expand |
Hi Paulo and Matt, [...] > @@ -3190,14 +3190,19 @@ static void dg1_irq_reset(struct drm_i915_private *dev_priv) mmmhhh... bad naming :/ [...] > - 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() Andi
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
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 <andi.shyti@linux.intel.com> Thanks, Andi
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 = &dev_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 = &dev_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)