Re: [PATCH i-g-t 2/3] tests/intel/xe_eu_stall: Do not check for presence of data on simulation

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

On Tue, 03 Jun 2025 16:57:35 -0700, Harish Chegondi wrote:
>
> Some simulation models may not have full EU stall sampling support.
>
> v2: 1. Make EU stall fd global and close any open fd before open (Ashutosh)

This is probably patch 1/3, so no need to mention here I think.

>     2. Move disable IOCTL call to before the assert.
>     3. Allocate and free the user buffer in the main routine to prevent
>        any memory leak due to an assert.

3 is probably not worth it since any allocated memory will be freed when
the process exits, so the memory "leak" is only for the duration the
process is running after the assert.

Generally multiple changes like these should be sent in separate patches to
make review easier, but in any case, here it is obvious what's happening so
this is:

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

>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> ---
>  tests/intel/xe_eu_stall.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tests/intel/xe_eu_stall.c b/tests/intel/xe_eu_stall.c
> index 1499ec4b4..b3a0ee742 100644
> --- a/tests/intel/xe_eu_stall.c
> +++ b/tests/intel/xe_eu_stall.c
> @@ -65,6 +65,7 @@
>  static FILE *output;
>  static char *p_args[8];
>  static char *output_file;
> +static uint8_t *user_buf;
>  static uint8_t p_gt_id;
>  static uint32_t p_rate;
>  static uint32_t p_user = DEFAULT_USER_BUF_SIZE;
> @@ -493,7 +494,6 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read, int ite
>	struct sigaction sa = { 0 };
>	int ret, flags;
>	uint64_t total_size;
> -	uint8_t *buf;
>
>	uint64_t properties[] = {
>		DRM_XE_EU_STALL_PROP_GT_ID, p_gt_id,
> @@ -520,9 +520,6 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read, int ite
>	else
>		igt_info("Workload: GPGPU fill\n");
>
> -	buf = malloc(p_user);
> -	igt_assert(buf);
> -
>	igt_assert_eq(igt_ioctl(drm_fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
>	igt_assert_neq(query.size, 0);
>
> @@ -579,11 +576,11 @@ enable:
>			igt_assert_eq(ret, 1);
>			igt_assert(pollfd.revents & POLLIN);
>		}
> -		ret = read(stream_fd, buf, p_user);
> +		ret = read(stream_fd, user_buf, p_user);
>		if (ret > 0) {
>			total_size += ret;
>			if (output)
> -				print_eu_stall_data(devid, buf, ret);
> +				print_eu_stall_data(devid, user_buf, ret);
>			num_samples += ret / query_eu_stall_data->record_size;
>		} else if ((ret < 0) && (errno != EAGAIN)) {
>			if (errno == EINTR)
> @@ -598,20 +595,21 @@ enable:
>		}
>	} while (child_is_running);
>
> +	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_DISABLE, 0);
> +
>	igt_info("Total size read: %lu\n", total_size);
>	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");
> +	if (!igt_run_in_simulation())
> +		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;
>
>	eu_stall_close(stream_fd);
> -	free(buf);
>  }
>
>  static int opt_handler(int opt, int opt_index, void *data)
> @@ -678,6 +676,8 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>			output = fopen(output_file, "w");
>			igt_require(output);
>		}
> +		user_buf = malloc(p_user);
> +		igt_assert(user_buf);
>	}
>
>	igt_describe("Verify non-blocking read of EU stall data during a workload run");
> @@ -717,6 +717,7 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>		test_invalid_event_report_count(drm_fd);
>
>	igt_fixture {
> +		free(user_buf);
>		if (output)
>			fclose(output);
>		drm_close_driver(drm_fd);
> --
> 2.48.1
>