[igt-dev] [Intel-gfx] [PATCH v2 i-g-t] tests/i915_pm_freq_api: Add a suspend subtest

[Dixit, Ashutosh]

On Mon, 12 Jun 2023 12:42:13 -0700, Vinay Belgaumkar wrote:
>

Hi Vinay,

> Verify that SLPC API works as expected after a suspend. Added
> another subtest that does multiple GT resets and checks freq api
> works as expected after each one.
>
> We now check requested frequency instead of soft min/max after a
> reset or suspend. That ensures the soft limits got applied
> correctly at init. Also, disable efficient freq before starting the
> test which allows current freq to be consistent with SLPC min freq.
>
> v2: Restore freq in exit handler (Ashutosh)
>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> ---
>  tests/i915/i915_pm_freq_api.c | 89 +++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/tests/i915/i915_pm_freq_api.c b/tests/i915/i915_pm_freq_api.c
> index 9005cd220..4e1d4edca 100644
> --- a/tests/i915/i915_pm_freq_api.c
> +++ b/tests/i915/i915_pm_freq_api.c
> @@ -18,6 +18,12 @@
>   *
>   * SUBTEST: freq-reset
>   * Description: Test basic freq API works after a reset
> + *
> + * SUBTEST: freq-reset-multiple
> + * Description: Test basic freq API works after multiple resets
> + *
> + * SUBTEST: freq-suspend
> + * Description: Test basic freq API works after a runtime suspend
>   */
>
>  IGT_TEST_DESCRIPTION("Test SLPC freq API");
> @@ -79,31 +85,64 @@ static void test_freq_basic_api(int dirfd, int gt)
>
>  }
>
> -static void test_reset(int i915, int dirfd, int gt)
> +static void test_reset(int i915, int dirfd, int gt, int count)
>  {
>	uint32_t rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
>	int fd;
>
> +	for (int i = 0; i < count; i++) {
> +		igt_assert_f(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0,
> +			     "Failed after %d good cycles\n", i);
> +		igt_assert_f(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0,
> +			     "Failed after %d good cycles\n", i);
> +		usleep(ACT_FREQ_LATENCY_US);
> +		igt_assert_f(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn,
> +			     "Failed after %d good cycles\n", i);
> +
> +		/* Manually trigger a GT reset */
> +		fd = igt_debugfs_gt_open(i915, gt, "reset", O_WRONLY);
> +		igt_require(fd >= 0);
> +		igt_ignore_warn(write(fd, "1\n", 2));

No need for 'usleep(ACT_FREQ_LATENCY_US)' here?

> +
> +		igt_assert_f(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn,
> +			     "Failed after %d good cycles\n", i);
> +	}
> +	close(fd);
> +}
> +
> +static void test_suspend(int i915, int dirfd, int gt)
> +{
> +	uint32_t rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
> +
>	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0);
>	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0);
>	usleep(ACT_FREQ_LATENCY_US);
> -	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
> +	igt_assert(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn);
>
> -	/* Manually trigger a GT reset */
> -	fd = igt_debugfs_gt_open(i915, gt, "reset", O_WRONLY);
> -	igt_require(fd >= 0);
> -	igt_ignore_warn(write(fd, "1\n", 2));
> -	close(fd);
> +	/* Manually trigger a suspend */
> +	igt_system_suspend_autoresume(SUSPEND_STATE_S3,
> +				      SUSPEND_TEST_NONE);

No need for 'usleep(ACT_FREQ_LATENCY_US)' here?

>
> -	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
> -	igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpn);
> +	igt_assert(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn);
>  }
>
> -igt_main
> +int i915 = -1;
> +uint32_t *stash_min, *stash_max;

nit: could we maybe make these fixed size array's (2 or 4 entries) and drop
the malloc's for these, malloc's seem excessive in this case.

> +
> +static void restore_sysfs_freq(int sig)
>  {
> -	int i915 = -1;
> -	uint32_t *stash_min, *stash_max;
> +	int dirfd, gt;
> +	/* Restore frequencies */
> +	for_each_sysfs_gt_dirfd(i915, dirfd, gt) {
> +		igt_pm_ignore_slpc_efficient_freq(i915, dirfd, false);
> +		igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, stash_max[gt]) > 0);
> +		igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, stash_min[gt]) > 0);

nit: I would remove the igt_assert's from here, it's basically a best
effort restore so we try to restore everything even if we fail.

> +	}
> +	close(i915);
> +}
>
> +igt_main
> +{
>	igt_fixture {
>		int num_gts, dirfd, gt;
>
> @@ -122,7 +161,9 @@ igt_main
>		for_each_sysfs_gt_dirfd(i915, dirfd, gt) {
>			stash_min[gt] = get_freq(dirfd, RPS_MIN_FREQ_MHZ);
>			stash_max[gt] = get_freq(dirfd, RPS_MAX_FREQ_MHZ);
> +			igt_pm_ignore_slpc_efficient_freq(i915, dirfd, true);
>		}
> +		igt_install_exit_handler(restore_sysfs_freq);
>	}
>
>	igt_describe("Test basic API for controlling min/max GT frequency");
> @@ -140,16 +181,24 @@ igt_main
>
>		for_each_sysfs_gt_dirfd(i915, dirfd, gt)
>			igt_dynamic_f("gt%u", gt)
> -				test_reset(i915, dirfd, gt);
> +				test_reset(i915, dirfd, gt, 1);
>	}
>
> -	igt_fixture {
> +	igt_describe("Test basic freq API works after multiple resets");
> +	igt_subtest_with_dynamic_f("freq-reset-multiple") {
>		int dirfd, gt;
> -		/* Restore frequencies */
> -		for_each_sysfs_gt_dirfd(i915, dirfd, gt) {
> -			igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, stash_max[gt]) > 0);
> -			igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, stash_min[gt]) > 0);
> -		}
> -		close(i915);
> +
> +		for_each_sysfs_gt_dirfd(i915, dirfd, gt)
> +			igt_dynamic_f("gt%u", gt)
> +				test_reset(i915, dirfd, gt, 50);
> +	}

Do we need both "freq-reset" and "freq-reset-multiple"? Since
"freq-reset" is a subset of "freq-reset-multiple"? Or we want "freq-reset"
to run as part of BAT and "freq-reset-multiple" as part of shards e.g.?

> +
> +	igt_describe("Test basic freq API works after suspend");
> +	igt_subtest_with_dynamic_f("freq-suspend") {
> +		int dirfd, gt;
> +
> +		for_each_sysfs_gt_dirfd(i915, dirfd, gt)
> +			igt_dynamic_f("gt%u", gt)
> +				test_suspend(i915, dirfd, gt);
>	}
>  }
> --
> 2.38.1
>

Thanks.
--
Ashutosh