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 Wed, 18 Mar 2026 14:30:06 -0700, Harish Chegondi wrote:
>
> 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.

Just switch the order of the two OR operands and you don't have that issue?

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

OK, leave this as is I think. Otherwise we'll need to read the base
register each time we see a zero write pointer. So it's ok, leave as is.

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

OK, we can go with -EBADFD, though still thinking about it.

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

No we don't suggest anything to UMD's. We decide what we want to do,
implement and enforce it that way and then maintain that uapi.

OK, then let us make sure after disable/enable, the reset_detected flag
remains set.

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