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

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

On Thu, Dec 18, 2025 at 11:53:02AM -0800, Dixit, Ashutosh wrote:
> On Sun, 07 Dec 2025 22:16:11 -0800, Harish Chegondi wrote:
> >
> 
Hi Ashutosh,
> Hi Harish,
> 
> > 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 can 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>
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_eu_stall.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
> > index 97dfb7945b7a..02c0beb4559f 100644
> > --- a/drivers/gpu/drm/xe/xe_eu_stall.c
> > +++ b/drivers/gpu/drm/xe/xe_eu_stall.c
> > @@ -541,9 +541,24 @@ static ssize_t xe_eu_stall_stream_read_locked(struct xe_eu_stall_data_stream *st
> >	size_t total_size = 0;
> >	u16 group, instance;
> >	unsigned int xecore;
> > +	u32 base_reg_value;
> >	int ret = 0;
> >
> >	mutex_lock(&stream->xecore_buf_lock);
> > +	/* If a GT or engine reset happens during EU stall data sampling,
> > +	 * all EU stall registers get reset to 0 and the cached values of
> > +	 * EU stall data buffers' read and write pointers are out of sync
> > +	 * with the register values. This can cause invalid data to be
> > +	 * returned from read(). To prevent this, check the value of a
> > +	 * EU stall base register. If it is zero, return -EBADFD. The
> > +	 * user is expected to close the fd and open a new fd.
> > +	 */
> > +	base_reg_value = xe_gt_mcr_unicast_read_any(gt, XEHPC_EUSTALL_BASE);
> > +	if (unlikely(!base_reg_value)) {
> > +		xe_gt_dbg(gt, "EU stall base register has been reset to 0\n");
> > +		mutex_unlock(&stream->xecore_buf_lock);
> > +		return -EBADFD;
> > +	}
> 
> So I am seeing two problems here:
> 
> 1. We are doing register read every read() call, rather than just when a
>    reset happens.
> 
> 2. The other issue is should reset itself unblock a blocked poll() or
>    blocking read() call? If we don't do that, it is possible that poll()
>    or blocking read() remains blocked indefinitely and so either the
>    non-blocking read() doesn't get called at all, or a blocking read()
>    remains indefinitely blocked. So that we never actually return -EBADFD
>    even though a reset has happened.
> 
>    (Note that, for exec(), I believe any blocked fences will unblock and
>    return error etc. if a reset happens during an exec() call (see
>    reset_status()), so EU stall should probably do something similar).
> 
> So to address these two issues how about doing something like this:
> 
> 1. Call an EU stall callback from xe_guc_exec_queue_reset_handler(). In the
>    callback, if an EU stall stream is open on that gt, check if
>    XEHPC_EUSTALL_BASE is 0 and set a stream variable stream->reset under a
>    suitable lock (likely xecore_buf_lock).
I thought about this and I think this can be racy - if read() is called
after the engine and the EU stall registers got reset but before the
stream->reset is set, the read() would return bad EU stall data. I think
the XEHPC_EUSTALL_BASE register should be checked either in the polling
thread or in read (as in this patch) to avoid any race conditions.

> 
> 2. From eu_stall_data_buf_poll(), if stream->reset is set, return true to
>    wake up any waiters. We may also need to set POLLERR or POLLHUP revents.
> 
> 3. Now from read(), if stream->reset is set return -EBADFD.
> 
> So I think something like this solves both problems mentioned above.
> 
> So could you please look into this and see if this is possible? Or any
> other thoughts about this?
> 
> Thanks.
> --
> Ashutosh
Thank You
Harish.
> 
> >	if (bitmap_weight(stream->data_drop.mask, XE_MAX_DSS_FUSE_BITS)) {
> >		if (!stream->data_drop.reported_to_user) {
> >			stream->data_drop.reported_to_user = true;
> > --
> > 2.43.0
> >