[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

[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

[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

[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

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

[sashiko-bot]

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

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

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

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

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

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

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

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

Or in:
gt_init_with_gt_forcewake()
  -> xe_gt_enable_host_l2_vram()

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

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

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

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

[sashiko-bot]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[sashiko-bot]

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

✗ 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)

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

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