Re: [PATCH v1 2/4] drm/xe/mcr: Sanitize steering semaphore on GT resume

Re: [PATCH v1 2/4] drm/xe/mcr: Sanitize steering semaphore on GT resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing MCR lock sanitization during driver load despite documented claims.
--

--- Patch [2]: [PATCH v1 2/4] drm/xe/mcr: Sanitize steering semaphore on GT resume ---
commit 5b9e1b6b52eef7a040056b851f2fb96429ee7d28
Author: Tales A. Mendonça <talesam@gmail.com>

drm/xe/mcr: Sanitize steering semaphore on GT resume

This commit addresses an issue where the MCR steering semaphore is left
held across a suspend cycle. It adds xe_gt_mcr_lock_sanitize() to forcibly
release the semaphore and calls it in xe_gt_resume() before the first MCR
access.

> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> index e1e3c0688dc2e..3934d0de7170a 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
[ ... ]
> @@ -750,6 +750,28 @@ static void mcr_unlock(struct xe_gt *gt, unsigned int fw_ref) __releases(&gt->mc
>  	}
>  }
>  
> +/**
> + * xe_gt_mcr_lock_sanitize - Release the hardware MCR steering semaphore
> + * @gt: GT structure
> + *
> + * The MCR steering semaphore can be left in a held state after a suspend
> + * cycle, in which case the first MCR access after resume stalls until the
> + * semaphore wait times out.  Forcibly release the semaphore during driver
> + * load/resume; this is safe because no other agents acquire the semaphore

[Severity: Medium]
The comment and commit message both mention releasing the semaphore during
driver load, but the patch only adds the sanitization call to xe_gt_resume().

If the boot firmware leaves the MCR lock held, won't the first MCR access
during driver load still stall and log a timeout error?

For example, this could occur in early initialization paths like:
xe_device_probe()
  -> xe_gt_init_early()
    -> xe_pat_init()

Or in:
gt_init_with_gt_forcewake()
  -> xe_gt_enable_host_l2_vram()

Should a call to xe_gt_mcr_lock_sanitize() be added to the early driver
initialization paths to fully cover the load sequence?

> + * during the load/resume process, mirroring i915's
> + * intel_gt_mcr_lock_sanitize().
> + */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722004654.744249-1-talesam@gmail.com?part=2

Re: [PATCH v1 1/4] drm/xe/mcr: Keep GT forcewake during MCR steering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected concurrent access to GuC capture `cachelist` and `outlist`.
--

--- Patch [1]: [PATCH v1 1/4] drm/xe/mcr: Keep GT forcewake during MCR steering ---
commit 3d26258fb8d98aa82b5daf58a00b99ab1ef9b4be
Author: Tales A. Mendonça <talesam@gmail.com>

drm/xe/mcr: Keep GT forcewake during MCR steering

This commit ports the i915 MCR locking model to the xe driver. It changes
the locking sequence to wait for the hardware semaphore before taking the
software spinlock, allowing it to sleep rather than spin. It also ensures
GT forcewake is held over the entire lock, steer, and unlock cycle.

> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> index a97b236dab7c9..e1e3c0688dc2e 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c

[Severity: High]
This isn't a bug introduced by this patch, but does the GuC capture code
lack locking when modifying the cachelist and outlist?

These double-linked lists appear to be accessed and modified concurrently
by multiple un-serialized asynchronous contexts. This could be triggered via
GPU hangs or timeouts during normal operation.

For example, during G2H event processing, the list is modified directly
when the CT worker calls xe_guc_capture_process():

drivers/gpu/drm/xe/xe_guc_capture.c:guc_capture_get_prealloc_node() {
        ...
        list_del(&found->link);
        ...
}

Concurrently, during DRM scheduler job timeouts, the outlist is also
modified without locking after xe_engine_manual_capture() calls
xe_guc_capture_get_matching_and_lock():

drivers/gpu/drm/xe/xe_guc_capture.c:guc_capture_add_node_to_outlist() {
        ...
        guc_capture_add_node_to_list(node, &gc->outlist);
        ...
}

Other contexts like devcoredump freeing also access these lists via
xe_guc_capture_put_matched_nodes().

Could concurrent modification of these lists without locking (such as
spinlocks) cause list corruption and inevitably lead to a kernel panic
when the lists are later traversed or modified?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722004654.744249-1-talesam@gmail.com?part=1

Re: [PATCH v1 3/4] drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] TOCTOU race condition in `xe_guc_ct_flush_g2h` can cause a Use-After-Free of `ct->g2h_wq` during device teardown.
--

--- Patch [3]: [PATCH v1 3/4] drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths ---
commit 2288520019a1eaed705d5a065fcb111af0102b2f
Author: Tales A. Mendonça <talesam@gmail.com>

drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths

This commit introduces xe_guc_ct_flush_g2h() to queue the G2H worker before
flushing it in timeout-recovery paths. This guarantees the flush always
drains the CTB even if the GUC2HOST interrupt was lost or coalesced.

> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index fe70c0fd85c52..11a05c2b8c773 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c

[ ... ]

> @@ -2046,6 +2046,27 @@ static void g2h_worker_func(struct work_struct *w)
>  	receive_g2h(ct);
>  }
>  
> +/**
> + * xe_guc_ct_flush_g2h() - Force processing of pending G2H messages
> + * @ct: GuC CT object
> + *
> + * The GUC2HOST interrupt for a G2H message may be lost or coalesced. When
> + * that happens the G2H worker is never queued and flushing it is a no-op
> + * that does not read the G2H CTB, leaving messages the GuC has already
> + * posted unprocessed until the next interrupt arrives. Queue the worker
> + * before flushing it so the flush always drains the G2H CTB. A spurious
> + * worker run is safe: it returns without side effects if the CTB is empty
> + * or CT communication is disabled.
> + */
> +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct)
> +{
> +	if (!xe_guc_ct_enabled(ct))
> +		return;
> +
> +	queue_work(ct->g2h_wq, &ct->g2h_worker);

[Severity: High]
Can a Time-of-Check to Time-of-Use (TOCTOU) race happen here during
concurrent device teardown?

If a timeout path like xe_tlb_inval_fence_timeout() calls this function on
an async worker, it checks xe_guc_ct_enabled(ct) without any locks. If the
thread is preempted right after evaluating to true:

Thread 1 (timeout worker):
xe_guc_ct_flush_g2h() {
    if (!xe_guc_ct_enabled(ct))
        // evaluates to true, thread preempted here

Thread 2 (device teardown):
Proceeds to disable the CT via the devm hook (guc_action_disable_ct) and
destroys the workqueue via the drmm hook (guc_ct_fini).

When Thread 1 resumes, it will call queue_work(ct->g2h_wq, ...) on the
destroyed workqueue pointer, which could result in a use-after-free.

Since drmm hooks execute in LIFO order, the guc_ct_fini() hook destroys the
workqueue before the teardown path would wait for the timeout worker via
destroy_workqueue(xe->ordered_wq).

Is there a way to safely synchronize the queue_work() call or validate
that the workqueue is still alive while holding a lock?

> +	flush_work(&ct->g2h_worker);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722004654.744249-1-talesam@gmail.com?part=3

Re: [PATCH v1 1/4] drm/xe/mcr: Keep GT forcewake during MCR steering

[Matt Roper]

On Tue, Jul 21, 2026 at 09:46:51PM -0300, Tales A. Mendonça wrote:
> On MTL and newer platforms xe uses STEER_SEMAPHORE to synchronize MCR
> steering with external agents such as firmware. The current code waits
> only 10us for the semaphore, while already holding the software MCR
> spinlock, and fires drm_WARN_ON_ONCE() with a full stack trace when the
> wait times out. On an Arrow Lake-P system this triggers during resume
> while firmware still owns the semaphore:
> 
>   drm_WARN_ON_ONCE(ret == -110)
>   WARNING: drivers/gpu/drm/xe/xe_gt_mcr.c:697 mcr_lock
> 
> Port the i915 MCR locking model (intel_gt_mcr_lock/unlock) to xe:
> 
> - Wait for the hardware semaphore before taking the software spinlock,
>   so the wait may sleep instead of spinning inside the critical
>   section.
> - Allow up to 100ms for firmware to release the semaphore, matching
>   i915.
> - Demote the WARN stack to a rate-limited GT error, using the same
>   message as i915 for easier cross-driver triage.
> - Grab GT forcewake before touching the semaphore and hold it over the
>   entire lock/steer/unlock cycle (Wa_22018931422). The steering
>   registers are in an "always on" domain with respect to RC6, but are
>   sensitive while higher-level platform sleep states are
>   entering/exiting.

This workaround only applies to specific platforms (MTL and ARL), but
your implementation here is grabbing forcewake on all platforms, which
we don't want to happen.  Workarounds should be implemented through the
workaround layers of the driver (either RTP or OOB) so that they only
apply on the specific platforms that need them.

I did a previous implementation of this workaround a couple years ago:

https://lore.kernel.org/all/20250114190242.2026743-2-matthew.d.roper@intel.com/

but we decided not to apply it because the Xe driver doesn't officially
support MTL or ARL (using force_probe to force Xe to probe the device is
only intended for driver developers, not end users).  Generally we only
include workarounds in Xe for unsupported platforms like MTL/ARL if they
also apply to one of the platforms that the driver is intended to
support, or if the workaround is trivial (e.g., one extra RTP entry).


Matt

> 
> The GT resume/reset paths already hold XE_FORCEWAKE_ALL around their
> MCR accesses, so the forcewake reference taken here is usually just a
> refcount increment; it closes the Wa_22018931422 window for the
> remaining callers (debugfs, OA, EU stall, etc.).
> 
> mcr_lock() may now sleep; add might_sleep() to document and enforce
> that contract. All current callers are in process context.
> 
> Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_mcr.c | 70 +++++++++++++++++++++++++---------
>  1 file changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> index a97b236dab7..e1e3c0688dc 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> @@ -10,6 +10,8 @@
>  
>  #include "regs/xe_gt_regs.h"
>  #include "xe_assert.h"
> +#include "xe_device.h"
> +#include "xe_force_wake.h"
>  #include "xe_gt_printk.h"
>  #include "xe_gt_topology.h"
>  #include "xe_gt_types.h"
> @@ -58,6 +60,8 @@ enum {
>  	MCR_OP_WRITE
>  };
>  
> +#define MCR_STEER_SEMAPHORE_TIMEOUT_US	100000
> +
>  static const struct xe_mmio_range xelp_l3bank_steering_table[] = {
>  	{ 0x00B100, 0x00B3FF },
>  	{},
> @@ -692,33 +696,58 @@ bool xe_gt_mcr_get_nonterminated_steering(struct xe_gt *gt,
>   * to synchronize with external clients (e.g., firmware), so a semaphore
>   * register will also need to be taken.
>   */
> -static void mcr_lock(struct xe_gt *gt) __acquires(&gt->mcr_lock)
> +static unsigned int mcr_lock(struct xe_gt *gt) __acquires(&gt->mcr_lock)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
> +	unsigned int fw_ref = 0;
>  	int ret = 0;
>  
> -	spin_lock(&gt->mcr_lock);
> +	might_sleep();
>  
>  	/*
>  	 * Starting with MTL we also need to grab a semaphore register
>  	 * to synchronize with external agents (e.g., firmware) that now
>  	 * shares the same steering control register. The semaphore is obtained
>  	 * when a read to the relevant register returns 1.
> +	 *
> +	 * The steering control and semaphore registers are inside an
> +	 * "always on" power domain with respect to RC6.  However there
> +	 * are some issues if higher-level platform sleep states are
> +	 * entering/exiting at the same time these registers are accessed.
> +	 * Grabbing GT forcewake and holding it over the entire
> +	 * lock/steer/unlock cycle ensures that those sleep states have
> +	 * fully exited before we access these registers, matching what
> +	 * i915 does in intel_gt_mcr_lock().
> +	 *
> +	 * Wa_22018931422
>  	 */
> -	if (GRAPHICS_VERx100(xe) >= 1270)
> -		ret = xe_mmio_wait32(&gt->mmio, STEER_SEMAPHORE, 0x1, 0x1, 10, NULL,
> -				     true);
> +	if (GRAPHICS_VERx100(xe) >= 1270) {
> +		fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +		if (!xe_force_wake_ref_has_domain(fw_ref, XE_FW_GT))
> +			xe_gt_err_ratelimited(gt, "failed to get MCR forcewake\n");
> +
> +		ret = xe_mmio_wait32(&gt->mmio, STEER_SEMAPHORE, 0x1, 0x1,
> +				     MCR_STEER_SEMAPHORE_TIMEOUT_US, NULL,
> +				     false);
> +	}
> +
> +	spin_lock(&gt->mcr_lock);
> +
> +	if (ret == -ETIMEDOUT)
> +		xe_gt_err_ratelimited(gt, "hardware MCR steering semaphore timed out\n");
>  
> -	xe_gt_WARN_ON_ONCE(gt, ret == -ETIMEDOUT);
> +	return fw_ref;
>  }
>  
> -static void mcr_unlock(struct xe_gt *gt) __releases(&gt->mcr_lock)
> +static void mcr_unlock(struct xe_gt *gt, unsigned int fw_ref) __releases(&gt->mcr_lock)
>  {
> +	spin_unlock(&gt->mcr_lock);
> +
>  	/* Release hardware semaphore - this is done by writing 1 to the register */
> -	if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270)
> +	if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270) {
>  		xe_mmio_write32(&gt->mmio, STEER_SEMAPHORE, 0x1);
> -
> -	spin_unlock(&gt->mcr_lock);
> +		xe_force_wake_put(gt_to_fw(gt), fw_ref);
> +	}
>  }
>  
>  /*
> @@ -807,10 +836,11 @@ u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, struct xe_reg_mcr reg_mcr)
>  						     &group, &instance);
>  
>  	if (steer) {
> -		mcr_lock(gt);
> +		unsigned int fw_ref = mcr_lock(gt);
> +
>  		val = rw_with_mcr_steering(gt, reg_mcr, MCR_OP_READ,
>  					   group, instance, 0);
> -		mcr_unlock(gt);
> +		mcr_unlock(gt, fw_ref);
>  	} else {
>  		val = xe_mmio_read32(&gt->mmio, reg);
>  	}
> @@ -832,13 +862,14 @@ u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
>  			   struct xe_reg_mcr reg_mcr,
>  			   int group, int instance)
>  {
> +	unsigned int fw_ref;
>  	u32 val;
>  
>  	xe_gt_assert(gt, !IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	mcr_lock(gt);
> +	fw_ref = mcr_lock(gt);
>  	val = rw_with_mcr_steering(gt, reg_mcr, MCR_OP_READ, group, instance, 0);
> -	mcr_unlock(gt);
> +	mcr_unlock(gt, fw_ref);
>  
>  	return val;
>  }
> @@ -857,11 +888,13 @@ u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
>  void xe_gt_mcr_unicast_write(struct xe_gt *gt, struct xe_reg_mcr reg_mcr,
>  			     u32 value, int group, int instance)
>  {
> +	unsigned int fw_ref;
> +
>  	xe_gt_assert(gt, !IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	mcr_lock(gt);
> +	fw_ref = mcr_lock(gt);
>  	rw_with_mcr_steering(gt, reg_mcr, MCR_OP_WRITE, group, instance, value);
> -	mcr_unlock(gt);
> +	mcr_unlock(gt, fw_ref);
>  }
>  
>  /**
> @@ -876,6 +909,7 @@ void xe_gt_mcr_multicast_write(struct xe_gt *gt, struct xe_reg_mcr reg_mcr,
>  			       u32 value)
>  {
>  	struct xe_reg reg = to_xe_reg(reg_mcr);
> +	unsigned int fw_ref;
>  
>  	xe_gt_assert(gt, !IS_SRIOV_VF(gt_to_xe(gt)));
>  
> @@ -884,9 +918,9 @@ void xe_gt_mcr_multicast_write(struct xe_gt *gt, struct xe_reg_mcr reg_mcr,
>  	 * access, the MULTICAST bit should already be set, so there's no need
>  	 * to touch the steering register.
>  	 */
> -	mcr_lock(gt);
> +	fw_ref = mcr_lock(gt);
>  	xe_mmio_write32(&gt->mmio, reg, value);
> -	mcr_unlock(gt);
> +	mcr_unlock(gt, fw_ref);
>  }
>  
>  void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p)
> -- 
> 2.55.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCH v1 3/4] drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths

[Matthew Brost]

On Tue, Jul 21, 2026 at 09:46:53PM -0300, Tales A. Mendonça wrote:
> Timeout-recovery paths flush the G2H worker to pick up a response that
> may have been posted by the GuC but not yet processed:
> 
>  - guc_ct_send_recv() after the 1s wait for a G2H response fails
>  - the TLB invalidation backend's .flush() hook, called by
>    xe_tlb_inval_fence_timeout() before declaring a fence timed out
> 
> However flush_work() on a work item that is neither queued nor running
> is a no-op. If the GUC2HOST interrupt for the response was lost or

GUC2HOST messages getting lost are a different issue and would be
catastrophic for a variety of reasons. I'd like to dig into that if it
occurs on a POR platform (i.e., LNL+).

> coalesced, the G2H worker was never queued: the flush does not read the
> G2H CTB and the response sits there unprocessed until an unrelated G2H
> interrupt arrives. For TLB invalidations this results in
> 
>   TLB invalidation fence timeout, seqno=N recv=N-1
> 
> with the fence force-signalled with -ETIME even though the ack may
> already be present in the CTB. Observed sporadically on ARL-H under CPU
> load, always with recv == seqno - 1 and self-recovering on the next G2H
> interrupt, which is consistent with a lost wakeup rather than a
> GuC-side failure.
> 
> Add xe_guc_ct_flush_g2h(), which queues the worker before flushing it,
> guaranteeing the flush always drains the CTB, and use it in both
> timeout paths. A spurious worker run is safe: g2h_read() returns no
> data under fast_lock, and receive_g2h() copes with runtime-PM state and
> disabled CT communication.
> 
> After this change the fence-timeout error only fires when the ack is
> genuinely absent from the CTB, making the message a reliable indicator
> of a GuC-side stall.
> 
> Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_ct.c        | 23 ++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_guc_ct.h        |  1 +
>  drivers/gpu/drm/xe/xe_guc_tlb_inval.c |  2 +-
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index fe70c0fd85c..11a05c2b8c7 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -1377,7 +1377,7 @@ static int guc_ct_send_recv(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  wait_again:
>  	ret = wait_event_timeout(ct->g2h_fence_wq, READ_ONCE(g2h_fence.done), HZ);
>  	if (!ret) {
> -		LNL_FLUSH_WORK(&ct->g2h_worker);
> +		xe_guc_ct_flush_g2h(ct);
>  		if (READ_ONCE(g2h_fence.done)) {
>  			xe_gt_warn(gt, "G2H fence %u, action %04x, done\n",
>  				   g2h_fence.seqno, action[0]);
> @@ -2046,6 +2046,27 @@ static void g2h_worker_func(struct work_struct *w)
>  	receive_g2h(ct);
>  }
>  
> +/**
> + * xe_guc_ct_flush_g2h() - Force processing of pending G2H messages
> + * @ct: GuC CT object
> + *
> + * The GUC2HOST interrupt for a G2H message may be lost or coalesced. When
> + * that happens the G2H worker is never queued and flushing it is a no-op
> + * that does not read the G2H CTB, leaving messages the GuC has already
> + * posted unprocessed until the next interrupt arrives. Queue the worker
> + * before flushing it so the flush always drains the G2H CTB. A spurious
> + * worker run is safe: it returns without side effects if the CTB is empty
> + * or CT communication is disabled.
> + */
> +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct)
> +{
> +	if (!xe_guc_ct_enabled(ct))
> +		return;
> +
> +	queue_work(ct->g2h_wq, &ct->g2h_worker);

If the original queued work gets lost or is never submitted, then there
is a bug somewhere else in the kernel, as mentioned above.

As for the flush part, yes, it's possible that this could take a while
if the scheduler goes out to lunch, but it shouldn't happen with a
sufficiently large timeout. Have you tried making the GuC CT worker high
priority? We could also consider making the G2H handlers RT once those
land upstream.

In general, I'm quite unhappy with the existing LNL_FLUSH_WORK, and we
never determined the root cause of the original issue. In retrospect, we
probably should never have merged that code. Looking at it now, the LNL
flush code isn't even scoped to LNL, which makes this even worse than I
initially thought, as it can paper over bugs. I'm going to post a patch
to scope the existing workaround to LNL and earlier platforms (or maybe
just removing it completely) and see what happens in our CI over the
next several months before it makes its way into a kernel release.

With that said, the patch as proposed is a no from me. If you want to
pursue my suggestion of scoping this change to LNL and earlier
platforms, we can discuss that separately.

Matt

> +	flush_work(&ct->g2h_worker);
> +}
> +
>  static struct xe_guc_ct_snapshot *guc_ct_snapshot_alloc(struct xe_guc_ct *ct, bool atomic,
>  							bool want_ctb)
>  {
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> index 767365a33de..4e0338953f1 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> @@ -22,6 +22,7 @@ void xe_guc_ct_runtime_suspend(struct xe_guc_ct *ct);
>  void xe_guc_ct_stop(struct xe_guc_ct *ct);
>  void xe_guc_ct_flush_and_stop(struct xe_guc_ct *ct);
>  void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
> +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct);
>  
>  struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct);
>  void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot, struct drm_printer *p);
> diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> index 046d0655122..91960dc4ba9 100644
> --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> @@ -327,7 +327,7 @@ static void tlb_inval_flush(struct xe_tlb_inval *tlb_inval)
>  {
>  	struct xe_guc *guc = tlb_inval->private;
>  
> -	LNL_FLUSH_WORK(&guc->ct.g2h_worker);
> +	xe_guc_ct_flush_g2h(&guc->ct);
>  }
>  
>  static long tlb_inval_timeout_delay(struct xe_tlb_inval *tlb_inval)
> -- 
> 2.55.0
> 

Re: [PATCH v1 3/4] drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths

[Matthew Brost]

On Wed, Jul 22, 2026 at 02:10:18PM -0700, Matthew Brost wrote:
> On Tue, Jul 21, 2026 at 09:46:53PM -0300, Tales A. Mendonça wrote:
> > Timeout-recovery paths flush the G2H worker to pick up a response that
> > may have been posted by the GuC but not yet processed:
> > 
> >  - guc_ct_send_recv() after the 1s wait for a G2H response fails
> >  - the TLB invalidation backend's .flush() hook, called by
> >    xe_tlb_inval_fence_timeout() before declaring a fence timed out
> > 
> > However flush_work() on a work item that is neither queued nor running
> > is a no-op. If the GUC2HOST interrupt for the response was lost or
> 
> GUC2HOST messages getting lost are a different issue and would be
> catastrophic for a variety of reasons. I'd like to dig into that if it
> occurs on a POR platform (i.e., LNL+).
> 
> > coalesced, the G2H worker was never queued: the flush does not read the
> > G2H CTB and the response sits there unprocessed until an unrelated G2H
> > interrupt arrives. For TLB invalidations this results in
> > 
> >   TLB invalidation fence timeout, seqno=N recv=N-1
> > 
> > with the fence force-signalled with -ETIME even though the ack may
> > already be present in the CTB. Observed sporadically on ARL-H under CPU
> > load, always with recv == seqno - 1 and self-recovering on the next G2H
> > interrupt, which is consistent with a lost wakeup rather than a
> > GuC-side failure.
> > 
> > Add xe_guc_ct_flush_g2h(), which queues the worker before flushing it,
> > guaranteeing the flush always drains the CTB, and use it in both
> > timeout paths. A spurious worker run is safe: g2h_read() returns no
> > data under fast_lock, and receive_g2h() copes with runtime-PM state and
> > disabled CT communication.
> > 
> > After this change the fence-timeout error only fires when the ack is
> > genuinely absent from the CTB, making the message a reliable indicator
> > of a GuC-side stall.
> > 
> > Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_ct.c        | 23 ++++++++++++++++++++++-
> >  drivers/gpu/drm/xe/xe_guc_ct.h        |  1 +
> >  drivers/gpu/drm/xe/xe_guc_tlb_inval.c |  2 +-
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index fe70c0fd85c..11a05c2b8c7 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -1377,7 +1377,7 @@ static int guc_ct_send_recv(struct xe_guc_ct *ct, const u32 *action, u32 len,
> >  wait_again:
> >  	ret = wait_event_timeout(ct->g2h_fence_wq, READ_ONCE(g2h_fence.done), HZ);
> >  	if (!ret) {
> > -		LNL_FLUSH_WORK(&ct->g2h_worker);
> > +		xe_guc_ct_flush_g2h(ct);
> >  		if (READ_ONCE(g2h_fence.done)) {
> >  			xe_gt_warn(gt, "G2H fence %u, action %04x, done\n",
> >  				   g2h_fence.seqno, action[0]);
> > @@ -2046,6 +2046,27 @@ static void g2h_worker_func(struct work_struct *w)
> >  	receive_g2h(ct);
> >  }
> >  
> > +/**
> > + * xe_guc_ct_flush_g2h() - Force processing of pending G2H messages
> > + * @ct: GuC CT object
> > + *
> > + * The GUC2HOST interrupt for a G2H message may be lost or coalesced. When
> > + * that happens the G2H worker is never queued and flushing it is a no-op
> > + * that does not read the G2H CTB, leaving messages the GuC has already
> > + * posted unprocessed until the next interrupt arrives. Queue the worker
> > + * before flushing it so the flush always drains the G2H CTB. A spurious
> > + * worker run is safe: it returns without side effects if the CTB is empty
> > + * or CT communication is disabled.
> > + */
> > +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct)
> > +{
> > +	if (!xe_guc_ct_enabled(ct))
> > +		return;
> > +
> > +	queue_work(ct->g2h_wq, &ct->g2h_worker);
> 
> If the original queued work gets lost or is never submitted, then there
> is a bug somewhere else in the kernel, as mentioned above.
> 
> As for the flush part, yes, it's possible that this could take a while
> if the scheduler goes out to lunch, but it shouldn't happen with a
> sufficiently large timeout. Have you tried making the GuC CT worker high
> priority? We could also consider making the G2H handlers RT once those
> land upstream.
> 
> In general, I'm quite unhappy with the existing LNL_FLUSH_WORK, and we
> never determined the root cause of the original issue. In retrospect, we
> probably should never have merged that code. Looking at it now, the LNL
> flush code isn't even scoped to LNL, which makes this even worse than I
> initially thought, as it can paper over bugs. I'm going to post a patch
> to scope the existing workaround to LNL and earlier platforms (or maybe
> just removing it completely) and see what happens in our CI over the
> next several months before it makes its way into a kernel release.
> 
> With that said, the patch as proposed is a no from me. If you want to
> pursue my suggestion of scoping this change to LNL and earlier
> platforms, we can discuss that separately.

This is roughly what I had in mind:

https://patchwork.freedesktop.org/series/170939/

Compile tested only, so if you could give it try and see what pops up, I
think that would be helpful. Fell free to use the code in anyway and
give me feedback.

Matt

> 
> Matt
> 
> > +	flush_work(&ct->g2h_worker);
> > +}
> > +
> >  static struct xe_guc_ct_snapshot *guc_ct_snapshot_alloc(struct xe_guc_ct *ct, bool atomic,
> >  							bool want_ctb)
> >  {
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> > index 767365a33de..4e0338953f1 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> > @@ -22,6 +22,7 @@ void xe_guc_ct_runtime_suspend(struct xe_guc_ct *ct);
> >  void xe_guc_ct_stop(struct xe_guc_ct *ct);
> >  void xe_guc_ct_flush_and_stop(struct xe_guc_ct *ct);
> >  void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
> > +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct);
> >  
> >  struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct);
> >  void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot, struct drm_printer *p);
> > diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > index 046d0655122..91960dc4ba9 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > @@ -327,7 +327,7 @@ static void tlb_inval_flush(struct xe_tlb_inval *tlb_inval)
> >  {
> >  	struct xe_guc *guc = tlb_inval->private;
> >  
> > -	LNL_FLUSH_WORK(&guc->ct.g2h_worker);
> > +	xe_guc_ct_flush_g2h(&guc->ct);
> >  }
> >  
> >  static long tlb_inval_timeout_delay(struct xe_tlb_inval *tlb_inval)
> > -- 
> > 2.55.0
> > 

Re: [PATCH v1 3/4] drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths

[Matthew Brost]

On Wed, Jul 22, 2026 at 08:28:54PM -0300, Tales A. Mendonça wrote:
> Hi Matt,
> 
> Thanks for the detailed look - and you're right about the framing.
> Later instrumentation on this system (sampling G2H CTB head/tail in
> the TDR *before* the flush) refuted the lost-wakeup theory from my
> commit message: in every observed timeout (30+ events across two ARL
> machines, 7d51 and 7dd1) the CTB is empty at timeout time (head ==

What about the H2G CT?

> tail, 0 pending dwords), and the ack lands 5-475ms (median ~22ms)
> later via the normal path. So the flush recovers nothing - the ack
> simply hasn't been posted yet by the GuC at that point. The stall
> appears to be firmware-side (GuC 70.53.0); I'm preparing a report with
> the full data.
> 
> That also means I agree with dropping this patch, and the data is
> consistent with your suspicion that LNL_FLUSH_WORK papers over nothing
> here.
> 
> I'll test your series from
> https://patchwork.freedesktop.org/series/170939/ on both ARL machines
> (daily-driver workloads that reproduce the timeouts at ~1.5/h) and
> report back.
> 

Another thing to look at is getting a devcoredump on a TLB invalidation
timeout. I thought we supported xe_devcoredump() without a queue or job,
but I guess we never got around to doing that. If we had the CT state
and GuC log, that would at least give us some hints about what is going
on. It would probably be worthwhile to add something like
xe_devcoredump_gt(gt) to Xe to help debug hangs that are unrelated to
queues or jobs.

That said, as Matt R. mentioned in an earlier patch, we don't officially
support ARL on Xe, so our bandwidth to help here is limited. In other
words, if you're hitting a platform-unsupported-specific bug, we may or
may not be able to investigate it.

Matt

>  Thanks,
>  Tales
> 
> Em qua., 22 de jul. de 2026 às 19:03, Matthew Brost
> <matthew.brost@intel.com> escreveu:
> >
> > On Wed, Jul 22, 2026 at 02:10:18PM -0700, Matthew Brost wrote:
> > > On Tue, Jul 21, 2026 at 09:46:53PM -0300, Tales A. Mendonça wrote:
> > > > Timeout-recovery paths flush the G2H worker to pick up a response that
> > > > may have been posted by the GuC but not yet processed:
> > > >
> > > >  - guc_ct_send_recv() after the 1s wait for a G2H response fails
> > > >  - the TLB invalidation backend's .flush() hook, called by
> > > >    xe_tlb_inval_fence_timeout() before declaring a fence timed out
> > > >
> > > > However flush_work() on a work item that is neither queued nor running
> > > > is a no-op. If the GUC2HOST interrupt for the response was lost or
> > >
> > > GUC2HOST messages getting lost are a different issue and would be
> > > catastrophic for a variety of reasons. I'd like to dig into that if it
> > > occurs on a POR platform (i.e., LNL+).
> > >
> > > > coalesced, the G2H worker was never queued: the flush does not read the
> > > > G2H CTB and the response sits there unprocessed until an unrelated G2H
> > > > interrupt arrives. For TLB invalidations this results in
> > > >
> > > >   TLB invalidation fence timeout, seqno=N recv=N-1
> > > >
> > > > with the fence force-signalled with -ETIME even though the ack may
> > > > already be present in the CTB. Observed sporadically on ARL-H under CPU
> > > > load, always with recv == seqno - 1 and self-recovering on the next G2H
> > > > interrupt, which is consistent with a lost wakeup rather than a
> > > > GuC-side failure.
> > > >
> > > > Add xe_guc_ct_flush_g2h(), which queues the worker before flushing it,
> > > > guaranteeing the flush always drains the CTB, and use it in both
> > > > timeout paths. A spurious worker run is safe: g2h_read() returns no
> > > > data under fast_lock, and receive_g2h() copes with runtime-PM state and
> > > > disabled CT communication.
> > > >
> > > > After this change the fence-timeout error only fires when the ack is
> > > > genuinely absent from the CTB, making the message a reliable indicator
> > > > of a GuC-side stall.
> > > >
> > > > Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_guc_ct.c        | 23 ++++++++++++++++++++++-
> > > >  drivers/gpu/drm/xe/xe_guc_ct.h        |  1 +
> > > >  drivers/gpu/drm/xe/xe_guc_tlb_inval.c |  2 +-
> > > >  3 files changed, 24 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > index fe70c0fd85c..11a05c2b8c7 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > @@ -1377,7 +1377,7 @@ static int guc_ct_send_recv(struct xe_guc_ct *ct, const u32 *action, u32 len,
> > > >  wait_again:
> > > >     ret = wait_event_timeout(ct->g2h_fence_wq, READ_ONCE(g2h_fence.done), HZ);
> > > >     if (!ret) {
> > > > -           LNL_FLUSH_WORK(&ct->g2h_worker);
> > > > +           xe_guc_ct_flush_g2h(ct);
> > > >             if (READ_ONCE(g2h_fence.done)) {
> > > >                     xe_gt_warn(gt, "G2H fence %u, action %04x, done\n",
> > > >                                g2h_fence.seqno, action[0]);
> > > > @@ -2046,6 +2046,27 @@ static void g2h_worker_func(struct work_struct *w)
> > > >     receive_g2h(ct);
> > > >  }
> > > >
> > > > +/**
> > > > + * xe_guc_ct_flush_g2h() - Force processing of pending G2H messages
> > > > + * @ct: GuC CT object
> > > > + *
> > > > + * The GUC2HOST interrupt for a G2H message may be lost or coalesced. When
> > > > + * that happens the G2H worker is never queued and flushing it is a no-op
> > > > + * that does not read the G2H CTB, leaving messages the GuC has already
> > > > + * posted unprocessed until the next interrupt arrives. Queue the worker
> > > > + * before flushing it so the flush always drains the G2H CTB. A spurious
> > > > + * worker run is safe: it returns without side effects if the CTB is empty
> > > > + * or CT communication is disabled.
> > > > + */
> > > > +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct)
> > > > +{
> > > > +   if (!xe_guc_ct_enabled(ct))
> > > > +           return;
> > > > +
> > > > +   queue_work(ct->g2h_wq, &ct->g2h_worker);
> > >
> > > If the original queued work gets lost or is never submitted, then there
> > > is a bug somewhere else in the kernel, as mentioned above.
> > >
> > > As for the flush part, yes, it's possible that this could take a while
> > > if the scheduler goes out to lunch, but it shouldn't happen with a
> > > sufficiently large timeout. Have you tried making the GuC CT worker high
> > > priority? We could also consider making the G2H handlers RT once those
> > > land upstream.
> > >
> > > In general, I'm quite unhappy with the existing LNL_FLUSH_WORK, and we
> > > never determined the root cause of the original issue. In retrospect, we
> > > probably should never have merged that code. Looking at it now, the LNL
> > > flush code isn't even scoped to LNL, which makes this even worse than I
> > > initially thought, as it can paper over bugs. I'm going to post a patch
> > > to scope the existing workaround to LNL and earlier platforms (or maybe
> > > just removing it completely) and see what happens in our CI over the
> > > next several months before it makes its way into a kernel release.
> > >
> > > With that said, the patch as proposed is a no from me. If you want to
> > > pursue my suggestion of scoping this change to LNL and earlier
> > > platforms, we can discuss that separately.
> >
> > This is roughly what I had in mind:
> >
> > https://patchwork.freedesktop.org/series/170939/
> >
> > Compile tested only, so if you could give it try and see what pops up, I
> > think that would be helpful. Fell free to use the code in anyway and
> > give me feedback.
> >
> > Matt
> >
> > >
> > > Matt
> > >
> > > > +   flush_work(&ct->g2h_worker);
> > > > +}
> > > > +
> > > >  static struct xe_guc_ct_snapshot *guc_ct_snapshot_alloc(struct xe_guc_ct *ct, bool atomic,
> > > >                                                     bool want_ctb)
> > > >  {
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> > > > index 767365a33de..4e0338953f1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> > > > @@ -22,6 +22,7 @@ void xe_guc_ct_runtime_suspend(struct xe_guc_ct *ct);
> > > >  void xe_guc_ct_stop(struct xe_guc_ct *ct);
> > > >  void xe_guc_ct_flush_and_stop(struct xe_guc_ct *ct);
> > > >  void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
> > > > +void xe_guc_ct_flush_g2h(struct xe_guc_ct *ct);
> > > >
> > > >  struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct);
> > > >  void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot, struct drm_printer *p);
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > index 046d0655122..91960dc4ba9 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
> > > > @@ -327,7 +327,7 @@ static void tlb_inval_flush(struct xe_tlb_inval *tlb_inval)
> > > >  {
> > > >     struct xe_guc *guc = tlb_inval->private;
> > > >
> > > > -   LNL_FLUSH_WORK(&guc->ct.g2h_worker);
> > > > +   xe_guc_ct_flush_g2h(&guc->ct);
> > > >  }
> > > >
> > > >  static long tlb_inval_timeout_delay(struct xe_tlb_inval *tlb_inval)
> > > > --
> > > > 2.55.0
> > > >
> 
> 
> 
> -- 
> Com os cumprimentos,
> 
> Tales A. Mendonça
> talesam.org
> communitybig.org