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 12:51, Dixit, Ashutosh wrote:
> 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.

If we want to keep it separate then yeah this patch isn't required I think.

I was just trying to make it dynamic so that we completely get rid of 
hardcoded oa buffer sizes.

I will drop this patch in next revision.

Thanks
- Sai Teja

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