Re: [PATCH v9 4/8] drm/xe/eustall: Add support to read() and poll() EU stall data

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

On Mon, 17 Feb 2025 16:35:35 -0800, Harish Chegondi wrote:
>
> > > > > > > +	if (!(file->f_flags & O_NONBLOCK)) {
> > > > > > > +		do {
> > > > > > > +			ret = wait_event_interruptible(stream->poll_wq, stream->pollin);
> > > > > > > +			if (ret)
> > > > > > > +				return -EINTR;
> > > > > > > +
> > > > > > > +			mutex_lock(&gt->eu_stall->stream_lock);
> > > > > > > +			ret = xe_eu_stall_stream_read_locked(stream, file, buf, count);
> > > > > > > +			mutex_unlock(&gt->eu_stall->stream_lock);
> > > > > > > +		} while (ret == -EAGAIN);
> > > > > > > +	} else {
> > > > > > > +		mutex_lock(&gt->eu_stall->stream_lock);
> > > > > > > +		ret = xe_eu_stall_stream_read_locked(stream, file, buf, count);
> > > > > > > +		mutex_unlock(&gt->eu_stall->stream_lock);
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	stream->pollin = false;
> > > > > >
> > > > > > Carry over comment from previous rev: this breaks if user buffer is smaller
> > > > > > than available data. But this is a corner case so let's fix this after the
> > > > > > initial merge.
> > > > > I will check your fix in OA to better understand the problem here. If
> > > > > the user buffer is smaller than data record size, it just returns
> > > > > without copying any data.
> > > >
> > > > No, it is the issue when user buffer is larger than record_size, but still
> > > > smaller than the size of total data available. In that case, basically we
> > > > should let stream->pollin set to true, so that the user thread does not get
> > > > blocked, enabling it to rapidly consume data, even with a small buffer.
> > > >
> > > > Otherwise the thread is getting blocked every 10 ms, even when data is
> > > > available. And because new data keeps arriving, the user thread constantly
> > > > falls behind and soon HW will start dropping data (because the kernel is
> > > > blocking the user thread, it's a kernel not a user thread issue)
> > > >
> > > > To handle this OA does:
> > > >
> > > >	if (ret != -ENOSPC && ret != -EIO)
> > > EU stall read() never returns -ENOSPC. Even if the user buffer is
> > > smaller then the size of total data in the buffer, only the data that
> > > can fit into the user buffer will be copied through read().
> >
> > OA read() also doesn't return -ENOSPC to userland. That is the expected
> > read() behavior (return number of bytes if you are returning any data). The
> > full code is this:
> >
> >         if (ret != -ENOSPC && ret != -EIO)
> >                 stream->pollin = false;
> >
> >         return offset ?: (ret ?: -EAGAIN);
> >
> > > As long as there are atleast wait_num_reports data rows in the buffer,
> > > stream->pollin is set to true. It sounds to me that this issue is
> > > specific to OA code only. Is there something that I missed?
> >
> > No this issue is there in EU stall code. You should not block reads till
> > the next time the polling thread runs, if we could not copy all data in the
> > previous read (because there already is data, don't block the user thread).
> The polling thread runs approximately once every 10ms. The question here
> is what would be time duration between two successive reads from the
> user space with no additional delay between them. If the duration is
> more than 10ms, the polling thread would set pollin to true. If not,
> the issue you have mentioned would occur.

If we don't block the user thread in the kernel, ideally it would call into
the kernel continuously to read data, let's say every few us. Say a memcpy
in userspace and then back into the kernel to read more data.

> However, I am changing the code here to this:
>
>         if (!eu_stall_data_buf_poll(stream))
>                 stream->pollin = false;
>
> Wouldn't this fix the issue?

No, this is not correct. There are two requirements:

1. Don't wake up the user thread too frequently, that is why we have the 10
   ms timer, so ordinarily, with a large buffer, the user thread only wakes
   up only every 10 ms.

2. Only when the user buffer is small, that is we have data which we could
   not copy in the user buffer, we shouldn't block the user thread.

In the above code, you are checking for data after completing the memcpy
into the user buffer. This would solve 2., but might break 1. Because the
code does not take into account that we should not block the user thread
only if the user buffer is small.

Let's not introduce this into the existing patches, this small user buffer
handling would need a new patch. As I said, let's do this after we merge
what we currently have. I don't want to review this stuff now.

Thanks.
--
Ashutosh