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

[Harish Chegondi <harish.chegondi@intel.com>]

On Tue, Mar 17, 2026 at 11:57:32PM -0700, Dixit, Ashutosh wrote:
> 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.

Thanks for the summary of the previous discussion. Yes, hooking into the
GUc reset notification handler will be racy and bad EUSS data will be
returned to the user space if read() happens after the reset but before
the GuC reset notification message is processed. That's the reason for
not taking that approach.

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

Agree. The only difference is that the if statement at the bottom will
evaluate true in the current execution of eu_stall_data_buf_poll_work_fn
if min_data_present is set to true. If min_data_present is not set to
true, the if statement will evaluate to true in the subsequent execution
of eu_stall_data_buf_poll_work_fn() which is still okay. So, yes, we
don't have to set min_data_present to true here. Will fix in the next
version.
> 
> 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)?
Good point. I have thought about reducing the number of this register
reads. The poll function reads the write pointers of all the xecores.
A reset can happen anytime the poll function is reading the write
pointers of the xecores. If the reset happens before the poll function
started reading the write pointers, all write pointers are zeros.
If the reset happens during the poll function, several write pointers
read so far can be non-zero while the rest of the pointers after reset
are all zeros. The if reset happens right after the poll function, the
write pointers can be a mix of zeros and non-zeros.
I think the only time this register read can be skipped is if the
LAST write pointer read is non-zero which means a reset did not happen
before or during the poll function. Do you agree? I thought of adding a
check to the if statement to check if the last write pointer is
non-zero, but to keep the code clean, I didn't. Also, if there are
n xecores, there will be n write pointer register reads plus one
additional base register read, which isn't too bad? Also, hoping the use
of unlikely macro would not impact the performance too much.
> 
> > +	}
> >	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.

I think EBADFD is more appropriate errno than EIO in this case since the
fd is in a corrupted state and user has to close and re-open the fd.
Currently, the -EIO is used to indicate drop data in which case, the
user space can continue to read the data (faster) without closing the fd.
> 
> > +	}
> >
> >	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)?
Technically, yes, since the EU stall registers programming is done in
enable, things will work again if the stream is disabled and re-enabled.
But if the EUSS registers programming is moved into open() in the
future, things may not work by disabling and re-enabling the stream. So,
I think we suggest to the UMDs to close the stream and open a new
stream.

Thank You
Harish.
> 
> >	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
> >