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

Harish Chegondi
2026-03-20T20:59:28Z

On Wed, Mar 18, 2026 at 08:55:42PM -0700, Dixit, Ashutosh wrote:
> 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?
But switching the order of the two OR operands would cause the function
eu_stall_data_buf_poll() to be unnecessarily called even after a reset
is detected.
>
> > >
> > > 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.
Okay, I will make sure the reset_detected flag remains set even after a
disable and an enable.

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