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 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?

> +
> +#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),
> +				   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 '('.

> +}
> +
>  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
>