Re: [PATCH v5 5/7] drm/xe/oa: Wait for in-flight OA batches before stream HW teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/xe/oa: context LRI batch fence in xe_oa_load_with_lri() is
  never drained
- [High] drm/xe/oa: post-teardown HW programming when
  xe_oa_stream_drain() times out
--

commit a42a6ce61696e1b256881421425e340fba9f094d
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/oa: Wait for in-flight OA batches before stream HW teardown

This commit records the LRI batch fence in stream->last_fence and adds
xe_oa_stream_drain() to wait for stream->last_fence before tearing down
OA hardware state on stream destruction.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 6383a2e036ab0..d2bd9622109af 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -733,6 +733,12 @@ static void xe_oa_free_configs(struct xe_oa_stream *stream)
>  	dma_fence_put(stream->last_fence);
>  }
>  
> +static void xe_oa_update_last_fence(struct xe_oa_stream *stream, struct dma_fence *fence)
> +{
> +	dma_fence_put(stream->last_fence);
> +	stream->last_fence = dma_fence_get(fence);
> +}
> +
>  static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *reg_lri, u32 count)
>  {
>  	struct dma_fence *fence;
> @@ -753,6 +759,7 @@ static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *re
>  		goto free_bb;
>  	}
>  	xe_bb_free(bb, fence);
> +	xe_oa_update_last_fence(stream, fence);
>  	dma_fence_put(fence);
>  
>  	return 0;
> @@ -876,6 +883,23 @@ static void xe_oa_disable_metric_set(struct xe_oa_stream *stream)
>  		xe_mmio_rmw32(mmio, OAM_COMPRESSION_T3_CONTROL, OAM_LAT_MEASURE_ENABLE, 0);
>  }
>  
> +/*
> + * Wait for the last OA batch of the stream, so that it can't program the HW after the HW
> + * teardown.
> + */
> +static void xe_oa_stream_drain(struct xe_oa_stream *stream)
> +{
> +	struct xe_exec_queue *q = stream->exec_q ?: stream->k_exec_q;
> +	long timeout;
> +
> +	if (!stream->last_fence || dma_fence_is_signaled(stream->last_fence))
> +		return;
> +
> +	timeout = msecs_to_jiffies(q->sched_props.job_timeout_ms) + HZ;
> +	if (!dma_fence_wait_timeout(stream->last_fence, false, timeout))
> +		drm_err(&stream->oa->xe->drm, "Timed out waiting for OA batch\n");
> +}

[Severity: High]
Can an OA config batch still program the hardware after teardown when
dma_fence_wait_timeout() times out here?

When xe_oa_emit_oa_config() submits a batch with both an unsignaled input
sync and an output sync, xe_oa_submit_bb() attaches the input sync as a
scheduler dependency via xe_sync_entry_add_deps() and returns without
waiting.  While that input sync remains unsignaled, the job sits in
q->entity before the scheduler's job_timeout_ms timer starts (or waits
behind earlier jobs on stream->exec_q).

If the stream is closed before the dependency signals,
xe_oa_stream_drain() times out after job_timeout_ms + HZ and returns
without killing the exec queue or canceling the job.  Because the pending
xe_sched_job holds a reference on job->q and free_oa_config_bo() keeps
oa_bo->bb->bo alive until stream->last_fence signals, signaling the input
sync after xe_oa_stream_destroy() completes will still run the batch on
the hardware after xe_oa_stream_hw_teardown() has restored the OA
registers, dropped forcewake, and cleared u->exclusive_stream.

>  /*
>   * Also drops forcewake, which accesses the HW too. On a wedged device this is skipped and
>   * the forcewake reference leaks, which is fine: the device needs a reset or rebind to
> @@ -914,6 +938,7 @@ static void xe_oa_stream_destroy(struct xe_oa_stream *stream)
>  	/* The poll timer may still be armed if the OA disable was skipped */
>  	hrtimer_cancel(&stream->poll_check_timer);
>  
> +	xe_oa_stream_drain(stream);
>  	if (!xe_wedged(stream->oa->xe))
>  		xe_oa_stream_hw_teardown(stream);

[Severity: High]
Does calling xe_oa_stream_drain() before xe_oa_stream_hw_teardown() leave
the context-restore LRI batch submitted during teardown undrained?

xe_oa_load_with_lri() is only reached via xe_oa_configure_oa_context().
During xe_oa_enable_metric_set(), the fence it stores in
stream->last_fence is immediately overwritten by the subsequent
xe_oa_emit_oa_config() call (or left undrained if xe_oa_emit_oa_config()
fails and jumps to err_put_k_exec_q).  During teardown,
xe_oa_stream_hw_teardown() -> xe_oa_disable_metric_set() ->
xe_oa_configure_oa_context(stream, false) calls xe_oa_load_with_lri() and
updates stream->last_fence after xe_oa_stream_drain() has already run.

As a result, the LRI batch that clears OACTXCONTROL, OAR_OACONTROL or
OAC_OACONTROL, and RING_CONTEXT_CONTROL remains in flight on
stream->exec_q after forcewake is released, the OA buffer is unpinned and
freed, and the OA unit is released for reuse by a new stream.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110190052.3514226-9-umesh.nerlige.ramappa@intel.com?part=5