Re: [PATCH i-g-t v2 07/14] tests/intel/xe_oa: Rewrite the polling small buf test

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

On Mon, 03 Mar 2025 15:14:55 -0800, Umesh Nerlige Ramappa wrote:
>
> The polling-small-buf test is specifically testing that POLLIN is still
> set after an ENOSPC error. In the buggy code, POLLIN was cleared on
> the ENOSPC error even though there was data to be read. The test was
> putting an upper bound on the number of bytes read in a fixed duration
> of time. While this worked, we had occassional failures that were due to
> scheduling and other dependencies outside the scope of this test.
>
> In an attempt to make the test more robust,
> - use a more definitive method to wait for few reports
> - force an ENOSPC error using a small buffer
> - Ensure POLLIN is still set in the subsequent poll() call
>
> v2:
> - Add "why" in commit message (Ashutosh)
> - Use uapi to wait for N reports

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  tests/intel/xe_oa.c | 53 ++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 30 deletions(-)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 08406ca5c..dd16b39a9 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -2216,7 +2216,6 @@ static void test_polling(uint64_t requested_oa_period,
>   */
>  static void test_polling_small_buf(void)
>  {
> -	int oa_exponent = max_oa_exponent_for_period_lte(40 * 1000); /* 40us */
>	uint64_t properties[] = {
>		DRM_XE_OA_PROPERTY_OA_UNIT_ID, 0,
>
> @@ -2226,50 +2225,44 @@ static void test_polling_small_buf(void)
>		/* OA unit configuration */
>		DRM_XE_OA_PROPERTY_OA_METRIC_SET, default_test_set->perf_oa_metrics_set,
>		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(default_test_set->perf_oa_format),
> -		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, oa_exponent,
> +		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, oa_exponent_default,
> +		DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS, 5,
>		DRM_XE_OA_PROPERTY_OA_DISABLED, true,
>	};
>	struct intel_xe_oa_open_prop param = {
>		.num_properties = ARRAY_SIZE(properties) / 2,
>		.properties_ptr = to_user_pointer(properties),
>	};
> -	uint32_t test_duration = 80 * 1000 * 1000;
> -	int sample_size = get_oa_format(default_test_set->perf_oa_format).size;
> -	int n_expected_reports = test_duration / oa_exponent_to_ns(oa_exponent);
> -	int n_expect_read_bytes = n_expected_reports * sample_size;
> -	struct timespec ts = {};
> -	int n_bytes_read = 0;
> -	uint32_t n_polls = 0;
> +	struct pollfd pollfd;
> +	uint8_t buf[10];
> +	int ret;
>
>	stream_fd = __perf_open(drm_fd, &param, true /* prevent_pm */);
>	set_fd_flags(stream_fd, O_CLOEXEC | O_NONBLOCK);
> -	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>
> -	while (igt_nsec_elapsed(&ts) < test_duration) {
> -		struct pollfd pollfd = { .fd = stream_fd, .events = POLLIN };
> -
> -		ppoll(&pollfd, 1, NULL, NULL);
> -		if (pollfd.revents & POLLIN) {
> -			uint8_t buf[1024];
> -			int ret;
> +	/* Kickstart the capture */
> +	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>
> -			ret = read(stream_fd, buf, sizeof(buf));
> -			if (ret >= 0)
> -				n_bytes_read += ret;
> -		}
> +	/*
> +	 * Wait for number of reports specified in
> +	 * DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS
> +	 */
> +	pollfd.fd = stream_fd;
> +	pollfd.events = POLLIN;
> +	poll(&pollfd, 1, -1);
> +	igt_assert(pollfd.revents & POLLIN);
>
> -		n_polls++;
> -	}
> +	/* Just read one report and expect ENOSPC */
> +	errno = 0;
> +	ret = read(stream_fd, buf, sizeof(buf));
> +	igt_assert_eq(ret, -1);
> +	igt_assert_eq(errno, ENOSPC);
>
> -	igt_info("Read %d expected %d (%.2f%% of the expected number), polls=%u\n",
> -		 n_bytes_read, n_expect_read_bytes,
> -		 n_bytes_read * 100.0f / n_expect_read_bytes,
> -		 n_polls);
> +	/* Poll with 0 timeout and expect POLLIN flag to be set */
> +	poll(&pollfd, 1, 0);
> +	igt_assert(pollfd.revents & POLLIN);
>
>	__perf_close(stream_fd);
> -
> -	igt_assert(abs(n_expect_read_bytes - n_bytes_read) <
> -		   0.20 * n_expect_read_bytes);
>  }
>
>  static int
> --
> 2.34.1
>