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

On Mon, Aug 03, 2026 at 02:25:47PM -0700, Umesh Nerlige Ramappa wrote:
>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;
>	}

&map when passing to iosys_map_rd

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