Re: [PATCH v9 2/3] drm/xe/lrc: Fix torn read of CTX_TIMESTAMP from LRC

On Fri, Jul 31, 2026 at 04:00:07AM +0000, Gajendra Uttamchand wrote:
>xe_lrc_ctx_timestamp() reads the 64-bit CTX_TIMESTAMP value stored in
>the LRC as two independent 32-bit reads (ldw, then udw). The GPU can
>concurrently overwrite this location as part of a context-save, so a
>plain back-to-back read of the two dwords can observe a torn
>combination that matches neither the previous value (e.g. the
>CONTEXT_ACTIVE sentinel written on context-restore) nor the final
>saved timestamp written by hardware. Callers such as context_active()
>and xe_lrc_context_timestamp() rely on this value being coherent to
>decide whether the context is still active, so a torn read can lead to
>an incorrect/garbage timestamp being sampled or reported.
>
>Make xe_lrc_ctx_timestamp() robust against torn reads by reading the
>upper 32-bit half first and re-reading the lower/upper pair until the
>upper half stabilizes across two consecutive reads (bounded to a small
>retry limit), then combine them into the final 64-bit value.
>
>This fixes the problem at the source so all callers get a coherent
>timestamp, instead of patching individual callers.
>
>Assisted-by: GitHub-Copilot:claude-sonnet-5
>Signed-off-by: Gajendra Uttamchand <[email protected]>
>---
> drivers/gpu/drm/xe/xe_lrc.c | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>index 78d0456cf3c9..6f0247bdf8e4 100644
>--- a/drivers/gpu/drm/xe/xe_lrc.c
>+++ b/drivers/gpu/drm/xe/xe_lrc.c
>@@ -874,14 +874,33 @@ static u64 xe_lrc_ctx_timestamp(struct xe_lrc *lrc)
> {
> 	struct xe_device *xe = lrc_to_xe(lrc);
> 	struct iosys_map map;
>-	u32 ldw, udw = 0;
>+	u32 ldw, udw = 0, old_ldw, old_udw;
>+	int retries;
>
>-	map = __xe_lrc_ctx_timestamp_map(lrc);
>-	ldw = xe_map_read32(xe, &map);
>+	if (!xe->info.has_64bit_timestamp) {
>+		map = __xe_lrc_ctx_timestamp_map(lrc);
>+		return xe_map_read32(xe, &map);
>+	}
>+
>+	for (retries = 5; retries; --retries) {
>+		map = __xe_lrc_ctx_timestamp_udw_map(lrc);
>+		old_udw = xe_map_read32(xe, &map);
>+
>+		map = __xe_lrc_ctx_timestamp_map(lrc);

The map can move out of the loop. You could define a new map_udw for the 
udw value.

>+		old_ldw = xe_map_read32(xe, &map);
>+
>+		smp_rmb();
>
>-	if (xe->info.has_64bit_timestamp) {
> 		map = __xe_lrc_ctx_timestamp_udw_map(lrc);
> 		udw = xe_map_read32(xe, &map);
>+
>+		map = __xe_lrc_ctx_timestamp_map(lrc);
>+		ldw = xe_map_read32(xe, &map);
>+
>+		if (udw == old_udw && ldw == old_ldw)
>+			return (u64)udw << 32 | ldw;
>+
>+		cpu_relax();

cpu_relax() is not needed here and can be dropped.

After looking into this a bit more, I think it is best to use the 
iosys_map helpers instead of the xe_map_read32. Also note that xe_map_* 
helpers may be removed in future. The only value add that xe_map helpers 
provide is the pm_runtime check. I have added it below.

So this should look something like:

static u64 xe_lrc_ctx_timestamp(struct xe_lrc *lrc)
{
	struct xe_device *xe = lrc_to_xe(lrc);
	struct iosys_map map;
	int retries;
	u64 val;

	xe_assert(xe, !xe_pm_runtime_suspended(xe));

	map = __xe_lrc_ctx_timestamp_map(lrc);
	if (!xe->info.has_64bit_timestamp)
		return iosys_map_rd(map, 0, u32);

	for (retries = 5; retries; --retries) {
		val = iosys_map_rd(map, 0, u64);
		if (iosys_map_rd(map, 0, u64) == val)
			break;
	}

	return val;
}

The barrier is also not required (or is handled by the iosys_map helpers 
internally when needed).

Thanks,
Umesh

> 	}
>
> 	return (u64)udw << 32 | ldw;
>--
>2.43.0
>