Re: [PATCH v2 1/1] drm/xe/eustall: Return EBADFD from read if EU stall registers get reset

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

On Mon, 16 Mar 2026 10:58:56 -0700, Harish Chegondi wrote:
>
> If a reset (GT or engine) happens during EU stall data sampling, all the
> EU stall registers can get reset to 0. This will result in EU stall data
> buffers' read and write pointer register values to be out of sync with
> the cached values. This will result in read() returning invalid data. To
> prevent this, check the value of a EU stall base register. If it is zero,
> it indicates a reset may have happened that wiped the register to zero.
> If this happens, return EBADFD from read() upon which the user space
> should close the fd and open a new fd for a new EU stall data
> collection session.
>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> ---
> v2: Move base register check from read to the poll function
>
>  drivers/gpu/drm/xe/xe_eu_stall.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
> index c34408cfd292..7e14de73a2c9 100644
> --- a/drivers/gpu/drm/xe/xe_eu_stall.c
> +++ b/drivers/gpu/drm/xe/xe_eu_stall.c
> @@ -44,6 +44,7 @@ struct per_xecore_buf {
>  struct xe_eu_stall_data_stream {
>	bool pollin;
>	bool enabled;
> +	bool reset_detected;
>	int wait_num_reports;
>	int sampling_rate_mult;
>	wait_queue_head_t poll_wq;
> @@ -428,6 +429,17 @@ static bool eu_stall_data_buf_poll(struct xe_eu_stall_data_stream *stream)
>			set_bit(xecore, stream->data_drop.mask);
>		xecore_buf->write = write_ptr;
>	}
> +	/* If a GT or engine reset happens during EU stall sampling,
> +	 * all EU stall registers get reset to 0 and the cached values of
> +	 * the EU stall data buffers' read pointers are out of sync with
> +	 * the register values. This causes invalid data to be returned
> +	 * from read(). To prevent this, check the value of a EU stall base
> +	 * register. If it is zero, there has been a reset.
> +	 */

As previously discussed, the best way would have been to not have to do
this. We would just plug into the handler for the reset message from GuC,
rather than to implement a reset detection here (and in other places such
as OA). But looks like if we do that, because of the way EUSS registers are
reset, we can return bad EUSS data. So looks like there is no way around
doing this "reset detection" here and a solution with the GuC reset handler
would always be racy. Just for the record.

> +	if (unlikely(!xe_gt_mcr_unicast_read_any(gt, XEHPC_EUSTALL_BASE))) {
> +		stream->reset_detected = true;
> +		min_data_present = true;

I don't believe we need to set 'min_data_present = true' if we are setting
'stream->reset_detected = true', correct? See if statement at the bottom.

Also, since the write pointer itself gets reset during reset, didn't we
want to do this register read only when the write pointer is 0 (to avoid an
extra register read every 5 ms)?

> +	}
>	mutex_unlock(&stream->xecore_buf_lock);
>
>	return min_data_present;
> @@ -554,6 +566,15 @@ static ssize_t xe_eu_stall_stream_read_locked(struct xe_eu_stall_data_stream *st
>		}
>		stream->data_drop.reported_to_user = false;
>	}
> +	/* If EU stall registers got reset due to a GT/engine reset,
> +	 * continuing with the read() will return invalid data to
> +	 * the user space. Just return -EBADFD instead.
> +	 */
> +	if (unlikely(stream->reset_detected)) {
> +		xe_gt_dbg(gt, "EU stall base register has been reset\n");
> +		mutex_unlock(&stream->xecore_buf_lock);
> +		return -EBADFD;

The other option is to return -EIO here and implement
DRM_XE_OBSERVATION_IOCTL_STATUS and return status from that. Let me think
some more about this.

> +	}
>
>	for_each_dss_steering(xecore, gt, group, instance) {
>		ret = xe_eu_stall_data_buf_read(stream, buf, count, &total_size,
> @@ -692,6 +713,7 @@ static int xe_eu_stall_stream_enable(struct xe_eu_stall_data_stream *stream)
>		xecore_buf->write = write_ptr;
>		xecore_buf->read = write_ptr;
>	}
> +	stream->reset_detected = false;

So after reset, if a stream is disabled and re-enabled, we expect things to
work again and EUSS data to be correct (without re-opening a new stream)?

>	stream->data_drop.reported_to_user = false;
>	bitmap_zero(stream->data_drop.mask, XE_MAX_DSS_FUSE_BITS);
>
> @@ -717,7 +739,7 @@ static void eu_stall_data_buf_poll_work_fn(struct work_struct *work)
>		container_of(work, typeof(*stream), buf_poll_work.work);
>	struct xe_gt *gt = stream->gt;
>
> -	if (eu_stall_data_buf_poll(stream)) {
> +	if (stream->reset_detected || eu_stall_data_buf_poll(stream)) {
>		stream->pollin = true;
>		wake_up(&stream->poll_wq);
>	}
> --
> 2.43.0
>