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

["Pottumuttu, Sai Teja" <sai.teja.pottumuttu@intel.com>]

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?

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?

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