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

[Harish Chegondi <harish.chegondi@intel.com>]

On Wed, Jun 04, 2025 at 03:34:16PM -0700, Dixit, Ashutosh wrote:
> 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...
Will remove.
> 
> > +			      "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).
I am moving the query IOCTL into the lib. Probably those changes should
go into a separate patch?
> 
> 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.
I initially used igt_require_f, but later changed it to igt_skip_on_f
as the condition logic is simpler with igt_skip_on_f.
> 
> >		if (output_file) {
> >			output = fopen(output_file, "w");
> >			igt_require(output);
> > --
> > 2.48.1
> >
Thank You
Harish.