[PATCH v9 5/8] drm/xe/eustall: Add support to handle dropped EU stall data
Harish Chegondi
harish.chegondi at intel.com
Thu Feb 13 21:55:06 UTC 2025
On Wed, Feb 12, 2025 at 10:31:15PM -0800, Dixit, Ashutosh wrote:
> On Mon, 10 Feb 2025 05:46:46 -0800, Harish Chegondi wrote:
> >
>
Hi Ashutosh,
> Hi Harish,
>
> > If the user space doesn't read the EU stall data fast enough,
> > it is possible that the EU stall data buffer can get filled,
> > and if the hardware wants to write more data, it simply drops
> > data due to unavailable buffer space. In that case, hardware
> > sets a bit in a register. If the driver detects data drop,
> > the driver read() returns -EIO error to let the user space
> > know that HW has dropped data. The -EIO error is returned
> > even if there is EU stall data in the buffer. A subsequent
> > read by the user space returns the remaining EU stall data.
> >
> > v9: Move all data drop handling code to this patch
>
> Good, separating out makes this easier to review. I would actually make
> this the last patch, but anyway it's ok as is too.
>
> > Clear all drop data bits before returning -EIO.
> >
> > Signed-off-by: Harish Chegondi <harish.chegondi at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_eu_stall.c | 39 ++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
> > index 53f17aac7d3b..428267010805 100644
> > --- a/drivers/gpu/drm/xe/xe_eu_stall.c
> > +++ b/drivers/gpu/drm/xe/xe_eu_stall.c
> > @@ -53,6 +53,10 @@ struct xe_eu_stall_data_stream {
> > struct xe_gt *gt;
> > struct xe_bo *bo;
> > struct per_xecore_buf *xecore_buf;
> > + struct {
> > + bool reported_to_user;
> > + xe_dss_mask_t mask;
> > + } data_drop;
> > struct delayed_work buf_poll_work;
> > struct workqueue_struct *buf_poll_wq;
> > };
> > @@ -331,12 +335,24 @@ static bool eu_stall_data_buf_poll(struct xe_eu_stall_data_stream *stream)
> > if (num_data_rows(total_data) >= stream->wait_num_reports)
> > min_data_present = true;
> > }
> > + if (write_ptr_reg & XEHPC_EUSTALL_REPORT_OVERFLOW_DROP)
> > + set_bit(xecore, stream->data_drop.mask);
> > xecore_buf->write = write_ptr;
> > mutex_unlock(&xecore_buf->ptr_lock);
> > }
> > return min_data_present;
> > }
> >
> > +static void clear_dropped_eviction_line_bit(struct xe_gt *gt, u16 group, u16 instance)
> > +{
> > + u32 write_ptr_reg;
> > +
> > + /* On PVC, the overflow bit has to be cleared by writing 1 to it. */
> > + write_ptr_reg = _MASKED_BIT_ENABLE(XEHPC_EUSTALL_REPORT_OVERFLOW_DROP);
> > +
> > + xe_gt_mcr_unicast_write(gt, XEHPC_EUSTALL_REPORT, write_ptr_reg, group, instance);
> > +}
> > +
> > static int xe_eu_stall_data_buf_read(struct xe_eu_stall_data_stream *stream,
> > char __user *buf, size_t count,
> > size_t *total_data_size, struct xe_gt *gt,
> > @@ -436,6 +452,22 @@ static ssize_t xe_eu_stall_stream_read_locked(struct xe_eu_stall_data_stream *st
> > unsigned int xecore;
> > int ret = 0;
> >
> > + if (bitmap_weight(stream->data_drop.mask, XE_MAX_DSS_FUSE_BITS)) {
> > + if (!stream->data_drop.reported_to_user) {
> > + for_each_dss_steering(xecore, gt, group, instance) {
> > + if (test_bit(xecore, stream->data_drop.mask)) {
> > + clear_dropped_eviction_line_bit(gt, group, instance);
> > + clear_bit(xecore, stream->data_drop.mask);
> > + }
> > + }
>
> This is not making any sense. How can we clear_dropped_eviction_line_bit
> before reading the data? The HW will set it right back up.
This bit is being cleared here because the driver has acknowledged to
the user space all the data that has been dropped so far. Any new data
drop after we clear the bit *should* result in another -EIO return.
HW will set the bit again *only* if it drops more data. User space is
expected to do another read() to clear the buffer ASAP after getting
an -EIO from the first read().
>
> At least the code in the previous version made some sense. So we should at
> least go back to that and review that. Though it also had issue with how
> many times -EIO is returned etc.
Can you please explain what is the issue? Every time there is a new data
drop after the drop bit is cleared, it *should* result in another -EIO return
The driver ensures that two successive reads will not return -EIO.
>
> The other issue is the locking. As I suggested in my comments on Patch 4/8,
> I don't think we need a per xecore_buf lock, just one ptr_lock for reading
> all the DSS's. And then we can use that here too.
I used a per xecore_buf lock so that the polling function can update for
example subslice 1's write pointer while read() reads the data from
subslice 2. It would lead to more parallelism. With a higher granularity
lock, the polling for new data blocks during read and vice versa.
>
> But at least let's get something sane for review first.
Before I spawn another version of the patch series, I would like to
list the approaches to take here and cons for each approach.
Feel free to add any other approaches you suggest here. Feedback from
one of the EU stall consumers that they read data even after an -EIO
return means that any approach to discard data in the buffer
is unacceptable.
1. Approach used in version 8:
First read() returns -EIO without clearing any drop bits
Second read() reads the data and clears the bit for each subslice
Cons: If the user space buffer is small and all subslice data is not
read, those subslices whose drop bit is not cleared can cause -EIO in
the next read(). This issue can be mitigated by user space buffer
large enough to read data from all the subslices.
2. Approach used in version 9:
First read() returns -EIO after clearing all drop bits
Second read() reads all the EU stall data that can fit into the user
buffer.
Pros: Unlike in version 8 approach, even if the user buffer is small,
all drop bits get cleared in the first read()
Cons: Because the buffers are full, if the user space doesn't call
another read() soon to clear the data, there is a higher chance of HW
dropping the data again, setting the bit again.
3. New approach: Hybrid of version 8 and 9:
First read() returns -EIO without clearing any drop bits
Second read() reads all the EU stall data that can fit into the user
buffer, but clears all the drop bits even for those subslices it
can't read the data if the user buffer can't accommodate.
Cons: For those subslices whose data buffers are full and drop bits
are cleared, there is a chance that the HW can set the bits again if
data is dropped again unless user space calls another read() fast
enough. This can also be mitigated with a user space buffer large
enough to read data from all the subslices.
Please feel free to add any new approaches here that those don't discard
data in the buffers, also add any more cons to the above approaches.
Let's agree on an approach for the next patch version.
Thank you
Harish.
> > + stream->data_drop.reported_to_user = true;
> > + xe_gt_dbg(gt, "EU stall data dropped in XeCores: %*pb\n",
> > + XE_MAX_DSS_FUSE_BITS, stream->data_drop.mask);
> > + return -EIO;
> > + }
> > + stream->data_drop.reported_to_user = false;
> > + }
> > +
> > for_each_dss_steering(xecore, gt, group, instance) {
> > ret = xe_eu_stall_data_buf_read(stream, buf, count, &total_size,
> > gt, group, instance, xecore);
> > @@ -457,6 +489,7 @@ static ssize_t xe_eu_stall_stream_read_locked(struct xe_eu_stall_data_stream *st
> > * before calling read().
> > *
> > * Returns: The number of bytes copied or a negative error code on failure.
> > + * -EIO if HW drops any EU stall data when the buffer is full.
> > */
> > static ssize_t xe_eu_stall_stream_read(struct file *file, char __user *buf,
> > size_t count, loff_t *ppos)
> > @@ -543,6 +576,9 @@ static int xe_eu_stall_stream_enable(struct xe_eu_stall_data_stream *stream)
> >
> > for_each_dss_steering(xecore, gt, group, instance) {
> > write_ptr_reg = xe_gt_mcr_unicast_read(gt, XEHPC_EUSTALL_REPORT, group, instance);
> > + /* Clear any drop bits set and not cleared in the previous session. */
> > + if (write_ptr_reg & XEHPC_EUSTALL_REPORT_OVERFLOW_DROP)
> > + clear_dropped_eviction_line_bit(gt, group, instance);
> > write_ptr = REG_FIELD_GET(XEHPC_EUSTALL_REPORT_WRITE_PTR_MASK, write_ptr_reg);
> > read_ptr_reg = REG_FIELD_PREP(XEHPC_EUSTALL_REPORT1_READ_PTR_MASK, write_ptr);
> > read_ptr_reg = _MASKED_FIELD(XEHPC_EUSTALL_REPORT1_READ_PTR_MASK, read_ptr_reg);
> > @@ -554,6 +590,9 @@ 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->data_drop.reported_to_user = false;
> > + bitmap_zero(stream->data_drop.mask, XE_MAX_DSS_FUSE_BITS);
> > +
> > reg_value = _MASKED_FIELD(EUSTALL_MOCS | EUSTALL_SAMPLE_RATE,
> > REG_FIELD_PREP(EUSTALL_MOCS, gt->mocs.uc_index << 1) |
> > REG_FIELD_PREP(EUSTALL_SAMPLE_RATE,
> > --
> > 2.48.1
> >
More information about the Intel-xe
mailing list