On Mon, Sep 21, 2026 at 03:10:06PM -0300, Tales A. Mendonça wrote: > On Sun, Sep 20, 2026 at 10:30:00PM -0700, Matthew Brost wrote: > > I discussed with my colleagues - the we agree the TLB component should > > hook into SIGID for logging but we will do this as a follow up on top of > > this change. > > > > So this is: > > Reviewed-by: Matthew Brost <[email protected]> > > Thanks Matt, and thanks for taking the question to the people who own > SIGID rather than letting it sit. > > I will carry your tag on this patch in v5. Agreed on leaving the SIGID > conversion as a follow-up: there is no TLB component in > DEFINE_XE_LOG_COMPONENTS() yet, so adding one touches the log ABI and > belongs with the people doing that work rather than bolted onto a bug > fix. Happy to rebase on top of it once it lands. >
I think we can stage the SIGID behind this, a colleagues volunteered to pick this up after. > v5 is otherwise ready and goes out this week. The only code delta is in > patch 1, from the data race Sashiko spotted after v4. > > While you are here - patch 3 is the one that actually stops the stalls, > and it is still the only one without review. It applies > XE_BO_FLAG_NEEDS_UC to the GuC-shared allocations (CTBs, log, ADS, SLPC, > engine activity) on the standalone media GT, scoped by a new OOB rule > (22016122933 MEDIA_VERSION(1300)). The approach was Daniele's > suggestion. It has been running on two ARL machines for about six weeks > across several kernel versions, with over 10 million invalidations and > zero stalls; before it, both machines hit the ~2.3s ack delay 20-60 > times a day. > > Daniele, Stuart, would either of you be able to take a look at that one? > Daniele is out on sabatical, I'll look now but this is typically not my department but should be able give feedback if I cross code all the code. Matt > Tales > > Em seg., 21 de set. de 2026 às 02:30, Matthew Brost > <[email protected]> escreveu: > > > > On Fri, Sep 18, 2026 at 03:06:25PM -0700, Matthew Brost wrote: > > > On Thu, Sep 17, 2026 at 01:35:52PM -0300, Tales A. Mendonça wrote: > > > > When a TLB invalidation fence times out we log the timeout, but if the > > > > ack for that seqno later shows up there is no record of it, making it > > > > impossible to tell from logs whether the ack was lost forever or merely > > > > (very) late. > > > > > > > > Track the most recent timed out seqno and log how late its ack arrives, > > > > relative to both the original request and the moment the fence was > > > > signaled with -ETIME. > > > > > > > > On ARL with GuC 70.53.0 this shows the acks are never lost: they > > > > consistently arrive ~2.3s after the request, tens of milliseconds after > > > > the TDR has already signaled the fence: > > > > > > > > TLB invalidation fence timeout, seqno=10992 recv=10991 > > > > TLB invalidation late ack: seqno=10992 recv=10992, > > > > request-to-ack=2314ms, timeout-to-ack=45ms > > > > > > > > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8678 > > > > Signed-off-by: Tales A. Mendonça <[email protected]> > > > > > > I would give this an RB but my colleagues have done a bunch of work on > > > SIGID and I haven't been involved at all, so I need some help here. > > > > > > > I discussed with my colleagues - the we agree the TLB component should > > hook into SIGID for logging but we will do this as a follow up on top of > > this change. > > > > So this is: > > Reviewed-by: Matthew Brost <[email protected]> > > > > > > --- > > > > drivers/gpu/drm/xe/xe_tlb_inval.c | 19 +++++++++++++++++++ > > > > drivers/gpu/drm/xe/xe_tlb_inval_types.h | 17 +++++++++++++++++ > > > > 2 files changed, 36 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c > > > > b/drivers/gpu/drm/xe/xe_tlb_inval.c > > > > index 7a0c04fac60..7047a347551 100644 > > > > --- a/drivers/gpu/drm/xe/xe_tlb_inval.c > > > > +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c > > > > @@ -14,6 +14,7 @@ > > > > #include "xe_guc_tlb_inval.h" > > > > #include "xe_mmio.h" > > > > #include "xe_pm.h" > > > > +#include "xe_printk.h" > > > > #include "xe_tlb_inval.h" > > > > #include "xe_trace.h" > > > > > > > > @@ -99,6 +100,11 @@ static void xe_tlb_inval_fence_timeout(struct > > > > work_struct *work) > > > > fence->seqno, tlb_inval->seqno_recv); > > > > > > > > timedout_seqno = fence->seqno; > > > > + if (!tlb_inval->timedout_seqno) { > > > > + tlb_inval->timedout_seqno = fence->seqno; > > > > + tlb_inval->timedout_inval_time = fence->inval_time; > > > > + tlb_inval->timedout_time = ktime_get(); > > > > + } > > > > > > > > fence->base.error = -ETIME; > > > > xe_tlb_inval_fence_signal(fence); > > > > @@ -227,6 +233,7 @@ void xe_tlb_inval_reset(struct xe_tlb_inval > > > > *tlb_inval) > > > > else > > > > pending_seqno = tlb_inval->seqno - 1; > > > > WRITE_ONCE(tlb_inval->seqno_recv, pending_seqno); > > > > + tlb_inval->timedout_seqno = 0; > > > > > > > > list_for_each_entry_safe(fence, next, > > > > &tlb_inval->pending_fences, link) > > > > @@ -454,6 +461,18 @@ void xe_tlb_inval_done_handler(struct xe_tlb_inval > > > > *tlb_inval, int seqno) > > > > > > > > WRITE_ONCE(tlb_inval->seqno_recv, seqno); > > > > > > > > + if (tlb_inval->timedout_seqno && > > > > + xe_tlb_inval_seqno_past(tlb_inval, tlb_inval->timedout_seqno)) { > > > > + ktime_t now = ktime_get(); > > > > + > > > > + xe_warn(xe, > > > > + "TLB invalidation late ack: seqno=%d recv=%d, > > > > request-to-ack=%lldms, timeout-to-ack=%lldms", > > > > + tlb_inval->timedout_seqno, seqno, > > > > + ktime_ms_delta(now, tlb_inval->timedout_inval_time), > > > > + ktime_ms_delta(now, tlb_inval->timedout_time)); > > > > > > Should this be some type SIGID message? > > > > > > Matt > > > > > > > + tlb_inval->timedout_seqno = 0; > > > > + } > > > > + > > > > list_for_each_entry_safe(fence, next, > > > > &tlb_inval->pending_fences, link) { > > > > trace_xe_tlb_inval_fence_recv(xe, fence); > > > > diff --git a/drivers/gpu/drm/xe/xe_tlb_inval_types.h > > > > b/drivers/gpu/drm/xe/xe_tlb_inval_types.h > > > > index d77be1aedc9..80d2019fa20 100644 > > > > --- a/drivers/gpu/drm/xe/xe_tlb_inval_types.h > > > > +++ b/drivers/gpu/drm/xe/xe_tlb_inval_types.h > > > > @@ -112,6 +112,23 @@ struct xe_tlb_inval { > > > > * @pending_lock: protects @pending_fences and updating @seqno_recv. > > > > */ > > > > spinlock_t pending_lock; > > > > + /** > > > > + * @timedout_seqno: seqno of the most recent timed out TLB > > > > + * invalidation, 0 if none. Used to measure how late the ack for a > > > > + * timed out invalidation actually arrives. Protected by > > > > + * @pending_lock. > > > > + */ > > > > + int timedout_seqno; > > > > + /** > > > > + * @timedout_inval_time: request time of @timedout_seqno. Protected > > > > by > > > > + * @pending_lock. > > > > + */ > > > > + ktime_t timedout_inval_time; > > > > + /** > > > > + * @timedout_time: time @timedout_seqno was signaled with -ETIME. > > > > + * Protected by @pending_lock. > > > > + */ > > > > + ktime_t timedout_time; > > > > /** > > > > * @fence_tdr: schedules a delayed call to xe_tlb_fence_timeout > > > > after > > > > * the timeout interval is over. > > > > -- > > > > 2.55.0 > > > > > > > > -- > Com os cumprimentos, > > Tales A. Mendonça > talesam.org > communitybig.org
