[Intel-xe] [PATCH v2 1/8] drm/xe/irq: Cleanup media GT interrupt flow
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 14 19:04:15 UTC 2023
On Wed, Apr 12, 2023 at 03:52:41PM -0700, Matt Roper wrote:
>Although platforms like MTL have media exposed as a separate GT, it's
>important to recognize that this second GT is *not* a tile as we see in
>platforms like PVC. The interrupt registers (including the ones related
>to GT interrupts) reside in the SGunit so there's only a single instance
>on the entire platform; they are not replicated per-GT. Thus all media
>interrupts are delivered together with the interrupts for the primary
>GT.
>
>v2:
> - Also update reset & postinstall routines to skip media GT.
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_irq.c | 79 ++++++++++++++++++++++++++++++-------
> 1 file changed, 65 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index 13f9f220bca0..7e78eecba62a 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -232,7 +232,40 @@ gt_other_irq_handler(struct xe_gt *gt, const u8 instance, const u16 iir)
> }
> }
>
>-static void gt_irq_handler(struct xe_device *xe, struct xe_gt *gt,
>+static struct xe_gt *find_media_gt(struct xe_device *xe)
>+{
>+ struct xe_gt *gt;
>+ int id;
>+
>+ for_each_gt(gt, xe, id)
>+ if (xe_gt_is_media_type(gt))
>+ return gt;
>+
>+ drm_WARN_ONCE(&xe->drm, 1, "Cannot find media GT for interrupt handling\n");
>+ return xe_device_get_gt(xe, 0);
>+}
>+
>+static struct xe_gt *pick_engine_gt(struct xe_gt *full_gt,
>+ enum xe_engine_class class,
>+ unsigned int instance)
>+{
>+ struct xe_device *xe = gt_to_xe(full_gt);
>+
>+ if (MEDIA_VER(xe) < 13)
>+ return full_gt;
>+
>+ if (class == XE_ENGINE_CLASS_VIDEO_DECODE ||
>+ class == XE_ENGINE_CLASS_VIDEO_ENHANCE)
>+ return find_media_gt(xe);
>+
>+ if (class == XE_ENGINE_CLASS_OTHER &&
>+ instance == OTHER_MEDIA_GUC_INSTANCE)
>+ return find_media_gt(xe);
>+
>+ return full_gt;
>+}
>+
>+static void gt_irq_handler(struct xe_device *xe, struct xe_gt *full_gt,
> u32 master_ctl, long unsigned int *intr_dw,
> u32 *identity)
> {
>@@ -241,27 +274,33 @@ static void gt_irq_handler(struct xe_device *xe, struct xe_gt *gt,
> enum xe_engine_class class;
> struct xe_hw_engine *hwe;
>
>+ /*
>+ * Media GT interrupts are delivered on the primary GT. This function
>+ * should never be called on the media GT itself.
>+ */
>+ drm_WARN_ON_ONCE(&xe->drm, xe_gt_is_media_type(full_gt));
>+
> spin_lock(&xe->irq.lock);
>
> for (bank = 0; bank < 2; bank++) {
> if (!(master_ctl & GT_DW_IRQ(bank)))
> continue;
>
>- if (!xe_gt_is_media_type(gt)) {
>- intr_dw[bank] =
>- xe_mmio_read32(gt, GT_INTR_DW(bank).reg);
>- for_each_set_bit(bit, intr_dw + bank, 32)
>- identity[bit] = gt_engine_identity(xe, gt,
>- bank, bit);
>- xe_mmio_write32(gt, GT_INTR_DW(bank).reg,
>- intr_dw[bank]);
>- }
>+ intr_dw[bank] = xe_mmio_read32(full_gt, GT_INTR_DW(bank).reg);
>+ for_each_set_bit(bit, intr_dw + bank, 32)
>+ identity[bit] = gt_engine_identity(xe, full_gt,
>+ bank, bit);
>+ xe_mmio_write32(full_gt, GT_INTR_DW(bank).reg, intr_dw[bank]);
>
> for_each_set_bit(bit, intr_dw + bank, 32) {
>+ struct xe_gt *gt;
>+
> class = INTR_ENGINE_CLASS(identity[bit]);
> instance = INTR_ENGINE_INSTANCE(identity[bit]);
> intr_vec = INTR_ENGINE_INTR(identity[bit]);
>
>+ gt = pick_engine_gt(full_gt, class, instance);
>+
> if (class == XE_ENGINE_CLASS_OTHER) {
> gt_other_irq_handler(gt, instance, intr_vec);
> continue;
>@@ -369,11 +408,17 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
> }
>
> for_each_gt(gt, xe, id) {
>+ /*
>+ * The media GT isn't a tile, so its interrupts are delivered
>+ * as part of the primary GT's flow.
>+ */
>+ if (xe_gt_is_media_type(gt))
>+ continue;
so it seems your classification for GT vs Tile is: does it have
interrupts routed through it? Or... this gt doensn't have local vram =>
it doesn't process the interrupts.
>+
> if ((master_tile_ctl & DG1_MSTR_TILE(gt->info.vram_id)) == 0)
> continue;
this check actually differentiates it by "does this GT have local vram",
and making the vram_id match the bits in DG1_MSTR_TILE.
I'm actually thinking if the mistake was not making the SAMedia a gt
instead of just attaching its engines to the GT matching its local
vram_id. It would then also mean we wouldn't have to do the whole dance
in xe_pci.c with main gt, remote gt, media gt.
It may be too late for changing that though and we would also need to
change other places like the mmio handling and GuC. But I'm thinking it
would make to a cleaner code as it seems we are adusting several parts
of the code that we wouldn't really need if we didn't consider it as a
separate gt and just used "a tile is a gt" like it used to be.
IMO a slightly less confusing change in this irq handling would be:
unsigned long done_tile = 0;
for_each_gt(gt, xe, id) {
if (__test_and_set_bit(gt->info.vram_id, &done_tile))
continue;
because then it's clear: if a gt points to the same tile, as per
xe_pci.c, we don't want to process interrupts again since it was already
done by the previous loop.
>
>- if (!xe_gt_is_media_type(gt))
>- master_ctl = xe_mmio_read32(gt, GFX_MSTR_IRQ.reg);
>+ master_ctl = xe_mmio_read32(gt, GFX_MSTR_IRQ.reg);
>
> /*
> * We might be in irq handler just when PCIe DPC is initiated
>@@ -386,8 +431,8 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
>- if (!xe_gt_is_media_type(gt))
>- xe_mmio_write32(gt, GFX_MSTR_IRQ.reg, master_ctl);
>+ xe_mmio_write32(gt, GFX_MSTR_IRQ.reg, master_ctl);
>+
> gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
>
> /*
>@@ -473,6 +518,9 @@ static void xe_irq_reset(struct xe_device *xe)
> u8 id;
>
> for_each_gt(gt, xe, id) {
>+ if (xe_gt_is_media_type(gt))
>+ continue;
ditto
And it also seems to have a wrong layering here as in dg1_irq_reset()
we do:
if (gt->info.id == 0)
dg1_intr_disable(gt_to_xe(gt));
that is unrelated to this commit though.
Lucas De Marchi
>+
> if (GRAPHICS_VERx100(xe) >= 1210)
> dg1_irq_reset(gt);
> else
>@@ -486,6 +534,9 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
> {
> struct xe_device *xe = gt_to_xe(gt);
>
>+ if (xe_gt_is_media_type(gt))
>+ return;
>+
> if (GRAPHICS_VERx100(xe) >= 1210)
> dg1_irq_postinstall(xe, gt);
> else
>--
>2.39.2
>
More information about the Intel-xe
mailing list