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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 21 Aug 2026 15:23:42 -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.
>
> The reports that user reads will always lag the HW tail by 32 reports,
> however since OA is used for post processing analysis, this should not
> affect any current use cases. When the stream is closed or disabled, we
> let the user read the remaining reports up until HW tail.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
> v2: Fix the LAG logic by using sliding window (Sashiko)
> v3: Fix checkpatch warning
> v4: Update sw tail on stream disable (Ashutosh)
> v5: Ensure read is not blocked forever (Sashiko)
> ---
>  drivers/gpu/drm/xe/xe_oa.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index d7647f37ab02..c8f2d7de1396 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,23 @@ 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 (!stream->enabled)
> +		stream->oa_buffer.tail = hw_tail;

I was originally thinking we would do this in the disable code path
(xe_oa_disable_locked etc.), but who knows where hw_tail is going to be
(whether it will be multiple of report_size etc.), so instead of
replicating that code, it seems good to do it here and call
xe_oa_buffer_check_unlocked() from xe_oa_disable_locked() as is being done
below.

Also, because xe_oa_buffer_check_unlocked() is called from the read() code paths,
I considered the following situations:

* xe_oa_buffer_check_unlocked() called multiple times after stream disable
* xe_oa_buffer_check_unlocked() called before enabling the stream

And all this seems fine to me. We would just be setting
'stream->oa_buffer.tail' multiple times unnecessarily, but looks like we
will always set it to the correct value. So all seems ok.

> +	else 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;
> @@ -1416,10 +1422,6 @@ static void xe_oa_stream_disable(struct xe_oa_stream *stream)
>
>	if (stream->sample)
>		hrtimer_cancel(&stream->poll_check_timer);
> -
> -	/* Update stream->oa_buffer.tail to allow any final reports to be read */
> -	if (xe_oa_buffer_check_unlocked(stream))
> -		wake_up(&stream->poll_wq);
>  }
>
>  static int xe_oa_enable_preempt_timeslice(struct xe_oa_stream *stream)
> @@ -1490,6 +1492,11 @@ static int xe_oa_disable_locked(struct xe_oa_stream *stream)
>		ret = xe_oa_enable_preempt_timeslice(stream);
>
>	stream->enabled = false;
> +
> +	/* Update stream->oa_buffer.tail to allow any final reports to be read */
> +	if (xe_oa_buffer_check_unlocked(stream))
> +		wake_up(&stream->poll_wq);
> +
>	return ret;
>  }

Just a few more things to consider and maybe add to this or a new patch:

1. Address this saskhiko comment:

   [High] The newly introduced 32-report lag reduces the maximum possible
   available reports, but `wait_num_reports` validation still allows values
   up to full buffer capacity, causing a permanent hang and `-EIO` on
   overflow.

   Looks like wait_num_reports check here

	if (param.wait_num_reports > param.oa_buffer_size / f->size)

   should be reduced by LAG_REPORTS (maybe make LAG_REPORTS a xe_oa_stream
   property if needed?)

2. Should we do this:

   if (!stream->enabled)
	stream->pollin = available;
   else
	stream->pollin = available >= stream->wait_num_reports * report_size;

   So after stream is disabled, pollin is set even if less than
   wait_num_reports are available (pre-existing issue I think), to unblock
   and enable reading all remaining reports after stream disable?

3. Does the OA buffer now need to be zero'd out in xe_oa_init_oa_buffer(),
   at least we need to remove the comment there:

        /* Zero out the OA buffer since we rely on zero report id and timestamp fields */
        xe_map_memset(stream->oa->xe, &stream->oa_buffer.bo->vmap, 0, 0,
                      xe_bo_size(stream->oa_buffer.bo));

   If we remove the zero'ing out, not sure if any IGT's will need to
   change. Also zero'ing out might have to be re-introduced in the future
   when reading OA registers is removed.

Thanks.
--
Ashutosh