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

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

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...

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.

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);
> > > > > > >                 }
> > > > > > 
>