Re: [igt-dev] [PATCH i-g-t v2] tests/xe: Verify actual frequency on the basis of GT state

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

On Mon, 17 Jul 2023 03:57:11 -0700, Badal Nilawar wrote:
>

Hi Badal,

> diff --git a/tests/xe/xe_guc_pc.c b/tests/xe/xe_guc_pc.c
> index c34df8d60..ff1b32613 100644
> --- a/tests/xe/xe_guc_pc.c
> +++ b/tests/xe/xe_guc_pc.c
> @@ -218,7 +218,7 @@ static void test_freq_basic_api(int fd, int gt_id)
>   * Run type: FULL
>   */
>
> -static void test_freq_fixed(int fd, int gt_id)
> +static void test_freq_fixed(int fd, int gt_id, bool gt_idle)

We could try to "deduce" the value of gt_idle (depending on whether gt is
in rc6 or not) but the gt_idle flag is better.


>  {
>	uint32_t rpn = get_freq(fd, gt_id, "rpn");
>	uint32_t rpe = get_freq(fd, gt_id, "rpe");
> @@ -226,6 +226,9 @@ static void test_freq_fixed(int fd, int gt_id)
>
>	igt_debug("Starting testing fixed request\n");
>
> +	if (gt_idle)
> +		igt_require(igt_wait(xe_is_gt_in_c6(fd, gt_id), 1000, 1));
> +
>	/*
>	 * For Fixed freq we need to set both min and max to the desired value
>	 * Then we check if hardware is actually operating at the desired freq
> @@ -235,13 +238,25 @@ static void test_freq_fixed(int fd, int gt_id)
>	igt_assert(set_freq(fd, gt_id, "max", rpn) > 0);
>	usleep(ACT_FREQ_LATENCY_US);
>	igt_assert(get_freq(fd, gt_id, "cur") == rpn);
> -	igt_assert(get_freq(fd, gt_id, "act") == rpn);
> +
> +	if (gt_idle) {
> +		usleep(ACT_FREQ_LATENCY_US);

Looks like this usleep should in the else? Since we've already igt_wait'ed
above for the idle case? In all places in this patch.

> +		igt_assert(get_freq(fd, gt_id, "act") == 0);
> +	} else {
> +		igt_assert(get_freq(fd, gt_id, "act") == rpn);
> +	}
>
>	igt_assert(set_freq(fd, gt_id, "min", rpe) > 0);
>	igt_assert(set_freq(fd, gt_id, "max", rpe) > 0);
>	usleep(ACT_FREQ_LATENCY_US);
>	igt_assert(get_freq(fd, gt_id, "cur") == rpe);
> -	igt_assert(get_freq(fd, gt_id, "act") == rpe);
> +
> +	if (gt_idle) {
> +		usleep(ACT_FREQ_LATENCY_US);
> +		igt_assert(get_freq(fd, gt_id, "act") == 0);
> +	} else {
> +		igt_assert(get_freq(fd, gt_id, "act") == rpe);
> +	}
>
>	igt_assert(set_freq(fd, gt_id, "min", rp0) > 0);
>	igt_assert(set_freq(fd, gt_id, "max", rp0) > 0);

We can't check for 0 act freq in this rp0 case?

> diff --git a/tests/xe/xe_pm_residency.c b/tests/xe/xe_pm_residency.c
> index a20c4449c..5c4516d03 100644
> --- a/tests/xe/xe_pm_residency.c
> +++ b/tests/xe/xe_pm_residency.c
> @@ -28,6 +28,16 @@ const double tolerance = 0.1;
>		     (tol) * 100.0, (tol) * 100.0, \
>		     (double)(ref))
>
> +/**
> + * SUBTEST: gt-c6-on-idle
> + * Description: Validate GT C6 state on idle
> + * Run type: BAT
> + *
> + * SUBTEST: idle-residency
> + * Description: basic residency test to validate idle residency
> + *		measured over a time interval is within the tolerance
> + * Run type: FULL
> + */

Why combine these two doc sections? idle-residency description should be
above test_idle_residency?

>  IGT_TEST_DESCRIPTION("Tests for gtidle properties");
>
>  static unsigned int measured_usleep(unsigned int usec)
> @@ -45,24 +55,6 @@ static unsigned int measured_usleep(unsigned int usec)
>	return igt_nsec_elapsed(&ts) / 1000;
>  }
>
> -/**
> - * SUBTEST: gt-c6-on-idle
> - * Description: Validate GT C6 state on idle
> - * Run type: BAT
> - */
> -static bool is_gt_in_c6(int fd, int gt)
> -{
> -	char gt_c_state[16];
> -	int gt_fd;
> -
> -	gt_fd = xe_sysfs_gt_open(fd, gt);
> -	igt_assert(gt_fd >= 0);
> -	igt_assert(igt_sysfs_scanf(gt_fd, "gtidle/idle_status", "%s", gt_c_state) == 1);
> -	close(gt_fd);
> -
> -	return strcmp(gt_c_state, "gt-c6") == 0;
> -}
> -
>  static unsigned long read_idle_residency(int fd, int gt)
>  {
>	unsigned long residency = 0;
> @@ -76,17 +68,11 @@ static unsigned long read_idle_residency(int fd, int gt)
>	return residency;
>  }
>
> -/**
> - * SUBTEST: idle-residency
> - * Description: basic residency test to validate idle residency
> - *		measured over a time interval is within the tolerance
> - * Run type: FULL
> - */
>  static void test_idle_residency(int fd, int gt)
>  {
>	unsigned long elapsed_ms, residency_start, residency_end;
>
> -	igt_assert_f(igt_wait(is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> +	igt_assert_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
>
>	residency_start = read_idle_residency(fd, gt);
>	elapsed_ms = measured_usleep(SLEEP_DURATION * 1000) / 1000;
> @@ -110,7 +96,7 @@ igt_main
>	igt_describe("Validate GT C6 on idle");
>	igt_subtest("gt-c6-on-idle")
>		xe_for_each_gt(fd, gt)
> -			igt_assert_f(igt_wait(is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> +			igt_assert_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
>
>	igt_describe("Validate idle residency measured over a time interval is within the tolerance");
>	igt_subtest("idle-residency")
> --
> 2.25.1

Thanks.
--
Ashutosh