[PATCH i-g-t 2/2] tests/intel/xe_oa: Replace hardcoded OA buffer size assumptions

[Dixit, Ashutosh]

On Mon, 18 Nov 2024 23:01:50 -0800, Pottumuttu, Sai Teja wrote:
>
> On 19-11-2024 01:54, Dixit, Ashutosh wrote:
> > On Sun, 17 Nov 2024 22:32:23 -0800, Sai Teja Pottumuttu wrote:
> >>
> >> A few xe_oa tests assumed that the default OA buffer size is 16MB
> >> and had calculations around it. These would fail if the default
> >> buffer size is changed on the kernel side.
> >>
> >
> > I don't think this is correct. buf_size below refers to the size of the
> > user's buffer, not the size of the kernel OA buffer.
> >
> > You also seem to be assuming that '65536 * report_size == oa_buffer_size'
> > (65536 * 256 == 16 MB) but report size is not only 256, Xe2 default report
> > size is 576 bytes and there are other report sizes too.
> >
> > Maybe ok to do what this patch is doing for test_buffer_fill and
> > test_enable_disable, but we need to be careful about test_non_zero_reason
> > since that test is very useful for testing OA buffer wrap-arounds (and some
> > corner cases there).
> >
>
> So, my assumption was that in i915 we were trying to do
>
> = (oa_buffer_size / report_size) * (report_size + header_size)
>
> which we hardcoded like
>
> = 65536 * (report_size + header_size)
>
> And this was done to get a buffer full of reports and allocate user buffer
> a little bigger than kernel buffer as we had headers there.
>
> I think this translated into just 65536 * report_size in xe as we don't
> have headers separately here.
>
> Am I correct with this assumption?

Yes.

>
> 1. If yes, replacing it with oa_buffer_size is fine for atleast
> test_buffer_fill and test_enable_disable right? As we were essentially
> trying to allocate a user buffer which can take all the reports which would
> fit in a kernel oa buffer. Am I missing something?
>
> If that's correct, as we are already at it, maybe we can keep the change
> for these two tests?

But even here, why would we link the kernel internal OA buffer size with
the user buffer size? User buffer size can be independent of the OA buffer
size.

> 2. For test_non_zero_reason I think I get your point that with the current
> sizing greater than 3 * oa_buffer_size, probably we could catch more
> issues. So, we can keep it as it is probably.
>
> Thanks
> - Sai Teja
>
> > I'd say let's drop this patch for now and revisit later if needed.
> >
> >> Replace such assumptions with the OA buffer size we get from
> >> DRM_XE_OBSERVATION_IOCTL_INFO ioctl
> >>
> >> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu at intel.com>
> >> ---
> >>   tests/intel/xe_oa.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> >> index 4e4d0fabc..3b65212ae 100644
> >> --- a/tests/intel/xe_oa.c
> >> +++ b/tests/intel/xe_oa.c
> >> @@ -2389,7 +2389,7 @@ test_buffer_fill(const struct drm_xe_engine_class_instance *hwe)
> >>		.properties_ptr = to_user_pointer(properties),
> >>	};
> >>	size_t report_size = get_oa_format(fmt).size;
> >> -	int buf_size = 65536 * report_size;
> >> +	int buf_size = oa_buffer_size;
> >>	uint8_t *buf = malloc(buf_size);
> >>	int len;
> >>	size_t oa_buf_size = oa_buffer_size;
> >> @@ -2533,7 +2533,7 @@ test_non_zero_reason(const struct drm_xe_engine_class_instance *hwe)
> >>		.num_properties = ARRAY_SIZE(properties) / 2,
> >>		.properties_ptr = to_user_pointer(properties),
> >>	};
> >> -	uint32_t buf_size = 3 * 65536 * report_size;
> >> +	uint32_t buf_size = 3 * oa_buffer_size;
> >>	uint8_t *buf = malloc(buf_size);
> >>	uint32_t total_len = 0;
> >>	const uint32_t *last_report;
> >> @@ -2615,7 +2615,7 @@ test_enable_disable(const struct drm_xe_engine_class_instance *hwe)
> >>		.properties_ptr = to_user_pointer(properties),
> >>	};
> >>	size_t report_size = get_oa_format(fmt).size;
> >> -	int buf_size = 65536 * report_size;
> >> +	int buf_size = oa_buffer_size;
> >>	uint8_t *buf = malloc(buf_size);
> >>	size_t oa_buf_size = oa_buffer_size;
> >>	int n_full_oa_reports = oa_buf_size / report_size;
> >> --
> >> 2.34.1
> >>
> >
> > Thanks.
> > --
> > Ashutosh