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

[Dixit, Ashutosh]

On Wed, 07 Jun 2023 16:40:53 -0700, Belgaumkar, Vinay wrote:
>
>
> On 6/7/2023 4:11 PM, Belgaumkar, Vinay wrote:
> >
> > On 6/7/2023 3:56 PM, Dixit, Ashutosh wrote:
> >> On Wed, 07 Jun 2023 15:31:33 -0700, Belgaumkar, Vinay wrote:
> >>> On 6/7/2023 2:12 PM, Dixit, Ashutosh wrote:
> >>>> On Tue, 06 Jun 2023 13:35:35 -0700, Vinay Belgaumkar wrote:
> >>>> Hi Vinay,
> >>>>
> >>>>> Verify that SLPC API works as expected after a suspend.
> >>>>>
> >>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> >>>>> ---
> >>>>>    tests/i915/i915_pm_freq_api.c | 30 ++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 30 insertions(+)
> >>>>>
> >>>>> diff --git a/tests/i915/i915_pm_freq_api.c
> >>>>> b/tests/i915/i915_pm_freq_api.c
> >>>>> index 9005cd220..f35f1f8e0 100644
> >>>>> --- a/tests/i915/i915_pm_freq_api.c
> >>>>> +++ b/tests/i915/i915_pm_freq_api.c
> >>>>> @@ -18,6 +18,9 @@
> >>>>>     *
> >>>>>     * SUBTEST: freq-reset
> >>>>>     * Description: Test basic freq API works after a reset
> >>>>> + *
> >>>>> + * SUBTEST: freq-suspend
> >>>>> + * Description: Test basic freq API works after a runtime suspend
> >>>>>     */
> >>>>>
> >>>>>    IGT_TEST_DESCRIPTION("Test SLPC freq API");
> >>>>> @@ -99,6 +102,24 @@ static void test_reset(int i915, int dirfd, int
> >>>>> gt)
> >>>>>     igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpn);
> >>>>>    }
> >>>>>
> >>>>> +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_MAX_FREQ_MHZ) == rpn);
> >>>>> +
> >>>>> +    /* Manually trigger a suspend */
> >>>>> +    igt_system_suspend_autoresume(SUSPEND_STATE_S3,
> >>>>> +                      SUSPEND_TEST_NONE);
> >>>>> +
> >>>>> +    igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
> >>>>> +    igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpn);
> >>>> I am wondering what the purpose/value of this test (and also
> >>>> "freq-reset")
> >>>> is?  How can the "set" min/max set freq (which are just input
> >>>> settings)
> >>>> change whether or not there is a suspend/resume or a reset? Especially
> >>>> when
> >>>> we just return cached min/max values from i915?
> >>> It is mainly checking that we don't smother the softlimit during a
> >>> reset or
> >>> suspend flow.
> >> How can softlimit which is a ordinary variable in memory get clobbered
> >> by
> >> suspend resume?
>
> It shouldn't, but funnier things have happened. Anyways, I can add a check
> for cur_freq and ensure that is at min. That will prove we applied the soft
> limit after suspend.
>
> Thanks,
>
> Vinay.
>
> >>
> >>> In addition, it also tests the read/write interface works as expected
> >>> after those events.
> >> There's no write. Sorry, but I'm not convinced. There should be some
> >> more
> >> meat to the test.
> > There are writes in the IGT fixture after the test completes.

This should also be in an exit handler, not in the fixture? See
gem_ctx_freq e.g.

Thanks.
--
Ashutosh



> >>
> >> Maybe we can write a test which will check /all/ sysfs values are the
> >> same
> >> after a suspend resume cycle? Why do only these specific ones have to be
> >> checked?
> >
> > This test is specific to the freq api, hence just min/max entries.
> >
> > Thanks,
> >
> > Vinay.
> >
> >>
> >> Thanks.
> >> --
> >> Ashutosh
> >>
> >>
> >>> Thanks,
> >>>
> >>> Vinay.
> >>>
> >>>> Thanks.
> >>>> --
> >>>> Ashutosh
> >>>>
> >>>>
> >>>>> +}
> >>>>> +
> >>>>>    igt_main
> >>>>>    {
> >>>>>     int i915 = -1;
> >>>>> @@ -143,6 +164,15 @@ igt_main
> >>>>>                 test_reset(i915, dirfd, gt);
> >>>>>     }
> >>>>>
> >>>>> +    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);
> >>>>> +    }
> >>>>> +
> >>>>>     igt_fixture {
> >>>>>         int dirfd, gt;
> >>>>>         /* Restore frequencies */
> >>>>> --
> >>>>> 2.38.1
> >>>>>