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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 23 Dec 2025 15:39:47 -0800, Harish Chegondi wrote:
>
> 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.

Yeah, I know, that's why I said check the mask bits.

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

Let's first check write_ptr. If it works out we can use that. Otherwise my
preference I think is to avoid introducing an additional register read
anywhere. What we could do is hook into the context reset callback and if
context reset occurs we just unblock any blocked threads, followed by
returning -EBADFD from read(), even if we return bad data. We revisit if
UMD's insist bad data is unacceptable (even in case of a reset).

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