Re: [PATCH 1/1] tests/intel/xe_eu_stall: Add invalid input tests for EU stall sampling

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

On Thu, 27 Mar 2025 10:35:52 -0700, Dixit, Ashutosh wrote:
>
> On Thu, 27 Mar 2025 02:35:10 -0700, Harish Chegondi wrote:
> >
> > Add tests to verify that invalid inputs fail the tests
> >
> > Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> > ---
> >  tests/intel/xe_eu_stall.c | 64 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/tests/intel/xe_eu_stall.c b/tests/intel/xe_eu_stall.c
> > index 964bb7c91..8f892a9ae 100644
> > --- a/tests/intel/xe_eu_stall.c
> > +++ b/tests/intel/xe_eu_stall.c
> > @@ -19,6 +19,15 @@
> >   *
> >   * SUBTEST: unprivileged-access
> >   * Description: Verify unprivileged open of a EU stall data stream fd
> > + *
> > + * SUBTEST: invalid-gt-id
> > + * Description: Verify that invalid input GT ID fails the test
> > + *
> > + * SUBTEST: invalid-sampling-rate
> > + * Description: Verify that invalid input sampling rate fails the test
> > + *
> > + * SUBTEST: invalid-event-report-count
> > + * Description: Verify that invalid input event report count fails the test
> >   */
> >
> >  #include <fcntl.h>
> > @@ -33,6 +42,9 @@
> >
> >  #define OBSERVATION_PARANOID	"/proc/sys/dev/xe/observation_paranoid"
> >
> > +#define NUM_DATA_ROWS(SIZE)	((SIZE) >> 6)
>
> What is SIZE?

Sorry, don't use all caps for args.

>
> > +
> > +#define MAX_SUBSLICES		64
>
> What is a SUBSLICE? Is it a XeCore?
>
> >  #define NUM_ITERS_GPGPU_FILL	100
> >  #define DEFAULT_NUM_REPORTS	1
> >  #define DEFAULT_SAMPLE_RATE	(251 * 4)
> > @@ -283,6 +295,46 @@ static void set_fd_flags(int fd, int flags)
> >	igt_assert_eq(0, fcntl(fd, F_SETFL, old | flags));
> >  }
> >
> > +/*
> > + * Verify that tests with invalid arguments fail.
> > + */
> > +static void test_invalid_arguments(int drm_fd, uint8_t gt_id, uint32_t rate, uint32_t num_reports)
> > +{
> > +	uint64_t properties[] = {
> > +		DRM_XE_EU_STALL_PROP_GT_ID, gt_id,
> > +		DRM_XE_EU_STALL_PROP_SAMPLE_RATE, rate,
> > +		DRM_XE_EU_STALL_PROP_WAIT_NUM_REPORTS, num_reports,
> > +	};
> > +
> > +	struct xe_eu_stall_open_prop props = {
> > +		.num_properties = ARRAY_SIZE(properties) / 2,
> > +		.properties_ptr = to_user_pointer(properties),
> > +	};
> > +
> > +	xe_eu_stall_ioctl_err(drm_fd, DRM_XE_OBSERVATION_OP_STREAM_OPEN, &props, EINVAL);
> > +}
> > +
> > +static void test_invalid_gt_id(int fd)
> > +{
> > +	test_invalid_arguments(fd, 255,
> > +				   DEFAULT_SAMPLE_RATE,
> > +				   DEFAULT_NUM_REPORTS);
> > +}
> > +
> > +static void test_invalid_sampling_rate(int fd)
> > +{
> > +	test_invalid_arguments(fd, p_gt_id,
>
> Instead of using p_gt_id, maybe just use 0 here. Sometime p_* are used,
> sometimes DEFAUL_* are used, it's annoying.
>
> For example, at the top of the file where the global's are defined, why is
> p_rate not assigned to DEFAULT_SAMPLE_RATE?
>
> > +				   (251 * 10),

Why brackets?

> > +				   DEFAULT_NUM_REPORTS);
> > +}
> > +
> > +static void test_invalid_event_report_count(int fd)
> > +{
> > +	test_invalid_arguments(fd, p_gt_id,
> > +				   DEFAULT_SAMPLE_RATE,
> > +				   (NUM_DATA_ROWS(512 * 1024) * MAX_SUBSLICES) + 1);
>
> Also the indentation is off. Things should be aligned to the opening '('.

Also why brackets.

>
> > +}
> > +
> >  static inline void enable_paranoid(void)
> >  {
> >	write_u64_file(OBSERVATION_PARANOID, 1);
> > @@ -615,6 +667,18 @@ igt_main_args("e:g:o:r:u:w:", long_options, help_str, opt_handler, NULL)
> >	igt_subtest("unprivileged-access")
> >		test_non_privileged_access(drm_fd);
> >
> > +	igt_describe("Verify that invalid input GT ID fails the test");
> > +	igt_subtest("invalid-gt-id")
> > +		test_invalid_gt_id(drm_fd);
> > +
> > +	igt_describe("Verify that invalid input sampling rate fails the test");
> > +	igt_subtest("invalid-sampling-rate")
> > +		test_invalid_sampling_rate(drm_fd);
> > +
> > +	igt_describe("Verify that invalid input event report count fails the test");
> > +	igt_subtest("invalid-event-report-count")
> > +		test_invalid_event_report_count(drm_fd);
> > +
> >	igt_fixture {
> >		if (output)
> >			fclose(output);
> > --
> > 2.48.1
> >