[PATCH v1 0/4] drm/xe: MCR semaphore and TLB invalidation timeout fixes for ARL

[PATCH v1 0/4] drm/xe: MCR semaphore and TLB invalidation timeout fixes for ARL

[Tales A. Mendonça]

This series addresses two issues observed on Arrow Lake laptops (two
ASUS Vivobook S 14 S3407CA units, Core Ultra 5 225H, iGPU 7d51/ARL-H
and 7dd1/ARL-P, GuC firmware 70.53.0), reproduced and bisected over
several weeks of daily use with instrumented kernels.

1) MCR steering semaphore held across resume (patches 1-2)

On s2idle resume the first MCR access finds STEER_SEMAPHORE still held
by firmware. The current code waits only 10us while spinning inside
the mcr_lock spinlock and then fires a WARN with a full backtrace:

  drm_WARN_ON_ONCE(ret == -110)
  WARNING: drivers/gpu/drm/xe/xe_gt_mcr.c:697 mcr_lock

Patch 1 ports the i915 MCR locking model: wait for the hardware
semaphore (up to 100ms, sleeping) before taking the software spinlock,
hold GT forcewake over the lock/steer/unlock cycle (Wa_22018931422),
and demote the WARN to a rate-limited GT error. Patch 2 ports
intel_gt_mcr_lock_sanitize() from i915, releasing an orphaned
semaphore at GT resume before the first MCR access.

Testing: before the fix the semaphore WARN fired on nearly every
s2idle resume. With patches 1-2, zero occurrences across dozens of
s2idle cycles (including several of 10+ hours) on both machines.

2) TLB invalidation fence timeouts / GuC ack latency (patches 3-4)

Under CPU load, TLB invalidation fences intermittently time out:

  TLB invalidation fence timeout, seqno=N recv=N-1

Instrumented kernels (logging the GT C-state at request and timeout
time, the G2H CTB occupancy at timeout time, and the ack arrival time)
show, across 26+ events on both machines:

  - the ack is not sitting unprocessed in the G2H CTB at timeout time
    (CTB head == tail, zero pending dwords), and
  - the GT C-state does not matter (events observed both with the GT
    parked in C6 the whole wait and, with forcewake artificially held,
    in C0 the whole wait), and
  - the ack arrives 5-475ms (median ~22ms) after the timeout handler
    runs, i.e. right after the handler's forced G2H drain and register
    traffic.

This looks like a GuC-side stall posting the ack (a separate report
with the full data is being prepared), but the host side can still be
made more robust: patch 3 fixes the timeout-recovery paths, which
flush a G2H worker that was possibly never queued (flush_work() on an
idle work item is a no-op, so a lost/coalesced GUC2HOST interrupt
turns the flush into a no-op too). Queueing the worker before flushing
guarantees the CTB is actually drained, and with it the fence-timeout
message becomes a reliable indicator of a genuine GuC-side stall.

Patch 4 (RFC) raises the driver-internal TLB invalidation deadline
from HZ/4 to HZ/2. All observed request-to-ack latencies cluster just
above the current 250ms deadline (2.5s only because the fence TDR
rounds up); HZ/2 keeps the WARN meaningful while not firing on
latencies the hardware demonstrably recovers from. This one is a
band-aid for the firmware behaviour above, hence RFC — happy to drop
it if the GuC stall gets fixed at the source.

Series based on drm-tip, compile-tested there; runtime testing was
done on 7.1.3 plus backports of these patches (kernel package
"linux71-comm" of the BigCommunity distribution, in daily use on both
machines).

Tales A. Mendonça (4):
  drm/xe/mcr: Keep GT forcewake during MCR steering
  drm/xe/mcr: Sanitize steering semaphore on GT resume
  drm/xe/guc/ct: Queue G2H worker before flushing it in timeout paths
  drm/xe: Raise hw_tlb_timeout to cover observed GuC ack latency

 drivers/gpu/drm/xe/xe_gt.c            |  7 +++
 drivers/gpu/drm/xe/xe_gt_mcr.c        | 90 ++++++++++++++++++++++-----
 drivers/gpu/drm/xe/xe_gt_mcr.h        |  1 +
 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 |  4 +-
 6 files changed, 106 insertions(+), 20 deletions(-)

-- 
2.55.0

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

[Tales A. Mendonça]

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.

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

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

[Tales A. Mendonça]

Thanks for the review.

  Agreed - the capture list locking concern is in xe_guc_capture.c and
pre-dates this patch, which only touches the MCR steering lock path.
  It seems worth tracking separately; I can file a report for it.

Em ter., 21 de jul. de 2026 às 21:47, Tales A. Mendonça
<talesam@gmail.com> escreveu:
>
> 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.
>
> 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
>


-- 
Com os cumprimentos,

Tales A. Mendonça
talesam.org
communitybig.org

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 1/4] drm/xe/mcr: Keep GT forcewake during MCR steering

[Tales A. Mendonça]

Hi Matt,

Thanks a lot for the context - and good to know this workaround had
already been prototyped on your side; that link is very helpful.

Understood regarding the platform policy (this is indeed a force_probe
setup on ARL).

Two questions to shape a possible v2:

1) Part of this patch is not the workaround itself: waiting for the
semaphore with a sleeping wait *before* taking the software spinlock
(instead of the 10us atomic spin inside it), and demoting the WARN
splat to a rate-limited GT error, both following i915. Would those be
acceptable on their own, with the forcewake hold (the actual
Wa_22018931422 part) dropped, or moved to the WA infrastructure gated
to the affected platforms?

2) If not, should I just drop this patch and keep 2-4? Patch 2 mirrors
i915's trivial resume-time sanitize, and 3-4 are platform-independent
CT/timeout robustness.

Happy to rework either way.

Thanks,
Tales

Em qua., 22 de jul. de 2026 às 15:10, Matt Roper
<matthew.d.roper@intel.com> escreveu:
>
> 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



-- 
Com os cumprimentos,

Tales A. Mendonça
talesam.org
communitybig.org

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

[Tales A. Mendonça]

The MCR steering semaphore can be left in a held state after a suspend
cycle. The first MCR access after resume then stalls until the
semaphore wait times out and logs:

  Tile0: GT1: hardware MCR steering semaphore timed out

as observed on an Arrow Lake-P system resuming from s2idle. Recovery
currently only happens because mcr_unlock() releases the semaphore
unconditionally.

i915 addresses the same issue in intel_gt_resume_early() via
intel_gt_mcr_lock_sanitize(), noting that the hardware team confirmed
that forcibly releasing the semaphore is safe during driver
load/resume because no other agents acquire it at that time.

Port that to xe: release the steering semaphore at the start of
xe_gt_resume(), before the first MCR access performed by
do_gt_restart().

Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
---
 drivers/gpu/drm/xe/xe_gt.c     |  7 +++++++
 drivers/gpu/drm/xe/xe_gt_mcr.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_gt_mcr.h |  1 +
 3 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
index dfdacc0f6de..c61f3d9fc9a 100644
--- a/drivers/gpu/drm/xe/xe_gt.c
+++ b/drivers/gpu/drm/xe/xe_gt.c
@@ -1068,6 +1068,13 @@ int xe_gt_resume(struct xe_gt *gt)
 		return -ETIMEDOUT;
 	}
 
+	/*
+	 * The MCR steering semaphore may be left held across a suspend
+	 * cycle; release it before the first MCR access below so that
+	 * mcr_lock() does not stall until the semaphore wait times out.
+	 */
+	xe_gt_mcr_lock_sanitize(gt);
+
 	err = do_gt_restart(gt);
 	if (err)
 		return err;
diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
index e1e3c0688dc..3934d0de717 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
+ * during the load/resume process, mirroring i915's
+ * intel_gt_mcr_lock_sanitize().
+ */
+void xe_gt_mcr_lock_sanitize(struct xe_gt *gt)
+{
+	if (IS_SRIOV_VF(gt_to_xe(gt)))
+		return;
+
+	lockdep_assert_not_held(&gt->mcr_lock);
+
+	if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270)
+		xe_mmio_write32(&gt->mmio, STEER_SEMAPHORE, 0x1);
+}
+
 /*
  * Access a register with specific MCR steering
  *
diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
index 75374662f10..162f54075cf 100644
--- a/drivers/gpu/drm/xe/xe_gt_mcr.h
+++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
@@ -16,6 +16,7 @@ void xe_gt_mcr_init_early(struct xe_gt *gt);
 void xe_gt_mcr_init(struct xe_gt *gt);
 
 void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
+void xe_gt_mcr_lock_sanitize(struct xe_gt *gt);
 
 u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
 			   int group, int instance);
-- 
2.55.0

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

[Tales A. Mendonça]

Thanks for the review.

The call site mirrors i915, where intel_gt_mcr_lock_sanitize() is also
only called on the resume path (intel_gt_resume_early(), its only
caller). The "driver load/resume" wording comes from i915's hardware
team note about when the forced release is *safe*, not about where it
is required.

Empirically the failure mode is resume-only: across dozens of cold
boots on two ARL machines the probe-time stall never reproduced (boot
firmware hands the semaphore over released; it is the suspend cycle
that leaves it held). That said, dding a sanitize call early in the
load path would be harmless per the same safety note - happy to do
that in a v2 if preferred.

Em ter., 21 de jul. de 2026 às 21:47, Tales A. Mendonça
<talesam@gmail.com> escreveu:
>
> The MCR steering semaphore can be left in a held state after a suspend
> cycle. The first MCR access after resume then stalls until the
> semaphore wait times out and logs:
>
>   Tile0: GT1: hardware MCR steering semaphore timed out
>
> as observed on an Arrow Lake-P system resuming from s2idle. Recovery
> currently only happens because mcr_unlock() releases the semaphore
> unconditionally.
>
> i915 addresses the same issue in intel_gt_resume_early() via
> intel_gt_mcr_lock_sanitize(), noting that the hardware team confirmed
> that forcibly releasing the semaphore is safe during driver
> load/resume because no other agents acquire it at that time.
>
> Port that to xe: release the steering semaphore at the start of
> xe_gt_resume(), before the first MCR access performed by
> do_gt_restart().
>
> Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c     |  7 +++++++
>  drivers/gpu/drm/xe/xe_gt_mcr.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_mcr.h |  1 +
>  3 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index dfdacc0f6de..c61f3d9fc9a 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -1068,6 +1068,13 @@ int xe_gt_resume(struct xe_gt *gt)
>                 return -ETIMEDOUT;
>         }
>
> +       /*
> +        * The MCR steering semaphore may be left held across a suspend
> +        * cycle; release it before the first MCR access below so that
> +        * mcr_lock() does not stall until the semaphore wait times out.
> +        */
> +       xe_gt_mcr_lock_sanitize(gt);
> +
>         err = do_gt_restart(gt);
>         if (err)
>                 return err;
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> index e1e3c0688dc..3934d0de717 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
> + * during the load/resume process, mirroring i915's
> + * intel_gt_mcr_lock_sanitize().
> + */
> +void xe_gt_mcr_lock_sanitize(struct xe_gt *gt)
> +{
> +       if (IS_SRIOV_VF(gt_to_xe(gt)))
> +               return;
> +
> +       lockdep_assert_not_held(&gt->mcr_lock);
> +
> +       if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270)
> +               xe_mmio_write32(&gt->mmio, STEER_SEMAPHORE, 0x1);
> +}
> +
>  /*
>   * Access a register with specific MCR steering
>   *
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> index 75374662f10..162f54075cf 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> @@ -16,6 +16,7 @@ void xe_gt_mcr_init_early(struct xe_gt *gt);
>  void xe_gt_mcr_init(struct xe_gt *gt);
>
>  void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
> +void xe_gt_mcr_lock_sanitize(struct xe_gt *gt);
>
>  u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
>                            int group, int instance);
> --
> 2.55.0
>


-- 
Com os cumprimentos,

Tales A. Mendonça
talesam.org
communitybig.org

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

[Tales A. Mendonça]

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

[Tales A. Mendonça]

Thanks for the review.

The check-then-queue pattern here is the same long-standing one in
xe_guc_ct_irq_handler() (xe_guc_ct.h), which also does an unlocked
xe_guc_ct_enabled() check followed by queue_work(ct->g2h_wq, ...), so
this patch does not introduce a new lifetime requirement - it relies
on the same guarantees the IRQ path already does.

Those guarantees hold for both callers: xe_tlb_inval_fence_timeout()
cannot be in flight during the drmm teardown that destroys g2h_wq,
because xe_tlb_inval_reset() cancels fence_tdr and force-signals all
pending fences with the backend already disabled before that point
(and with no pending fences the TDR is never re-armed); and
guc_ct_send_recv() is synchronous within an active CT session, so it
cannot overlap CT fini either. Note the LNL_FLUSH_WORK/flush_work
calls being replaced already required the work item to outlive the
caller in the same way.

If additional hardening is still desirable, the check+queue could be
done under ct->fast_lock, but that would equally apply to the existing
IRQ handler path.

Em ter., 21 de jul. de 2026 às 21:47, Tales A. Mendonça
<talesam@gmail.com> escreveu:
>
> 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
> 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);
> +       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

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

[Tales A. Mendonça]

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

 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

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

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

[Tales A. Mendonça]

Hi Matt,

> What about the H2G CT?

Good question - I hadn't sampled it. I've just extended the
instrumentation to record H2G head/tail (both from the shared
descriptor, so it reflects the GuC's own consumption progress) at
timeout time, alongside the existing G2H sampling. That should tell us
whether the GuC hasn't even consumed the invalidation request or has
consumed it and is sitting on the ack. I'll have data from the next
round of testing, together with your series.

> Another thing to look at is getting a devcoredump on a TLB invalidation timeout. [...] 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 sounds genuinely useful beyond this bug (and
platform-independent). I'd be happy to take a stab at
xe_devcoredump_gt() and hook it to the TLB invalidation timeout path,
if you wouldn't mind reviewing the result.

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

Completely understood - I appreciate the time you've both already put
in. My aim here is mostly to make sure the data lands where it's
useful: for the LNL_FLUSH_* scoping decision (which does affect
supported platforms) and, if the GuC stall is firmware-side, a report
the firmware team can act on whenever ARL support materializes.

Thanks,
Tales

Em qua., 22 de jul. de 2026 às 21:26, Matthew Brost
<matthew.brost@intel.com> escreveu:
>
> 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



-- 
Com os cumprimentos,

Tales A. Mendonça
talesam.org
communitybig.org

[PATCH v1 4/4] drm/xe: Raise hw_tlb_timeout to cover observed GuC ack latency

[Tales A. Mendonça]

On ARL-H (Arrow Lake-H) under sustained load, TLB invalidation acks can
arrive slightly later than the current fence timeout budget:

  tlb_inval_timeout_delay() = hw_tlb_timeout (HZ/4)
                            + 2 * xe_guc_ct_queue_proc_time_jiffies()

which with the 4K H2G buffer works out to 2.25s. When that happens the
fence is force-signalled with -ETIME even though the invalidation
completes an instant later:

  xe 0000:00:02.0: [drm] *ERROR* TLB invalidation fence timeout,
      seqno=15519 recv=15518

Local instrumentation that records the request and timeout times of a
timed-out invalidation and logs the deltas when its ack finally lands
measured, over ~24h of heavy use (kernel builds, 3 VMs, browser; ~311k
invalidations at ~7.4/s):

  - 34 fence timeouts, every one followed by its ack
  - request-to-ack: 2254-2355 ms (mean 2306 ms)
  - timeout-to-ack: 2-59 ms (mean 25 ms)

i.e. the ack always arrives, and always within tens of milliseconds of
the deadline - there is no firmware stall (nothing arrives seconds
late). A prior change guaranteeing a G2H CTB drain in the timeout path
confirmed the acks are genuinely not yet delivered when the timeout
fires, ruling out lost interrupts or an unprocessed CTB entry.

Signalling -ETIME on an invalidation that completes ~25 ms later is
worse than waiting a little longer: waiters see a spurious error for an
operation that succeeds. Raise hw_tlb_timeout to HZ/2, extending the
budget to 2.5s, which covers the observed worst case (2.355s) with
~150 ms of margin and eliminated all spurious timeouts in testing on
this workload.

Signed-off-by: Tales A. Mendonça <talesam@gmail.com>
---
 drivers/gpu/drm/xe/xe_guc_tlb_inval.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
index 91960dc4ba9..c5ceac5ab86 100644
--- a/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
+++ b/drivers/gpu/drm/xe/xe_guc_tlb_inval.c
@@ -335,7 +335,7 @@ static long tlb_inval_timeout_delay(struct xe_tlb_inval *tlb_inval)
 	struct xe_guc *guc = tlb_inval->private;
 
 	/* this reflects what HW/GuC needs to process TLB inv request */
-	const long hw_tlb_timeout = HZ / 4;
+	const long hw_tlb_timeout = HZ / 2;
 
 	/* this estimates actual delay caused by the CTB transport */
 	long delay = xe_guc_ct_queue_proc_time_jiffies(&guc->ct);
-- 
2.55.0

✗ LGCI.VerificationFailed: failure for drm/xe: MCR semaphore and TLB invalidation timeout fixes for ARL

[Patchwork]

== Series Details ==

Series: drm/xe: MCR semaphore and TLB invalidation timeout fixes for ARL
URL   : https://patchwork.freedesktop.org/series/170916/
State : failure

== Summary ==

Series author address 'talesam@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://intel-gfx-ci-public.igk.intel.com:8080/job/xe_pw_trigger/1223168/ (on master)