[PATCH 17/17] drm/xe/oa: Enable Xe2+ overrun mode

[Dixit, Ashutosh]

On Tue, 12 Mar 2024 13:14:14 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Mon, Mar 11, 2024 at 08:40:03PM -0700, Ashutosh Dixit wrote:
> > Enable Xe2+ overrun mode. For Xe2+, when overrun mode is enabled, there are
> > no partial reports at the end of buffer, making the OA buffer effectively a
> > non-power-of-2 size circular buffer whose size, circ_size, is a multiple of
> > the report size.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_oa.c       | 36 +++++++++++++++++++++++++-------
> > drivers/gpu/drm/xe/xe_oa_types.h |  3 +++
> > 2 files changed, 31 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 6f5bbb0787d9..6a0d2e229254 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -106,7 +106,15 @@ static const struct xe_oa_format oa_formats[] = {
> >
> > static u32 xe_oa_circ_diff(struct xe_oa_stream *stream, u32 tail, u32 head)
> > {
> > -	return (tail - head) & (XE_OA_BUFFER_SIZE - 1);
> > +	if (stream->oa_buffer.circ_size == XE_OA_BUFFER_SIZE)
> > +		return (tail - head) & (XE_OA_BUFFER_SIZE - 1);
> > +	else
> > +		return (tail - head) % stream->oa_buffer.circ_size;
> > +}
>
> For ex: consider a 16 MB buffer with a report size of 384 bytes. At the end
> of the buffer, you would have an empty space of 256 bytes (16 MB % 384)
>
> (For ref: 16 MB = 0x1000000, 384 = 0x180)
> In this case circ_size = 0xFFFF00
>
> Let's say your head is pointing to 0xFFFD80 and tail is pointing to 0x180
> (essentially there is one unread report at the end of the buffer and one
> unread report at the beginning of the buffer).
>
> In this case, (tail - head) % stream->oa_buffer.circ_size, is not
> calculating the correct size. Should be 0x300, but I am not getting
> that. Can you please check/verify?
>
> I am thinking we need something like this (roughly). We don't need the mod
> operation.

First of all, thank you so much for checking, and catching, this huge
bug. I did some digging into it and it is to do with how the % operator
behaves with -ve numbers, as well as with u32 vs. s32 data types. And yes
we should not use the % operation.

If this bug had got merged, I can only imagine what a nightmare it would be
to find and fix it, so it's great it is caught in the code review.

I missed this completely because IGT tests with Xe2 overrun mode enabled
were passing in spite of this bug! Specially igt at non-zero-reason would hang
previously, because we wouldn't find any reports because we would be
misaligned, due to the empty space at the end of the OA buffer. This test
was also passing in spite of this bug, I am still trying to figure out how
it could still pass.

In any case, I have sent out the latest version of just this patch (rather
than the entire series) with this issue hopefully fixed. Please take a
look.

> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index 6984e7d04be5..d8d5c9d8c22e 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -163,6 +163,9 @@ struct xe_oa_buffer {
> >
> >	/** @tail: The last verified cached tail where HW has completed writing */
> >	u32 tail;
> > +
> > +	/** @circ_size: The effective circular buffer size, for Xe2+ */
> > +	u32 circ_size;
>
> You could store the difference here instead.
>
>	/** @empty_space: empty space at tend of buffer */
>	u32 empty_space;

I have left circ_size here as it was in the previous version, since I think
it better indicates that we still have a non-power-of-2 sized circular
buffer even in this Xe2+ overrun case.

Thanks.
--
Ashutosh