Re: [PATCH i-g-t 3/3] tests/intel/xe_eu_stall: Use query IOCTL to check for EU stall support

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

On Tue, 03 Jun 2025 16:57:36 -0700, Harish Chegondi wrote:
>
> Use EU stall query IOCTL to check if EU stall monitoring feature is
> available on the platform and if it is supported in the driver.
> This would replace the checks in the test with those in the driver.
>
> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Adam Miszczak <adam.miszczak@linux.intel.com>
> Cc: Jakub Kolakowski <jakub1.kolakowski@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> Resolves: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5131
> ---
>  tests/intel/xe_eu_stall.c | 55 +++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/tests/intel/xe_eu_stall.c b/tests/intel/xe_eu_stall.c
> index b3a0ee742..4e76018b6 100644
> --- a/tests/intel/xe_eu_stall.c
> +++ b/tests/intel/xe_eu_stall.c
> @@ -74,6 +74,8 @@ static int stream_fd = -1;
>
>  static volatile bool child_is_running = true;
>
> +static struct drm_xe_query_eu_stall *query_eu_stall_data;
> +
>  /*
>   * EU stall data format for PVC
>   */
> @@ -506,33 +508,12 @@ static void test_eustall(int drm_fd, uint32_t devid, bool blocking_read, int ite
>		.properties_ptr = to_user_pointer(properties),
>	};
>
> -	struct drm_xe_query_eu_stall *query_eu_stall_data;
> -	struct drm_xe_device_query query = {
> -		.extensions = 0,
> -		.query = DRM_XE_DEVICE_QUERY_EU_STALL,
> -		.size = 0,
> -		.data = 0,
> -	};
> -
>	igt_info("User buffer size: %u\n", p_user);
>	if (p_args[0])
>		igt_info("Workload: %s\n", p_args[0]);
>	else
>		igt_info("Workload: GPGPU fill\n");
> -
> -	igt_assert_eq(igt_ioctl(drm_fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
> -	igt_assert_neq(query.size, 0);
> -
> -	query_eu_stall_data = malloc(query.size);
> -	igt_assert(query_eu_stall_data);
> -
> -	query.data = to_user_pointer(query_eu_stall_data);
> -	igt_assert_eq(igt_ioctl(drm_fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
> -
> -	igt_assert(query_eu_stall_data->num_sampling_rates > 0);
> -	if (p_rate == 0)
> -		properties[3] = query_eu_stall_data->sampling_rates[0];
> -	igt_info("Sampling Rate: %lu\n", properties[3]);
> +	igt_info("Sampling Rate: %lu\n", p_rate);
>
>	stream_fd = eu_stall_open(drm_fd, &props);
>
> @@ -660,18 +641,42 @@ static struct option long_options[] = {
>
>  igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
>  {
> -	int drm_fd;
> +	bool blocking_read = true;
> +	int drm_fd, ret;
>	uint32_t devid;
>	struct stat sb;
> -	bool blocking_read = true;
> +	struct drm_xe_device_query query = {
> +		.extensions = 0,
> +		.query = DRM_XE_DEVICE_QUERY_EU_STALL,
> +		.size = 0,
> +		.data = 0,
> +	};
>
>	igt_fixture {
>		drm_fd = drm_open_driver(DRIVER_XE);
>		igt_require_fd(drm_fd);
>		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_f(!stat(OBSERVATION_PARANOID, &sb), "no observation_paranoid file\n");
> +
> +		ret = igt_ioctl(drm_fd, DRM_IOCTL_XE_DEVICE_QUERY, &query);
> +		igt_skip_on_f((ret == -1 && errno == ENODEV),
> +			      "EU stall monitoring is not available on this platform\n");
> +		igt_skip_on_f((ret == -1 && errno == EINVAL),

inner brackets not needed...

> +			      "EU stall monitoring is not supported in the driver\n");
> +		igt_assert_neq(query.size, 0);
> +
> +		query_eu_stall_data = malloc(query.size);
> +		igt_assert(query_eu_stall_data);
> +
> +		query.data = to_user_pointer(query_eu_stall_data);
> +		igt_assert_eq(igt_ioctl(drm_fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
> +
> +		igt_assert(query_eu_stall_data->num_sampling_rates > 0);
> +		if (p_rate == 0)
> +			p_rate = query_eu_stall_data->sampling_rates[0];
> +

Maybe cleaner to put all this query stuff into a function and calling the
function from the fixture? Also, queries are generally cached in 'struct
xe_device' so we could do the same here? See xe_device_get() and also
xe_query_oa_units_new(). Since that's in the lib, make sure IGT still works
even if the eu_stall query fails (see 851e62faf971).

Also, another nit, use igt_require or igt_require_f instead of
igt_skip_on_f? Though they are mostly NOT of each other so they are
equivalent, so either is ok.

>		if (output_file) {
>			output = fopen(output_file, "w");
>			igt_require(output);
> --
> 2.48.1
>