[Intel-gfx] [PATCH v3 3/5] drm/i915/mtl: make IRQ reset and postinstall multi-gt aware

[Matt Roper]

On Mon, Mar 06, 2023 at 04:14:49PM -0800, Sripada, Radhakrishna wrote:
> +Daniele,
> 
> Hi Matt,
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > Sent: Monday, March 6, 2023 2:55 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org; De Marchi, Lucas
> > <lucas.demarchi at intel.com>; Zanoni, Paulo R <paulo.r.zanoni at intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v3 3/5] drm/i915/mtl: make IRQ reset and
> > postinstall multi-gt aware
> > 
> > On Wed, Mar 01, 2023 at 12:10:51PM -0800, Radhakrishna Sripada wrote:
> > > Irq reset and post install are to be made multi-gt aware for the
> > > interrupts to work for the media tile on Meteorlake. Iterate through
> > > all the gts to process irq reset for each gt.
> > 
> > I think I mentioned on the previous version, but this isn't right.  MTL
> > does not have separate interrupt registers for each GT the way
> > multi-tile platforms like PVC do.  The GT interrupt registers you're
> > handling here are in the sgunit so there's only a single copy of them;
> > iterating over them multiple times in a row doesn't accomplish anything.
> > 
> > The media engine bits are still on the same registers as the primary GT
> > and the GSC and media GuC are new additional bits that need to be
> > handled.  The necessary handling for the GSC and media GuC should have
> > already landed in a187f13d51fa ("drm/i915/guc: handle interrupts from
> > media GuC") and c07ee636901d ("drm/i915/mtl: add GSC CS interrupt
> > support"), but if there's another bit that was missed somewhere (or if
> > we were doing something like looking at the wrong intel_gt's engine mask
> > somewhere), that would need to be addressed directly rather than just
> > looping over the same IRQ registers multiple times.
> 
> This patch is needed to handle media interrupts. Without this patch we observed
> GuC not loading/communication errors on media gt.
> 
> My understanding is that Sgunit is embedded into the SAMedia block and hence need
> To be iterated over separately.

No, the sgunit is not replicated.  You can confirm by just going to the
various IRQ register pages in the bspec...there's only a single register
offset rather than (offset) and (offset+0x380000) like there are for GT
GSI registers.  The i915 code also only adds the GSI offset to register
operations in the 0x0 - 0x40000 range.


Matt

> 
> Daniele,
> Can you confirm if that is accurate.
> 
> Thanks,
> RK
> > 
> > 
> > Matt
> > 
> > >
> > > Based on original version by Paulo and Tvrtko
> > >
> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 30 ++++++++++++++++++------------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 417c981e4968..9377f59c1ac2 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2759,16 +2759,19 @@ static void gen11_irq_reset(struct
> > drm_i915_private *dev_priv)
> > >
> > >  static void dg1_irq_reset(struct drm_i915_private *dev_priv)
> > >  {
> > > -	struct intel_gt *gt = to_gt(dev_priv);
> > > -	struct intel_uncore *uncore = gt->uncore;
> > > +	struct intel_gt *gt;
> > > +	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(gt, dev_priv, i) {
> > > +		gen11_gt_irq_reset(gt);
> > >
> > > -	GEN3_IRQ_RESET(uncore, GEN11_GU_MISC_);
> > > -	GEN3_IRQ_RESET(uncore, GEN8_PCU_);
> > > +		GEN3_IRQ_RESET(gt->uncore, GEN11_GU_MISC_);
> > > +		GEN3_IRQ_RESET(gt->uncore, GEN8_PCU_);
> > > +	}
> > > +
> > > +	gen11_display_irq_reset(dev_priv);
> > >  }
> > >
> > >  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> > > @@ -3422,13 +3425,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 = to_gt(dev_priv);
> > > -	struct intel_uncore *uncore = gt->uncore;
> > >  	u32 gu_misc_masked = GEN11_GU_MISC_GSE;
> > > +	struct intel_gt *gt;
> > > +	unsigned int i;
> > >
> > > -	gen11_gt_irq_postinstall(gt);
> > > +	for_each_gt(gt, dev_priv, i) {
> > > +		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);
> > > @@ -3437,8 +3443,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(to_gt(dev_priv)->uncore->regs);
> > > +	intel_uncore_posting_read(to_gt(dev_priv)->uncore,
> > DG1_MSTR_TILE_INTR);
> > >  }
> > >
> > >  static void cherryview_irq_postinstall(struct drm_i915_private *dev_priv)
> > > --
> > > 2.34.1
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation