Re: [PATCH] drm/xe: Wait for HW clearance before issuing the next TLB inval.

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Mar 25, 2026 at 04:43:33PM -0600, Summers, Stuart wrote:
> On Wed, 2026-03-25 at 15:38 -0700, Matthew Brost wrote:
> > On Wed, Mar 25, 2026 at 04:25:22PM -0600, Summers, Stuart wrote:
> > > On Wed, 2026-03-25 at 15:00 -0700, Matthew Brost wrote:
> > > > On Wed, Mar 25, 2026 at 12:37:18PM -0600, Summers, Stuart wrote:
> > > > > On Tue, 2026-03-24 at 16:36 -0700, Matthew Brost wrote:
> > > > > > On Tue, Mar 24, 2026 at 03:10:41PM -0600, Summers, Stuart
> > > > > > wrote:
> > > > > > > On Tue, 2026-03-24 at 13:58 -0700, Matthew Brost wrote:
> > > > > > > > On Tue, Mar 24, 2026 at 01:53:27PM -0700, Matthew Brost
> > > > > > > > wrote:
> > > > > > > > > On Tue, Mar 24, 2026 at 02:39:54PM -0600, Yang, Fei
> > > > > > > > > wrote:
> > > > > > > > > > > On Tue, Mar 17, 2026 at 05:28:14PM -0600, Summers,
> > > > > > > > > > > Stuart
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Tue, 2026-03-17 at 16:21 -0700,
> > > > > > > > > > > > fei.yang@intel.com wrote:
> > > > > > > > > > > > > From: Fei Yang <fei.yang@intel.com>
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Hardware requires the software to poll the
> > > > > > > > > > > > > valid
> > > > > > > > > > > > > bit
> > > > > > > > > > > > > and
> > > > > > > > > > > > > make sure
> > > > > > > > > > > > > it's cleared before issuing a new TLB
> > > > > > > > > > > > > invalidation
> > > > > > > > > > > > > request.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Fei Yang <fei.yang@intel.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/gpu/drm/xe/xe_guc_tlb_inval.c | 15
> > > > > > > > > > > > > +++++++++++++++
> > > > > > > > > > > > >  1 file changed, 15 insertions(+)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > > > > > > > > > > b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > > > > > > > > > > index ced58f46f846..4c2f87db3167 100644
> > > > > > > > > > > > > --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > > > > > > > > > > +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > > > > > > > > > > @@ -63,6 +63,7 @@ static int
> > > > > > > > > > > > > send_tlb_inval_ggtt(struct
> > > > > > > > > > > > > xe_tlb_inval
> > > > > > > > > > > > > *tlb_inval, u32 seqno)
> > > > > > > > > > > > >         struct xe_guc *guc = tlb_inval-
> > > > > > > > > > > > > >private;
> > > > > > > > > > > > >         struct xe_gt *gt = guc_to_gt(guc);
> > > > > > > > > > > > >         struct xe_device *xe = guc_to_xe(guc);
> > > > > > > > > > > > > +       int ret;
> > > > > > > > > > > > > 
> > > > > > > > > > > > >         /*
> > > > > > > > > > > > >          * Returning -ECANCELED in this
> > > > > > > > > > > > > function is
> > > > > > > > > > > > > squashed at the
> > > > > > > > > > > > > caller and @@ -85,11 +86,25 @@ static int
> > > > > > > > > > > > > send_tlb_inval_ggtt(struct
> > > > > > > > > > > > > xe_tlb_inval *tlb_inval, u32 seqno)
> > > > > > > > > > > > > 
> > > > > > > > > > > > >                 CLASS(xe_force_wake,
> > > > > > > > > > > > > fw_ref)(gt_to_fw(gt),
> > > > > > > > > > > > > XE_FW_GT);
> > > > > > > > > > > > >                 if (xe->info.platform == XE_PVC
> > > > > > > > > > > > > ||
> > > > > > > > > > > > > GRAPHICS_VER(xe)
> > > > > > > > > > > > > > = 20) {
> > > > > > > > > > > > > +                       /* Wait 1-second for
> > > > > > > > > > > > > the
> > > > > > > > > > > > > valid
> > > > > > > > > > > > > bit
> > > > > > > > > > > > > to be
> > > > > > > > > > > > > cleared */
> > > > > > > > > > > > > +                       ret =
> > > > > > > > > > > > > xe_mmio_wait32(mmio,
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC0,
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC0_VALID,
> > > > > > > > > > > > > +                                            0,
> > > > > > > > > > > > > 1000 *
> > > > > > > > > > > > > +USEC_PER_MSEC,
> > > > > > > > > > > > > NULL, false);
> > > > > > > > > > > > > +                       if (ret) {
> > > > > > > > > > > > > +                               pr_info("TLB
> > > > > > > > > > > > > INVAL
> > > > > > > > > > > > > cancelled due to
> > > > > > > > > > > > > uncleared valid bit\n");
> > > > > > > > > > > > > +                               return -
> > > > > > > > > > > > > ECANCELED;
> > > > > > > > > > > > > +                       }
> > > > > > > > > > > > 
> > > > > > > > > > > > Is there a reason we aren't waiting after the
> > > > > > > > > > > > write
> > > > > > > > > > > > to
> > > > > > > > > > > > make
> > > > > > > > > > > > sure the
> > > > > > > > > > > > invalidation completed? It seems like we should
> > > > > > > > > > > > be
> > > > > > > > > > > > serializing these
> > > > > > > > > > > > and at least making sure hardware completes the
> > > > > > > > > > > > request
> > > > > > > > > > > > rather than
> > > > > > > > > > > > just sending and hoping for the best.
> > > > > > > > > > > 
> > > > > > > > > > > Yes, this is correct—we should after wait issue
> > > > > > > > > > > *if*
> > > > > > > > > > > this
> > > > > > > > > > > code
> > > > > > > > > > > is actually needed.
> > > > > > > > > > 
> > > > > > > > > > No, the issue is that software cannot issue another
> > > > > > > > > > TLB
> > > > > > > > > > invalidation request while the ongoing
> > > > > > > > > > one has not been completed yet. Otherwise the
> > > > > > > > > > hardware
> > > > > > > > > > could
> > > > > > > > > > potentially lockup.
> > > > > > > > > > So we need to make sure the valid bit is cleared
> > > > > > > > > > before
> > > > > > > > > > issuing
> > > > > > > > > > another TLB invalidation request.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Yes, but we signal the TLB invalidation fence as
> > > > > > > > > complete
> > > > > > > > > without
> > > > > > > > > waiting for the hardware to actually finish. The
> > > > > > > > > locking
> > > > > > > > > here
> > > > > > > > > is
> > > > > > > > > also
> > > > > > > > > incorrect for MMIO-based invalidations, now that I
> > > > > > > > > think
> > > > > > > > > about
> > > > > > > > > it.
> > > > > > > > > What
> > > > > > > > > really needs to happen is:
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Ah, this actually another weird corner where we take down
> > > > > > > > the
> > > > > > > > CTs
> > > > > > > > but
> > > > > > > > GuC is still using the GAM port...
> > > > > > > > 
> > > > > > > > > - In send_tlb_inval_ggtt(), if the MMIO path is taken,
> > > > > > > > > acquire
> > > > > > > > > a
> > > > > > > > > per-GT
> > > > > > > > >   MMIO TLB invalidation lock after obtaining the FW
> > > > > > > > 
> > > > > > > > So maybe 'Wait for the valid bit to clear' here too but
> > > > > > > > this
> > > > > > > > still
> > > > > > > > isn't
> > > > > > > > fully hardend as the GuC could immediately use the GAM
> > > > > > > > port
> > > > > > > > again...
> > > > > > > > 
> > > > > > > > Or perhaps we go straight to my suggestion below - when
> > > > > > > > reloading
> > > > > > > > the
> > > > > > > > GuC issue MMIO GT invalidation...
> > > > > > > 
> > > > > > > I feel like we really should be avoiding these MMIO based
> > > > > > > invalidations
> > > > > > > wherever possible. It creates a lot of race conditions like
> > > > > > > what
> > > > > > > you
> > > > > > > suggested or even parallel invalidation between the GuC and
> > > > > > > KMD
> > > > > > > while
> > > > > > > we're tearing down (KMD lock might not be able to guarantee
> > > > > > > the
> > > > > > > GuC
> > > > > > > isn't still invalidating).
> > > > > > > 
> > > > > > 
> > > > > > My guess is the issue calling xe_managed_bo_reinit_in_vram on
> > > > > > some
> > > > > > BOs -
> > > > > > the GGTT don't get invalidated GuC side and it reloads with
> > > > > > stale
> > > > > > GGTTs.
> > > > > > 
> > > > > > > Can we instead rely more heavily on the GT reset to flush
> > > > > > > the
> > > > > > > TLBs
> > > > > > > for
> > > > > > 
> > > > > > We likely need a MMIO invalidate whenever doing PM resume
> > > > > > events
> > > > > > too
> > > > > > as memory can move without PM refs (CTs go down when PM ref
> > > > > > == 0)
> > > > > > if
> > > > > > I'm
> > > > > > not mistaken...
> > > > > 
> > > > > But we already do a GT reset in that case right? So this should
> > > > > be
> > > > > covered?
> > > > > 
> > > > 
> > > > D3hot doesn't perform a GT reset or enter D3cold. However,
> > > > looking at
> > > > xe_ggtt.c, GGTT invalidations always hold a PM reference, so the
> > > > CT
> > > > should remain live unless a GT reset is in progress.
> > > > 
> > > > Therefore, the only two cases where we might need MMIO
> > > > invalidations
> > > > are:
> > > > 
> > > > - During initial driver load, after we call
> > > > xe_managed_bo_reinit_in_vram
> > > >   on some buffers used during the hwconfig GuC load phase, and
> > > > then
> > > > move
> > > >   the memory while retaining the same GGTT addresses.
> > > > 
> > > > - During a GT reset, where memory is allowed to move around.
> > > > Likely
> > > > do
> > > >   this step before reloading the GuC.
> > > 
> > > But for both of these cases, we should be able to do:
> > > for_each_gt_on_tile()
> > >     xe_tlb_inval_fence_init()
> > >     xe_tlb_inval_all()
> > > for_each_gt_on_tile()
> > >     xe_tlb_inval_all()
> > > 
> > > Right before GuC communication goes down right? Then we wouldn't
> > > need
> > > any MMIO communication since the TLBs are flushed at that point.
> > 
> > CTs are live during hwconfig GuC load phase but not so much in the GT
> > reset case.
> > 
> > What is probably easiest /safest is any place xe_uc_load_hw is
> > called,
> > do a MMIO invalidation of GuC TLB immediately before (but skip this
> > step
> > on a VF).
> 
> We need a way to quiesce GuC and HW if we're going to do this. We could
> issue the reset and disable CT communication, but GuC still is in the
> middle of performing a TLB invalidation, thus conflicting with ours.
> 

Yes, through the layers we always call xe_guc_reset before
xe_uc_load_hw. Based on Fei’s observations, a GDRST doesn’t wipe out
the TLBs, so the best place for an MMIO TLB invalidate of the GuC TLBs
may actually be in xe_guc_reset after the GDRST completes.

> For the GT reset case are you worried about a situation where the
> hardware is busted and we reset after the fact to recover? Or a more

No — we wedge the device if GDRST fails, and then (WIP) an FLR can be
issued to recover. I assume an FLR will wipe the TLBs.

> explicit invalidation in a TDR scenario? In the former, I wouldn't
> think any invalidation would matter there. In the latter, it still
> seems like we could invalidate TLBs explicitly through GuC before
> disabling CT communication and writing GDRST.
> 

I think placing it after the GDRST is correct, as that ensures the GuC
is no longer using the GAM port. We likely still need the “wait for
valid to clear before + after” semantics in case a GDRST races with the
GuC programming the GAM port.

> And on that note, the GT reset I would think should clear the TLBs for

It doesn’t, based on Fei’s observations, as we do a GDRST before
reloading the GuC after the hwconfig phase. This actually makes sense,
since the GAM and TLBs are separate hardware components from the GuC IP.

> us. Do we really care about any explicit invalidation from the KMD in
> that case? It seems like the main interesting case here is the time
> between the hwconfig GuC load and the main GuC load where we overwrite
> the same memory and thus need to flush.
> 

So, the TL;DR of what needs to be done:

- Update xe_guc_tlb_inval.c to blindly send CT-based GGTT invalidations
  (like PPGTT invalidations). If the CT is down, the error gets squashed
  and the upper layers move on. In other words, drop the MMIO
  invalidations.

- Add a xe_gam_port component that has a lock around MMIO (to
  future-proof this) and a single function,
  xe_gam_port_tlb_inval_ggtt(), which, under the lock, polls for the valid 
  bit to clear, issues the GGTT invalidation, and then polls again for the
  valid bit to clear.

- In xe_guc_reset(), the last thing it does is call
  xe_gam_port_tlb_inval_ggtt().

Matt

> Thanks,
> Stuart
> 
> > 
> > Matt
> > 
> > > 
> > > Thanks,
> > > Stuart
> > > 
> > > > 
> > > > > > 
> > > > > > I'd also like the GAM port interaction broken out in
> > > > > > component
> > > > > > like
> > > > > > xe_gam_port.c (with a dedicated lock) in this seires [1]
> > > > > > albiet
> > > > > > way
> > > > > > simplier as we only need GGTT invalidates at this time.
> > > > > 
> > > > > I'd still like to push we find a way to completely remove the
> > > > > MMIO
> > > > > invalidation from the driver and rely on either the resets or
> > > > > GuC
> > > > > based
> > > > > invalidation prior to teardowns if at all possible.
> > > > > 
> > > > 
> > > > I agree MMIO TLB invalidations have no place xe_guc_tlb_inval.c.
> > > > 
> > > > Matt
> > > > 
> > > > > I hadn't seen your other patch yet though. I'll take a look as
> > > > > soon
> > > > > as
> > > > > I have time.
> > > > > 
> > > > > Thanks,
> > > > > Stuart
> > > > > 
> > > > > > 
> > > > > > Matt
> > > > > > 
> > > > > > [1]
> > > > > > https://patchwork.freedesktop.org/patch/707237/?series=162171&rev=1
> > > > > > 
> > > > > > > us? And for the GuC memory specifically, maybe we do a full
> > > > > > > invalidation after quiescing the GuC during hwconfig load
> > > > > > > (the
> > > > > > > first
> > > > > > > time we load the GuC during driver load) and before any
> > > > > > > kind of
> > > > > > > reload/reset?
> > > > > > > 
> > > > > > > We'd still need to cover the case where hardware is fully
> > > > > > > hung
> > > > > > > up
> > > > > > > and
> > > > > > > GuC isn't responding, but then I don't know that we really
> > > > > > > care
> > > > > > > about
> > > > > > > MMIO based invalidations since we'll want to fully reset
> > > > > > > the GT
> > > > > > > there
> > > > > > > too.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Stuart
> > > > > > > 
> > > > > > > > 
> > > > > > > > Matt
> > > > > > > > 
> > > > > > > > > - Issue the TLB invalidation
> > > > > > > > > - Wait for the valid bit to clear
> > > > > > > > > - Release the GT MMIO TLB invalidation lock
> > > > > > > > > 
> > > > > > > > > Without this lock, two threads could both observe the
> > > > > > > > > valid
> > > > > > > > > bit
> > > > > > > > > clearing
> > > > > > > > > and then both attempt to issue invalidations,
> > > > > > > > > clobbering
> > > > > > > > > each
> > > > > > > > > other.
> > > > > > > > > 
> > > > > > > > > > > This is early Xe code from me, and it’s
> > > > > > > > > > > questionable
> > > > > > > > > > > whether
> > > > > > > > > > > it’s even required.
> > > > > > > > > > 
> > > > > > > > > > This seems to be required, otherwise modprobe would
> > > > > > > > > > fail
> > > > > > > > > > at
> > > > > > > > > > golden context submission,
> > > > > > > > > > [  480.237382] xe 0000:01:00.0: [drm] *ERROR* Tile0:
> > > > > > > > > > GT0:
> > > > > > > > > > hwe
> > > > > > > > > > ccs0: nop emit_nop_job failed (-ETIME) guc_id=4
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I’m somewhat surprised by this. A better solution might
> > > > > > > > > be
> > > > > > > > > to
> > > > > > > > > drop
> > > > > > > > > the
> > > > > > > > > MMIO GT invalidation code in xe_guc_tlb_inval.c and
> > > > > > > > > instead
> > > > > > > > > issue
> > > > > > > > > an
> > > > > > > > > MMIO GGTT invalidation whenever we reload the GuC.
> > > > > > > > > 
> > > > > > > > > We can defer trying this until later, as it is a
> > > > > > > > > riskier
> > > > > > > > > change.
> > > > > > > > > 
> > > > > > > > > Matt
> > > > > > > > > 
> > > > > > > > > > > Typically, if the CTs are not live, the GuC isn’t
> > > > > > > > > > > doing
> > > > > > > > > > > anything meaningful in terms of
> > > > > > > > > > > referencing memory that the KMD is moving around
> > > > > > > > > > > (which
> > > > > > > > > > > would
> > > > > > > > > > > require an invalidation).
> > > > > > > > > > > So this entire flow of issuing a GAM port TLB
> > > > > > > > > > > invalidation
> > > > > > > > > > > is,
> > > > > > > > > > > again, questionable.
> > > > > > > > > > > 
> > > > > > > > > > > So I'd suggest move the wait after issue, plus
> > > > > > > > > > > throw
> > > > > > > > > > > in:
> > > > > > > > > > > 
> > > > > > > > > > > “XXX: Why do we need to invalidate GGTT memory when
> > > > > > > > > > > the
> > > > > > > > > > > CTs
> > > > > > > > > > > are
> > > > > > > > > > > not live? This suggests
> > > > > > > > > > > the GuC is still in the load phase. Investigate and
> > > > > > > > > > > remove
> > > > > > > > > > > this
> > > > > > > > > > > code once confirmed.'
> > > > > > > > > > 
> > > > > > > > > > The issue is a consequence of an earlier failure
> > > > > > > > > > which
> > > > > > > > > > caused
> > > > > > > > > > the
> > > > > > > > > > CT to be disabled. And the KMD
> > > > > > > > > > sees a bunch of TLB invalidation timeouts.
> > > > > > > > > > At this time I would expect a GT reset, but that is
> > > > > > > > > > not
> > > > > > > > > > how
> > > > > > > > > > Xe
> > > > > > > > > > behaves (the ole i915 driver triggers
> > > > > > > > > > a GT reset on TLB invalidation timeout if I remember
> > > > > > > > > > correctly)
> > > > > > > > > > 
> > > > > > > > > > -Fei
> > > > > > > > > > 
> > > > > > > > > > > Matt
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks,
> > > > > > > > > > > > Stuart
> > > > > > > > > > > > 
> > > > > > > > > > > > >                         xe_mmio_write32(mmio,
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC1,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC1_INVALID ATE);
> > > > > > > > > > > > >                         xe_mmio_write32(mmio,
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC0,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > PVC_GUC_TLB_INV_DESC0_VALID);
> > > > > > > > > > > > >                 } else {
> > > > > > > > > > > > > +                       /* Wait 1-second for
> > > > > > > > > > > > > the
> > > > > > > > > > > > > valid
> > > > > > > > > > > > > bit
> > > > > > > > > > > > > to be
> > > > > > > > > > > > > cleared */
> > > > > > > > > > > > > +                       ret =
> > > > > > > > > > > > > xe_mmio_wait32(mmio,
> > > > > > > > > > > > > GUC_TLB_INV_CR,
> > > > > > > > > > > > > GUC_TLB_INV_CR_INVALIDATE,
> > > > > > > > > > > > > +                                            0,
> > > > > > > > > > > > > 1000 *
> > > > > > > > > > > > > +USEC_PER_MSEC,
> > > > > > > > > > > > > NULL, false);
> > > > > > > > > > > > > +                       if (ret) {
> > > > > > > > > > > > > +                               pr_info("TLB
> > > > > > > > > > > > > INVAL
> > > > > > > > > > > > > cancelled due to
> > > > > > > > > > > > > uncleared valid bit\n");
> > > > > > > > > > > > > +                               return -
> > > > > > > > > > > > > ECANCELED;
> > > > > > > > > > > > > +                       }
> > > > > > > > > > > > >                         xe_mmio_write32(mmio,
> > > > > > > > > > > > > GUC_TLB_INV_CR,
> > > > > > > > > > > > >                                        
> > > > > > > > > > > > > GUC_TLB_INV_CR_INVALIDATE);
> > > > > > > > > > > > >                 }
> > > > > > > > > > > > 
> > > > > > > 
> > > > > 
> > > 
>