Re: [PATCH 4/4] tests/intel/xe_oa: Cleanup blocking and polling tests

On Mon, Jul 20, 2026 at 02:08:11PM -0700, Dixit, Ashutosh wrote:
>On Fri, 17 Jul 2026 14:00:32 -0700, Umesh Nerlige Ramappa wrote:
>>
>> 1) Drop set_kernel_timer since we don't support it anymore.
>> 2) When running all tests, a failure in blocking/polling tests breaks
>>    all subsequent tests. Move __perf_close before the asserts.
>
>Hmm, this should, in the future, be handled by having some sort of an
>exit/assert handler, which will close the stream in the event of an
>assert. Yeah this is annoying.

I think this issue only affects the blocking/polling test since they use 
a local perf_fd to open the stream. stream_fd is closed inside perf_open 
for each new test, so other tests using stream_fd can assert before 
closing the stream.

>
>> 3) We cannot guarantee latency when reading OA reports as there are no
>>    guarantees how long it takes for reports to land in memory w.r.t
>>    any running workloads. Since OA reports are mostly used for
>>    post-processing, drop the latency checks in these tests.
>>
>> Signed-off-by: Umesh Nerlige Ramappa <[email protected]>
>> ---
>>  tests/intel/xe_oa.c | 153 +++++---------------------------------------
>>  1 file changed, 17 insertions(+), 136 deletions(-)
>>
>> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> index 4f652b187bae..9ebb18690ec7 100644
>> --- a/tests/intel/xe_oa.c
>> +++ b/tests/intel/xe_oa.c
>> @@ -2127,8 +2127,6 @@ get_time(void)
>>   * kernelspace.
>>   */
>>  static void test_blocking(uint64_t requested_oa_period,
>> -			  bool set_kernel_hrtimer,
>> -			  uint64_t kernel_hrtimer,
>>			  const struct drm_xe_oa_unit *oau)
>>  {
>>	int oa_exponent = max_oa_exponent_for_period_lte(requested_oa_period);
>> @@ -2142,24 +2140,9 @@ static void test_blocking(uint64_t requested_oa_period,
>>	int64_t user_ns, kernel_ns;
>>	int64_t tick_ns = 1000000000 / sysconf(_SC_CLK_TCK);
>>	int64_t test_duration_ns = tick_ns * 100;
>> -	int max_iterations = (test_duration_ns / oa_period) + 2;
>> -	int n_extra_iterations = 0;
>>	int perf_fd;
>> -
>> -	/* It's a bit tricky to put a lower limit here, but we expect a
>> -	 * relatively low latency for seeing reports, while we don't currently
>> -	 * give any control over this in the api.
>> -	 *
>> -	 * We assume a maximum latency of 6 millisecond to deliver a POLLIN and
>> -	 * read() after a new sample is written (46ms per iteration) considering
>> -	 * the knowledge that that the driver uses a 200Hz hrtimer (5ms period)
>> -	 * to check for data and giving some time to read().
>> -	 */
>> -	int min_iterations = (test_duration_ns / (oa_period + kernel_hrtimer + kernel_hrtimer / 5));
>>	int64_t start, end;
>> -	int n = 0;
>>	struct intel_xe_perf_metric_set *test_set = oa_unit_metric_set(oau);
>> -	size_t format_size = get_oa_format(test_set->perf_oa_format).size;
>>
>>	ADD_PROPS(props, idx, SAMPLE_OA, true);
>>	ADD_PROPS(props, idx, OA_METRIC_SET, test_set->perf_oa_metrics_set);
>> @@ -2174,13 +2157,8 @@ static void test_blocking(uint64_t requested_oa_period,
>>	perf_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>>          set_fd_flags(perf_fd, O_CLOEXEC);
>>
>> -	times(&start_times);
>> -
>> -	igt_debug("tick length = %dns, test duration = %"PRIu64"ns, min iter. = %d,"
>> -		  " estimated max iter. = %d, oa_period = %s\n",
>> -		  (int)tick_ns, test_duration_ns,
>> -		  min_iterations, max_iterations,
>> -		  pretty_print_oa_period(oa_period));
>> +	igt_debug("tick length = %dns, test duration = %"PRIu64"ns, oa_period = %s\n",
>> +		  (int)tick_ns, test_duration_ns, pretty_print_oa_period(oa_period));
>>
>>	/* In the loop we perform blocking polls while the HW is sampling at
>>	 * ~25Hz, with the expectation that we spend most of our time blocked
>> @@ -2204,44 +2182,26 @@ static void test_blocking(uint64_t requested_oa_period,
>>	 * the error delta.
>>	 */
>>	start = get_time();
>> +	times(&start_times);
>>	do_ioctl(perf_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>> -	for (/* nop */; ((end = get_time()) - start) < test_duration_ns; /* nop */) {
>> -		bool timer_report_read = false;
>> -		bool non_timer_report_read = false;
>> +	while (((end = get_time()) - start) < test_duration_ns) {
>>		int ret;
>>
>>		while ((ret = read(perf_fd, buf, sizeof(buf))) < 0 &&
>>		       (errno == EINTR || errno == EIO))
>>			;
>>		igt_assert_lt(0, ret);
>> -
>> -		for (int offset = 0; offset < ret; offset += format_size) {
>> -			uint32_t *report = (void *)(buf + offset);
>> -
>> -			if (oa_report_is_periodic(report))
>> -				timer_report_read = true;
>> -			else
>> -				non_timer_report_read = true;
>> -		}
>> -
>> -		if (non_timer_report_read && !timer_report_read)
>> -			n_extra_iterations++;
>> -
>> -		n++;
>>	}
>> -
>>	times(&end_times);
>>
>> +	__perf_close(perf_fd);
>> +
>>	/* Using nanosecond units is fairly silly here, given the tick in-
>>	 * precision - ah well, it's consistent with the get_time() units.
>>	 */
>>	user_ns = (end_times.tms_utime - start_times.tms_utime) * tick_ns;
>>	kernel_ns = (end_times.tms_stime - start_times.tms_stime) * tick_ns;
>>
>> -	igt_debug("%d blocking reads during test with %"PRIu64" Hz OA sampling (expect no more than %d)\n",
>> -		  n, NSEC_PER_SEC / oa_period, max_iterations);
>> -	igt_debug("%d extra iterations seen, not related to periodic sampling (e.g. context switches)\n",
>> -		  n_extra_iterations);
>>	igt_debug("time in userspace = %"PRIu64"ns (+-%dns) (start utime = %d, end = %d)\n",
>>		  user_ns, (int)tick_ns,
>>		  (int)start_times.tms_utime, (int)end_times.tms_utime);
>> @@ -2249,20 +2209,7 @@ static void test_blocking(uint64_t requested_oa_period,
>>		  kernel_ns, (int)tick_ns,
>>		  (int)start_times.tms_stime, (int)end_times.tms_stime);
>>
>> -	/* With completely broken blocking (but also not returning an error) we
>> -	 * could end up with an open loop,
>> -	 */
>> -	igt_assert_lte(n, (max_iterations + n_extra_iterations));
>> -
>> -	/* Make sure the driver is reporting new samples with a reasonably
>> -	 * low latency...
>> -	 */
>> -	igt_assert_lt((min_iterations + n_extra_iterations), n);
>> -
>> -	if (!set_kernel_hrtimer)
>> -		igt_assert(kernel_ns <= (test_duration_ns / 100ull));
>> -
>> -	__perf_close(perf_fd);
>> +	igt_assert(kernel_ns <= (test_duration_ns / 100ull));
>
>This check (and the identical check in the polling test) is also sort of
>arbitrary. Should we remove even this? Anyway, it's ok to leave if it is
>passing everywhere. Therefore lgtm:

I wanted to leave it since it still checks that CPU is minimally used in 
these cases. Good to catch any regressions in OA implementation if we do 
something CPU heavy.

>
>Reviewed-by: Ashutosh Dixit <[email protected]>

Thanks,
Umesh