On 3/6/2023 4:24 PM, Matt Roper wrote:
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.
I agree with Matt that this is how the HW works. The issue here is that
the gen11_gt_irq_* functions only program the interrupts for GuC and
engines on the given GT, so when calling them for the root GT they will
only program the interrupts for RCS, BCS, CCS and the root GuC. We could
modify the functions to program all registers from the root GT, but IMO
that doesn't work very well with how other parts of the driver implement
the MTL multi-gt flow.
Daniele
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
