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

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

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
>