[PATCH i-g-t 3/4] tests/intel/xe_oa: Add a test for tail address wrap

[Umesh Nerlige Ramappa]

On Mon, Aug 25, 2025 at 11:03:37AM -0700, Dixit, Ashutosh wrote:
>On Fri, 22 Aug 2025 17:34:05 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>OK good to add such as test, previously tail address wrap was only tested
>indirectly, e.g. by report headers being incorrect. But with multiple
>problems such as report sizes being wrong for different formats, a specific
>test for tail address wrap is good to have.
>
>> Add a test to verify that the HW wraps to start of the OA buffer when a
>> report does not fit in the remaining space in the buffer.
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> ---
>>  tests/intel/xe_oa.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>
>> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> index 745f364e52e9..7381e7b9bd44 100644
>> --- a/tests/intel/xe_oa.c
>> +++ b/tests/intel/xe_oa.c
>> @@ -4535,6 +4535,76 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
>>	try_invalid_access(vaddr);
>>  }
>>
>> +/**
>> + * SUBTEST: tail-address-wrap
>> + * Description: Test tail address wrap on odd format sizes. Ensure that the
>> + * format size is not a power of 2. This means that the last report will not be
>> + * broken down across the OA buffer end. Instead it will be written to the
>> + * beginning of the OA buffer. We will check the end of the buffer to ensure it
>> + * has zeroes in it.
>> + */
>> +static void
>> +test_tail_address_wrap(const struct drm_xe_engine_class_instance *hwe)
>> +{
>> +#define TIMER_PERIOD_US 20
>
>Let's not call this "TIMER_PERIOD", something like ADDR_WRAP_US etc.
>

It's actually OA EXPONENT PERIOD, so I called it timer period. Reports 
are being generated at 20us intervals.

>> +	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
>> +	uint64_t exponent = max_oa_exponent_for_period_lte(TIMER_PERIOD_US * 1000);
>> +	uint64_t fmt = test_set->perf_oa_format;
>> +	uint64_t properties[] = {
>> +		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
>> +		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
>> +		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
>> +		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
>> +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, exponent,
>> +		DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE, hwe->engine_instance,
>> +		DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE, buffer_fill_size,
>> +	};
>> +	struct intel_xe_oa_open_prop param = {
>> +		.num_properties = ARRAY_SIZE(properties) / 2,
>> +		.properties_ptr = to_user_pointer(properties),
>> +	};
>> +	uint32_t fmt_size = get_oa_format(fmt).size;
>> +	uint32_t fit_reports = buffer_fill_size / fmt_size;
>> +	uint32_t wrap_offset = (fit_reports * fmt_size) >> 2;
>> +	uint32_t end_offset = buffer_fill_size >> 2;
>> +	uint32_t *report, *zero_area, *buffer_end;
>> +	void *buffer_start;
>> +	int i, tolerance = 10;
>> +
>> +	igt_require(oau->capabilities & DRM_XE_OA_CAPS_OA_BUFFER_SIZE);
>> +
>> +	/* Ensure report does not fit */
>> +	igt_require(wrap_offset < end_offset);
>> +
>> +	/* mmap the buffer */
>> +	stream_fd = __perf_open(drm_fd, &param, false);
>> +	buffer_start = mmap(0, buffer_fill_size, PROT_READ, MAP_PRIVATE, stream_fd, 0);
>> +	igt_assert(buffer_start);
>> +
>> +	/* Wait for reports to fill the buffer with some tolerance */
>> +	report = buffer_start;
>> +	for (i = 0; fit_reports && i < fit_reports + tolerance; i++) {
>
>Since fit_reports is decreasing below and i is increasing, looks like this
>loop will exit when OA buffer is half full? Maybe that is why the test is
>always passing? Tolerance is not making much sense either since it will
>make the report pointer go over the OA buffer? So we need to simplify the
>loop.

Yikes, bad programming... Cannot use fit_reports that is decrementing in 
the loop check.

Tolerance is intentional. I want the reports to go over the fit_report 
value so that I can ensure that the HW did not write to the region in 
the end of the buffer where the report does not fit. Technically, a 
tolerance of 1 is good enough.

>
>Maybe we can add an argument to mmap_wait_for_periodic_reports() to also
>count all reports, so make the function work for both periodic and
>non-periodic reports and use it here?

Yeah, a possibility.

>
>So wait for the buffer to first fill once and once that happens wait for it
>to completely fill again (maybe even more than once). Something like
>that. But maybe will need to zero out the buffer for
>mmap_wait_for_periodic_reports() which probably is not allowed by the
>kernel.

No, I am just wanting to fill the buffer once and then wait for at least 
one additional report to land in the buffer. However, like you said, 
mmap mode will not clear the buffer header, so we don't know if a new 
report landed. So I am just using a 10 * TIMER_PERIOD_US delay to let 
that happen.

>So anyway, need some way to figure out if the buffer is filled
>again, like read the tail pointer. Or go back to verifying OA packet
>headers...


Maybe this:

We could stop the OA capture, read out the entire buffer + a tolreance 
amount of reports. Ensure all reports are valid, Then we check this 
region to make sure it's all zeroes.

>
>> +		usleep(TIMER_PERIOD_US);
>> +		if (*report && oa_timestamp(report, fmt)) {
>> +			fit_reports--;
>> +			report += (fmt_size >> 2);
>> +		}
>> +	}
>> +
>> +	/* Let some reports land in the start of the buffer */
>> +	usleep(tolerance * TIMER_PERIOD_US);
>
>My preference is to not use direct usleep's since it breaks in different
>environments (sim etc.), see comments above. We've eliminated several such
>things in Xe OA IGT's previously iirc.
>
>Maybe separate out this patch so that the other patches can get merged
>first?

Will post others first, but anyways wanted to get your feedback on this 
and looks like it has a glaring bug.

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh
>
>> +
>> +	zero_area = (uint32_t *)buffer_start + wrap_offset;
>> +	buffer_end = (uint32_t *)buffer_start + end_offset;
>> +
>> +	/* Ensure HW did not write to this area */
>> +	while (zero_area < buffer_end)
>> +		igt_assert_eq(*zero_area++, 0);
>> +
>> +	munmap(buffer_start, buffer_fill_size);
>> +	__perf_close(stream_fd);
>> +}
>> +
>>  /**
>>   * SUBTEST: map-oa-buffer
>>   * Description: Verify mapping of oa buffer
>> @@ -5091,6 +5161,10 @@ igt_main
>>		igt_subtest_with_dynamic("closed-fd-and-unmapped-access")
>>			__for_one_hwe_in_oag(hwe)
>>				closed_fd_and_unmapped_access(hwe);
>> +
>> +		igt_subtest_with_dynamic("tail-address-wrap")
>> +			__for_one_hwe_in_oag(hwe)
>> +				test_tail_address_wrap(hwe);
>>	}
>>
>>	igt_subtest_group {
>> --
>> 2.43.0
>>