On Mon, Aug 03, 2026 at 11:14:39PM -0300, Tales A. Mendonça wrote: > TLB invalidation timeouts currently leave no record of the firmware > state behind: there is no exec queue or job to blame, so nothing calls > xe_devcoredump() and the GuC log content at the time of the hang is > lost. > > Add xe_devcoredump_gt(), a variant of xe_devcoredump() for hangs that > are not tied to an exec queue or job. It captures the GuC log and CT > state of the affected GT, reusing the existing snapshot machinery and > the "only first snapshot" policy, and hook it up to the TLB invalidation > timeout path. > > This was instrumental in diagnosing GuC TLB invalidation ack stalls on > ARL (see Link), where the invalidation request is consumed from the H2G > CTB immediately but the ack G2H only arrives ~2.3s later, after the > timeout has already fired. >
Thanks for doing this. A couple suggestions. > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8678 > Signed-off-by: Tales A. Mendonça <[email protected]> > --- > drivers/gpu/drm/xe/xe_devcoredump.c | 68 +++++++++++++++++++++++++++++ > drivers/gpu/drm/xe/xe_devcoredump.h | 6 +++ > drivers/gpu/drm/xe/xe_tlb_inval.c | 20 +++++++++ > 3 files changed, 94 insertions(+) > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c > b/drivers/gpu/drm/xe/xe_devcoredump.c > index 5f2b90b18f9..0ccaed176a4 100644 > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c > @@ -403,6 +403,74 @@ void xe_devcoredump(struct xe_exec_queue *q, struct > xe_sched_job *job, const cha > mutex_unlock(&coredump->lock); > } > > +static void devcoredump_snapshot_gt(struct xe_devcoredump *coredump, > + struct xe_gt *gt) > +{ > + struct xe_devcoredump_snapshot *ss = &coredump->snapshot; > + struct xe_guc *guc = >->uc.guc; > + bool cookie; > + > + ss->snapshot_time = ktime_get_real(); > + ss->boot_time = ktime_get_boottime(); > + > + strscpy(ss->process_name, "no process"); > + > + ss->gt = gt; > + INIT_WORK(&ss->work, xe_devcoredump_deferred_snap_work); > + > + /* keep going if fw fails as we still want to save the SW data */ > + CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + > + cookie = dma_fence_begin_signalling(); > + > + ss->guc.log = xe_guc_log_snapshot_capture(&guc->log, true); > + ss->guc.ct = xe_guc_ct_snapshot_capture(&guc->ct); > + > + queue_work(system_dfl_wq, &ss->work); > + > + dma_fence_end_signalling(cookie); > +} > + > +/** > + * xe_devcoredump_gt - Take GT-level snapshots and initialize coredump > device. > + * @gt: The GT where the issue was detected. > + * @fmt: Printf format + args to describe the reason for the core dump > + * > + * Variant of xe_devcoredump() for hangs that are not tied to an exec queue > + * or job, e.g. TLB invalidation timeouts. Captures the GuC log and CT state > + * of @gt so the firmware side of the hang can be inspected. Skipped if a > + * coredump is already captured, same as xe_devcoredump(). > + */ > +__printf(2, 3) > +void xe_devcoredump_gt(struct xe_gt *gt, const char *fmt, ...) I'd unify these functions with the existing xe_devcoredump/devcoredump_snapshot by adding a GT argument to both and teaching those functions that 'q' can be NULL. Then replace s/xe_devcoredump/__xe_devcoredump/ and add wrapper macros xe_devcoredump and xe_devcoredump_gt in the header file. More below. > +{ > + struct xe_device *xe = gt_to_xe(gt); > + struct xe_devcoredump *coredump = &xe->devcoredump; > + va_list varg; > + > + mutex_lock(&coredump->lock); > + > + if (coredump->captured) { > + drm_dbg(&xe->drm, "Multiple hangs are occurring, but only the > first snapshot was taken\n"); > + mutex_unlock(&coredump->lock); > + return; > + } > + > + coredump->captured = true; > + > + va_start(varg, fmt); > + coredump->snapshot.reason = kvasprintf(GFP_ATOMIC, fmt, varg); > + va_end(varg); > + > + devcoredump_snapshot_gt(coredump, gt); > + > + drm_info(&xe->drm, "Xe device coredump has been created\n"); > + drm_info(&xe->drm, "Check your > /sys/class/drm/card%d/device/devcoredump/data\n", > + xe->drm.primary->index); > + > + mutex_unlock(&coredump->lock); > +} > + > static void xe_driver_devcoredump_fini(void *arg) > { > struct drm_device *drm = arg; > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.h > b/drivers/gpu/drm/xe/xe_devcoredump.h > index 5391a80a4d1..f071bd11f24 100644 > --- a/drivers/gpu/drm/xe/xe_devcoredump.h > +++ b/drivers/gpu/drm/xe/xe_devcoredump.h > @@ -11,10 +11,12 @@ > struct drm_printer; > struct xe_device; > struct xe_exec_queue; > +struct xe_gt; > struct xe_sched_job; > > #ifdef CONFIG_DEV_COREDUMP > void xe_devcoredump(struct xe_exec_queue *q, struct xe_sched_job *job, const > char *fmt, ...); > +void xe_devcoredump_gt(struct xe_gt *gt, const char *fmt, ...); This what I'm suggesting for a header... void __xe_devcoredump(struct xe_gt *gt, struct xe_exec_queue *q, struct xe_sched_job *job, const char *fmt, ...); #define xe_devcoredump(_q, _job, _fmt, ...) \ __xe_devcoredump((_q)->gt, _q, _job, _fmt, ##__VA_ARGS__) #define xe_devcoredump_gt(_gt, _fmt, ...) \ __xe_devcoredump(_gt, NULL, NULL, _fmt, ##__VA_ARGS__) I think we need wrapper macros rather than inline wrappers because of how .../##__VA_ARGS__ work. Matt > int xe_devcoredump_init(struct xe_device *xe); > #else > static inline void xe_devcoredump(struct xe_exec_queue *q, > @@ -23,6 +25,10 @@ static inline void xe_devcoredump(struct xe_exec_queue *q, > { > } > > +static inline void xe_devcoredump_gt(struct xe_gt *gt, const char *fmt, ...) > +{ > +} > + > static inline int xe_devcoredump_init(struct xe_device *xe) > { > return 0; > diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c > b/drivers/gpu/drm/xe/xe_tlb_inval.c > index bbd21d39306..833fb92cd3e 100644 > --- a/drivers/gpu/drm/xe/xe_tlb_inval.c > +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c > @@ -5,6 +5,7 @@ > > #include <drm/drm_managed.h> > > +#include "xe_devcoredump.h" > #include "xe_device_types.h" > #include "xe_force_wake.h" > #include "xe_gt_stats.h" > @@ -29,6 +30,12 @@ > > #define FENCE_STACK_BIT DMA_FENCE_FLAG_USER_BITS > > +/* The frontend is only ever embedded in a GT */ > +static struct xe_gt *tlb_inval_to_gt(struct xe_tlb_inval *tlb_inval) > +{ > + return container_of(tlb_inval, struct xe_gt, tlb_inval); > +} > + > static void xe_tlb_inval_fence_fini(struct xe_tlb_inval_fence *fence) > { > if (WARN_ON_ONCE(!fence->tlb_inval)) > @@ -73,6 +80,7 @@ static void xe_tlb_inval_fence_timeout(struct work_struct > *work) > struct xe_device *xe = tlb_inval->xe; > struct xe_tlb_inval_fence *fence, *next; > long timeout_delay = tlb_inval->ops->timeout_delay(tlb_inval); > + int timedout_seqno = 0; > > tlb_inval->ops->flush(tlb_inval); > > @@ -90,6 +98,8 @@ static void xe_tlb_inval_fence_timeout(struct work_struct > *work) > "TLB invalidation fence timeout, seqno=%d recv=%d", > fence->seqno, tlb_inval->seqno_recv); > > + timedout_seqno = fence->seqno; > + > fence->base.error = -ETIME; > xe_tlb_inval_fence_signal(fence); > } > @@ -97,6 +107,16 @@ static void xe_tlb_inval_fence_timeout(struct work_struct > *work) > queue_delayed_work(tlb_inval->timeout_wq, &tlb_inval->fence_tdr, > timeout_delay); > spin_unlock_irq(&tlb_inval->pending_lock); > + > + /* > + * Capture the GuC log and CT state so the firmware side of the hang > + * can be inspected; there is no queue or job to blame here. Must be > + * outside pending_lock as the capture takes sleeping locks. > + */ > + if (timedout_seqno) > + xe_devcoredump_gt(tlb_inval_to_gt(tlb_inval), > + "TLB invalidation fence timeout, seqno=%d > recv=%d", > + timedout_seqno, tlb_inval->seqno_recv); > } > > /** > -- > 2.55.0 >
