[igt-dev] [Intel-gfx] [PATCH v2 i-g-t] tests/i915_pm_freq_api: Add a suspend subtest
Belgaumkar, Vinay
vinay.belgaumkar at intel.com
Wed Jun 14 01:01:27 UTC 2023
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 <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?
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
More information about the igt-dev
mailing list