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

[Belgaumkar, Vinay]

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?
>
>> 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.
>
> 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
>>>>