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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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

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

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?

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

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

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
>