Re: [PATCH 3/3] drm/xe/xe_oa: Add a lag to the reports that is exported to user

On Fri, Aug 14, 2026 at 08:42:31AM -0700, Dixit, Ashutosh wrote:
>On Thu, 06 Aug 2026 15:47:06 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> When running heavy workloads, reading the OA reports too soon does not
>> guarantee that the report has landed in memory. To make sure correct
>> reports are copied to user buffer, only return reports that lag the
>> current HW_TAIL register by 32 reports. This is an empirical number
>> based on a heavy render workload and several test iterations.
>
>Looks good overall, but I want to discuss a couple of further points:
>
>1. I know you are doing some verification of OA data in IGT, but I am
>   thinking it would be good to have some verification of OA data (of the
>   sort that is removed in Patch 2) also in the kernel. So e.g. after a
>   report is read, we could set the timestamp field in the report to
>   0. Then when we advance the SW tail pointer, we could check if the
>   timestamp fields for new reports are non-zero. If we see a 0 timestamp
>   value, this would mean that the 32 report delay is not sufficient (for
>   who knows what will happen in future platforms). An error in dmesg if we
>   see a 0 timestamp should suffise.

I am hesitant to write to the OA buffer at all, especially due to the 
different coherence behavior between discrete and integrated. We should 
just address it as an issue/bug at that point. Let me think about it a 
bit and see what we can do if we see an error in future.

>
>   I understand that, because cachelines are landing out of order, a
>   non-zero timestamp value doesn't absolutely guarantee that all data is
>   correct. But I am thinking statistially we should see 0 timestamp
>   values, once in a while, if the 32 report delay were insufficient. So at
>   least we'll have some indication from the kernel if that were to happen.


>
>2. The second point is about "what happens in the end", the UMD never sees
>   the last 32 reports? Maybe we could do the following to address this:
>   when OA stream is disabled, we advance the SW tail pointer to the HW
>   tail (overriding the 32 report delay). Then when UMD reads the last bit
>   of data, after disabling the stream, they will get all data. Though all
>   cachelines might still not have landed, but at least we will have
>   advanced the SW tail pointer.

oh, I missed that part. Yeah, if the stream is disabled, I would need to 
drain the data. I think that would just add a delay for 32 reports, 
based on the timer period before updating the tail to the latest.

Thanks,
Umesh

>
>Thoughts?
>
>Thanks.
>--
>Ashutosh
>
>
>>
>> Signed-off-by: Umesh Nerlige Ramappa <[email protected]>
>> ---
>> v2: Fix the LAG logic by using sliding window (Sashiko)
>> v3: Fix checkpatch warning
>> ---
>>  drivers/gpu/drm/xe/xe_oa.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>> index 5952010e8f51..dc8b402d059a 100644
>> --- a/drivers/gpu/drm/xe/xe_oa.c
>> +++ b/drivers/gpu/drm/xe/xe_oa.c
>> @@ -224,7 +224,7 @@ static bool mert_wa_14026633728(struct xe_oa_stream *s)
>>  static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
>>  {
>>	u32 gtt_offset = xe_bo_ggtt_addr(stream->oa_buffer.bo);
>> -	u32 hw_tail, partial_report_size, available;
>> +	u32 hw_tail, partial_report_size, available, lag;
>>	int report_size = stream->oa_buffer.format->size;
>>	unsigned long flags;
>>
>> @@ -234,17 +234,21 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
>>	hw_tail -= gtt_offset;
>>
>>	/*
>> -	 * The tail pointer increases in 64 byte (cacheline size), not in report_size
>> +	 * The hw_tail pointer increases in 64 byte (cacheline size), not in report_size
>>	 * increments. Also report size may not be a power of 2. Compute potential
>>	 * partially landed report in OA buffer.
>>	 */
>>	partial_report_size = xe_oa_circ_diff(stream, hw_tail, stream->oa_buffer.tail);
>>	partial_report_size %= report_size;
>>
>> -	/* Subtract partial amount off the tail */
>> +	/* Subtract partial amount off the hw_tail */
>>	hw_tail = xe_oa_circ_diff(stream, hw_tail, partial_report_size);
>>
>> -	stream->oa_buffer.tail = hw_tail;
>> +#define LAG_REPORTS 32
>> +	lag = xe_oa_circ_diff(stream, hw_tail, stream->oa_buffer.tail);
>> +	if (lag > LAG_REPORTS * report_size)
>> +		stream->oa_buffer.tail = xe_oa_circ_diff(stream, hw_tail,
>> +							 LAG_REPORTS * report_size);
>>
>>	available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head);
>>	stream->pollin = available >= stream->wait_num_reports * report_size;
>> --
>> 2.51.0
>>