[Intel-xe] [PATCH v2 1/8] drm/xe/irq: Cleanup media GT interrupt flow
Matt Roper
matthew.d.roper at intel.com
Mon Apr 17 17:33:47 UTC 2023
On Fri, Apr 14, 2023 at 12:04:15PM -0700, Lucas De Marchi wrote:
> 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.
A tile is pretty close to being an entire GPU by itself. It has a
dedicated MMIO space which replicates _all_ registers (including non-GT
registers like sgunit, soc, etc.), it has a dedicated system agent, it
has a dedicated interrupt flow, it has a dedicated GGTT, and it has its
own dedicated LMEM (if discrete). Although the standalone media is on a
separate chip, it's only the small subset of a GPU that the hardware
team refers to as a "GT" and it doesn't have all the other non-GT stuff
required to basically function as a complete GPU on its own.
>
> > +
> > 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.
For the sole multi-tile platform we have today (PVC) it is true that the
ID in the master tile register matches the LMEM ID. We don't have any
platforms yet that combine both multi-tile and standalone media, so it's
hard to say how the hardware will work if/when such a platform shows up
down the road.
>
> 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.
I don't think that would work very well. The GuC is still inside the GT
and thus is replicated on a standalone media platform; the GuC is
completely unaware of the engines from the other GT. There are also
things like forcewake that need to be tracked separately; grabbing the
"GT" forcewake domain on the primary GT does not wake up the "GT" domain
on the media GT or vice versa. It would also likely lead to us
forgetting to do some of the per-GT initialization flows (e.g., things
like applying GT workarounds and such that aren't tied to an engine).
I think what we should really do is break out another level of
structures: xe_tile and xe_gt. An xe_tile can contain multiple xe_gt
structures (e.g., render and media). It would also be the structure
that holds shared things like the GGTT, anything sgunit-specific, etc.
That would also be cleaner than the current nonsense of making 'xe_gt'
the target of MMIO operations, even for stuff that lives outside the GT.
xe_tile {
xe_gt[] {
guc
engine list
forcewake
MCR / steering
GT workarounds
...
}
system agent (MMIO target)
ggtt
lmem
interrupt stuff
...
}
Matt
>
> 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
> >
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list