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@xxxxxxxxx>
> > Sent: Monday, March 6, 2023 2:55 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; De Marchi, Lucas
> > <lucas.demarchi@xxxxxxxxx>; Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>
> > Subject: Re: [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@xxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > > ---
> > > 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
