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 Wed, Mar 18, 2026 at 09:59:04AM -0700, Cabral, Matias A wrote:

Hi Matias,

For UMD, read() of EU stall stream would return -1 and errno will be EBADFD.

Thanks
Harish.
> Hi Harish,
> 
> > return -EBADFD;
> 
> Is this making the read() syscall return -EBADFD or setting erno to EBADFD ?  would like to clearly understand how to detect this condition in the UMD. 
> 
> Thanks, 
> _Matias
> 
> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com> 
> Sent: Tuesday, March 17, 2026 11:58 PM
> To: Chegondi, Harish <harish.chegondi@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Degrood, Felix J <felix.j.degrood@intel.com>; Cabral, Matias A <matias.a.cabral@intel.com>; Ranjan, Joshua Santhosh <joshua.santosh.ranjan@intel.com>
> Subject: Re: [PATCH v2 1/1] drm/xe/eustall: Return EBADFD from read if EU stall registers get reset
> 
> 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
> >