* [Intel-gfx] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest @ 2023-06-06 20:35 Vinay Belgaumkar 2023-06-07 21:12 ` [Intel-gfx] [igt-dev] " Dixit, Ashutosh 0 siblings, 1 reply; 8+ messages in thread From: Vinay Belgaumkar @ 2023-06-06 20:35 UTC (permalink / raw) To: intel-gfx, igt-dev Verify that SLPC API works as expected after a suspend. Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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); +} + 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-06 20:35 [Intel-gfx] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest Vinay Belgaumkar @ 2023-06-07 21:12 ` Dixit, Ashutosh 2023-06-07 22:31 ` Belgaumkar, Vinay 0 siblings, 1 reply; 8+ messages in thread From: Dixit, Ashutosh @ 2023-06-07 21:12 UTC (permalink / raw) To: Vinay Belgaumkar; +Cc: igt-dev, intel-gfx 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@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? 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 21:12 ` [Intel-gfx] [igt-dev] " Dixit, Ashutosh @ 2023-06-07 22:31 ` Belgaumkar, Vinay 2023-06-07 22:56 ` Dixit, Ashutosh 0 siblings, 1 reply; 8+ messages in thread From: Belgaumkar, Vinay @ 2023-06-07 22:31 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: igt-dev, intel-gfx 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@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. In addition, it also tests the read/write interface works as expected after those events. 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 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 22:31 ` Belgaumkar, Vinay @ 2023-06-07 22:56 ` Dixit, Ashutosh 2023-06-07 23:11 ` Belgaumkar, Vinay 0 siblings, 1 reply; 8+ messages in thread From: Dixit, Ashutosh @ 2023-06-07 22:56 UTC (permalink / raw) To: Belgaumkar, Vinay; +Cc: igt-dev, intel-gfx 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@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. 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? 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 > >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 22:56 ` Dixit, Ashutosh @ 2023-06-07 23:11 ` Belgaumkar, Vinay 2023-06-07 23:40 ` Belgaumkar, Vinay 0 siblings, 1 reply; 8+ messages in thread From: Belgaumkar, Vinay @ 2023-06-07 23:11 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: igt-dev, intel-gfx 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@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 >>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 23:11 ` Belgaumkar, Vinay @ 2023-06-07 23:40 ` Belgaumkar, Vinay 2023-06-07 23:49 ` Dixit, Ashutosh 2023-06-09 1:25 ` Dixit, Ashutosh 0 siblings, 2 replies; 8+ messages in thread From: Belgaumkar, Vinay @ 2023-06-07 23:40 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: igt-dev, intel-gfx 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@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. >> >> 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 >>>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 23:40 ` Belgaumkar, Vinay @ 2023-06-07 23:49 ` Dixit, Ashutosh 2023-06-09 1:25 ` Dixit, Ashutosh 1 sibling, 0 replies; 8+ messages in thread From: Dixit, Ashutosh @ 2023-06-07 23:49 UTC (permalink / raw) To: Belgaumkar, Vinay; +Cc: igt-dev, intel-gfx 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@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 > >>>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest 2023-06-07 23:40 ` Belgaumkar, Vinay 2023-06-07 23:49 ` Dixit, Ashutosh @ 2023-06-09 1:25 ` Dixit, Ashutosh 1 sibling, 0 replies; 8+ messages in thread From: Dixit, Ashutosh @ 2023-06-09 1:25 UTC (permalink / raw) To: Belgaumkar, Vinay; +Cc: igt-dev, intel-gfx 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@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. Yes, let's do this. With this at least we will see the min freq being applied after a resume and should be sufficient reason to merge this. This test is present for xe in it's present form, but for xe it is reading shared data (which can be in vram). So it is harder to criticize that it is just returning a cpu memory variable. Thanks. -- Ashutosh > >>> 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 > >>>>> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-09 1:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-06 20:35 [Intel-gfx] [PATCH i-g-t] tests/i915_pm_freq_api: Add a suspend subtest Vinay Belgaumkar 2023-06-07 21:12 ` [Intel-gfx] [igt-dev] " Dixit, Ashutosh 2023-06-07 22:31 ` Belgaumkar, Vinay 2023-06-07 22:56 ` Dixit, Ashutosh 2023-06-07 23:11 ` Belgaumkar, Vinay 2023-06-07 23:40 ` Belgaumkar, Vinay 2023-06-07 23:49 ` Dixit, Ashutosh 2023-06-09 1:25 ` Dixit, Ashutosh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox