From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <3f62360e-ed2c-240d-c9c9-f984bf77f3e0@intel.com> Date: Tue, 13 Jun 2023 18:01:27 -0700 Content-Language: en-US To: "Dixit, Ashutosh" References: <20230612194213.528058-1-vinay.belgaumkar@intel.com> <874jnbyrz7.wl-ashutosh.dixit@intel.com> From: "Belgaumkar, Vinay" In-Reply-To: <874jnbyrz7.wl-ashutosh.dixit@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [igt-dev] [Intel-gfx] [PATCH v2 i-g-t] tests/i915_pm_freq_api: Add a suspend subtest List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org, intel-gfx@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 6/13/2023 2:25 PM, Dixit, Ashutosh wrote: > 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 >> --- >> 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? Don't think we need it. The delay is specifically for H2G calls. I haven't seen the need for a delay here in the limited testing I have done. > >> + >> + 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? I believe this is a blocking call and will only return after resume completes (when console comes back), so delay is not needed. >> - 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. What if this is a multi-card device? Though, one thing missing here is the 'free' for the allocations. Will add that. > >> + >> +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. If we fail, it means the api is not working, so we should flag an error. > >> + } >> + 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.? yes, something like that. We don't want to run 50 resets every time BAT runs. Thanks, Vinay. > >> + >> + 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