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

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

On Mon, Dec 22, 2025 at 09:08:04PM -0800, Dixit, Ashutosh wrote:
> On Mon, 22 Dec 2025 14:37:53 -0800, Harish Chegondi wrote:
> >
> > 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.
> 
> In that case we need to ask GuC team about "pre context reset" g2h/h2g
> messages/callbacks.
> 
> Is it possible to deduce that reset has happened from the write_ptr
> register which is read in eu_stall_data_buf_poll? Say what happens to the
> mask bits when reset occurs (why are the mask bits there in the first place
> in a register which cannot be written by SW)? Then we can use that to set
> stream->reset, and not have to introduce an extra register read.
The write_ptr gets reset to 0, but it happens when the buffer wraps
around too. I am not sure about the mask bits, but I can check what
happens to them after a reset.
> 
> In a sense read() cannot return bad data since it uses cached read/wrt
> ptr's. So as long as we catch the reset while updating write_ptr, we should
> be ok.
> 
> Or, can we can drop the constraint that read() will never return bad
> data. If a reset happens, read() can return bad data.
> 
> Or do you have any other ideas to address the issues I mentioned above?
You earlier suggested about checking the base register in the polling
thread. If we set the stream->reset in the polling thread, it can be
used in both read() and poll(). Do you think this would address the
issues you mentioned above ?

Thank You
Harish.


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