Re: [PATCH 1/1] tests/intel/xe_eu_stall: Add tests to run blocking and non-blocking read twice

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

On Tue, 22 Apr 2025 19:57:34 -0700, Harish Chegondi wrote:
>
> Add tests that enable EU stall sampling, read all the EU stall data and
> disable EU stall sampling, two times back to back. Add tests for both
> blocking and non-blocking reads. Also, add a check for the presence of
> /proc/sys/dev/xe/observation_paranoid file which is required for the
> unprivileged-access test.
>
> Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> ---
>  tests/intel/xe_eu_stall.c | 44 ++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/tests/intel/xe_eu_stall.c b/tests/intel/xe_eu_stall.c
> index da9bd7843..15d4589c6 100644
> --- a/tests/intel/xe_eu_stall.c
> +++ b/tests/intel/xe_eu_stall.c
> @@ -14,9 +14,15 @@
>   * SUBTEST: non-blocking-read
>   * Description: Verify non-blocking read of EU stall data during a workload run
>   *
> + * SUBTEST: non-blocking-read-twice
> + * Description: Run non-blocking read test twice with disable and enable between the runs
> + *
>   * SUBTEST: blocking-read
>   * Description: Verify blocking read of EU stall data during a workload run
>   *
> + * SUBTEST: blocking-read-twice
> + * Description: Run blocking read test twice with disable and enable between the runs
> + *

Not sure about the "read-twice" name, i.e. not sure if that name clearly
communicates the purpose of these new tests. The purpose to me seems that
reads can be done after a disable and enable, correct?

Maybe "blocking-enable-disable" and "non-blocking-enable-disable" are
better names?

Otherwise this is:

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

Let me know if you are going to change the names. Thanks.

>   * SUBTEST: unprivileged-access
>   * Description: Verify unprivileged open of a EU stall data stream fd
>   *
> @@ -33,6 +39,7 @@
>  #include <fcntl.h>
>  #include <poll.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  #include <sys/wait.h>
>
>  #include "igt.h"
> @@ -460,13 +467,13 @@ static void print_eu_stall_data(uint32_t devid, uint8_t *buf, size_t size)
>   * while the parent process reads the stall counters data, disables EU stall
>   * counters once the workload completes execution.
>   */
> -static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
> +static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read, int iter)
>  {
> -	uint32_t num_samples = 0, num_drops = 0;
> +	uint32_t num_samples, num_drops;
>	struct igt_helper_process work_load = {};
>	struct sigaction sa = { 0 };
>	int ret, flags, stream_fd;
> -	uint64_t total_size = 0;
> +	uint64_t total_size;
>	uint8_t *buf;
>
>	uint64_t properties[] = {
> @@ -520,7 +527,7 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
>		flags = O_CLOEXEC;
>
>	set_fd_flags(stream_fd, flags);
> -
> +enable:
>	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>
>	sa.sa_handler = sighandler;
> @@ -540,6 +547,9 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
>			_exit(run_gpgpu_fill(drm_fd, devid));
>		}
>	}
> +	total_size = 0;
> +	num_samples = 0;
> +	num_drops = 0;
>	/* Parent process reads the EU stall counters data */
>	do {
>		if (!blocking_read) {
> @@ -574,14 +584,16 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read)
>	igt_info("Number of samples: %u\n", num_samples);
>	igt_info("Number of drops reported: %u\n", num_drops);
>
> +	ret = wait_child(&work_load);
> +	igt_assert_f(ret == 0, "waitpid() - ret: %d, errno: %d\n", ret, errno);
> +	igt_assert_f(num_samples, "No EU stalls detected during the workload\n");
> +
>	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_DISABLE, 0);
> +	if (--iter)
> +		goto enable;
>
>	close(stream_fd);
>	free(buf);
> -
> -	ret = wait_child(&work_load);
> -	igt_assert_f(ret == 0, "waitpid() - ret: %d, errno: %d\n", ret, errno);
> -	igt_assert_f(num_samples, "No EU stalls detected during the workload\n");
>  }
>
>  static int opt_handler(int opt, int opt_index, void *data)
> @@ -634,6 +646,7 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>  {
>	int drm_fd;
>	uint32_t devid;
> +	struct stat sb;
>	bool blocking_read = true;
>
>	igt_fixture {
> @@ -642,6 +655,7 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>		devid = intel_get_drm_devid(drm_fd);
>		igt_require(IS_PONTEVECCHIO(devid) || intel_graphics_ver(devid) >= IP_VER(20, 0));
>		igt_require_f(igt_get_gpgpu_fillfunc(devid), "no gpgpu-fill function\n");
> +		igt_require(!stat(OBSERVATION_PARANOID, &sb));
>		if (output_file) {
>			output = fopen(output_file, "w");
>			igt_require(output);
> @@ -650,12 +664,22 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>
>	igt_describe("Verify non-blocking read of EU stall data during a workload run");
>	igt_subtest("non-blocking-read") {
> -		test_eustall(drm_fd, devid, !blocking_read);
> +		test_eustall(drm_fd, devid, !blocking_read, 1);
> +	}
> +
> +	igt_describe("Run non-blocking read test twice with disable and enable between the runs");
> +	igt_subtest("non-blocking-read-twice") {
> +		test_eustall(drm_fd, devid, !blocking_read, 2);
>	}
>
>	igt_describe("Verify blocking read of EU stall data during a workload run");
>	igt_subtest("blocking-read") {
> -		test_eustall(drm_fd, devid, blocking_read);
> +		test_eustall(drm_fd, devid, blocking_read, 1);
> +	}
> +
> +	igt_describe("Run blocking read test twice with disable and enable between the runs");
> +	igt_subtest("blocking-read-twice") {
> +		test_eustall(drm_fd, devid, blocking_read, 2);
>	}
>
>	igt_describe("Verify that unprivileged open of a EU stall data fd fails");
> --
> 2.48.1
>