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

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