Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Tvrtko Ursulin]

On 28/02/2018 12:27, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-02-28 11:38:01)


On 28/02/2018 11:12, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)

From: Tvrtko Ursulin 

Some tests (the ones which call igt_setup_runtime_pm and
igt_pm_enable_audio_runtime_pm) change default system configuration and
never restore it.

The configured runtime suspend is aggressive and may influence behaviour
of subsequent tests, so it is better to restore to previous values on test
exit.

This way system behaviour, with regards to a random sequence of executed
tests, will be more consistent from one run to another.


Otoh, if behaviour changes in subsequent tests, we likely have a lack of
testing :(


Yeah, and I am not saying it does - haven't spotted anything like that,
just that it leaves the auto-suspend with zero delay afterwards,
compared to otherwise default 10s.

Maybe it is good for coverage, even with the downside of randomness
considering shard runs, or maybe it needs to be more explicit.


I actually thought (or at least picked up the idea) we enabled 0
autosuspend delay throughout igt. And that it's only because most of time
we have a display connected that prevents rpm madness.


Yes fair point, the effect of this patch is only visible on esoteric 
headless setups like mine. :)


Anyway, unless I am missing something, I think it is conceptually 
correct to restore, as long as the zero autosuspend delay is not a 
global IGT setup but a helper called by a few tests.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin 
> 
> Some tests (the ones which call igt_setup_runtime_pm and
> igt_pm_enable_audio_runtime_pm) change default system configuration and
> never restore it.
> 
> The configured runtime suspend is aggressive and may influence behaviour
> of subsequent tests, so it is better to restore to previous values on test
> exit.
> 
> This way system behaviour, with regards to a random sequence of executed
> tests, will be more consistent from one run to another.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Imre Deak 
Reviewed-by: Chris Wilson 
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-28 11:38:01)
> 
> On 28/02/2018 11:12, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> >> From: Tvrtko Ursulin 
> >>
> >> Some tests (the ones which call igt_setup_runtime_pm and
> >> igt_pm_enable_audio_runtime_pm) change default system configuration and
> >> never restore it.
> >>
> >> The configured runtime suspend is aggressive and may influence behaviour
> >> of subsequent tests, so it is better to restore to previous values on test
> >> exit.
> >>
> >> This way system behaviour, with regards to a random sequence of executed
> >> tests, will be more consistent from one run to another.
> > 
> > Otoh, if behaviour changes in subsequent tests, we likely have a lack of
> > testing :(
> 
> Yeah, and I am not saying it does - haven't spotted anything like that, 
> just that it leaves the auto-suspend with zero delay afterwards, 
> compared to otherwise default 10s.
> 
> Maybe it is good for coverage, even with the downside of randomness 
> considering shard runs, or maybe it needs to be more explicit.

I actually thought (or at least picked up the idea) we enabled 0
autosuspend delay throughout igt. And that it's only because most of time
we have a display connected that prevents rpm madness.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Tvrtko Ursulin]

On 28/02/2018 11:12, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)

From: Tvrtko Ursulin 

Some tests (the ones which call igt_setup_runtime_pm and
igt_pm_enable_audio_runtime_pm) change default system configuration and
never restore it.

The configured runtime suspend is aggressive and may influence behaviour
of subsequent tests, so it is better to restore to previous values on test
exit.

This way system behaviour, with regards to a random sequence of executed
tests, will be more consistent from one run to another.


Otoh, if behaviour changes in subsequent tests, we likely have a lack of
testing :(


Yeah, and I am not saying it does - haven't spotted anything like that, 
just that it leaves the auto-suspend with zero delay afterwards, 
compared to otherwise default 10s.


Maybe it is good for coverage, even with the downside of randomness 
considering shard runs, or maybe it needs to be more explicit.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin 
> 
> Some tests (the ones which call igt_setup_runtime_pm and
> igt_pm_enable_audio_runtime_pm) change default system configuration and
> never restore it.
> 
> The configured runtime suspend is aggressive and may influence behaviour
> of subsequent tests, so it is better to restore to previous values on test
> exit.
> 
> This way system behaviour, with regards to a random sequence of executed
> tests, will be more consistent from one run to another.

Otoh, if behaviour changes in subsequent tests, we likely have a lack of
testing :(
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit

[Tvrtko Ursulin]

From: Tvrtko Ursulin 

Some tests (the ones which call igt_setup_runtime_pm and
igt_pm_enable_audio_runtime_pm) change default system configuration and
never restore it.

The configured runtime suspend is aggressive and may influence behaviour
of subsequent tests, so it is better to restore to previous values on test
exit.

This way system behaviour, with regards to a random sequence of executed
tests, will be more consistent from one run to another.

Signed-off-by: Tvrtko Ursulin 
Cc: Imre Deak 
---
 lib/igt_pm.c | 107 +--
 1 file changed, 104 insertions(+), 3 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 5bf5b2e23cdc..0c8b5e8e9257 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -63,6 +63,46 @@ enum {
 /* Remember to fix this if adding longer strings */
 #define MAX_POLICY_STRLEN  strlen(MAX_PERFORMANCE_STR)
 
+static char __igt_pm_audio_runtime_power_save[64];
+static char __igt_pm_audio_runtime_control[64];
+
+static void __igt_pm_audio_runtime_exit_handler(int sig)
+{
+   int fd;
+
+   igt_debug("Restoring audio power management to '%s' and '%s'\n",
+ __igt_pm_audio_runtime_power_save,
+ __igt_pm_audio_runtime_control);
+
+   fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
+   if (fd < 0)
+   return;
+   if (write(fd, __igt_pm_audio_runtime_power_save,
+ strlen(__igt_pm_audio_runtime_power_save)) !=
+   strlen(__igt_pm_audio_runtime_power_save))
+   igt_warn("Failed to restore audio power_save to '%s'\n",
+__igt_pm_audio_runtime_power_save);
+   close(fd);
+
+   fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_WRONLY);
+   if (fd < 0)
+   return;
+   if (write(fd, __igt_pm_audio_runtime_control,
+ strlen(__igt_pm_audio_runtime_control)) !=
+   strlen(__igt_pm_audio_runtime_control))
+   igt_warn("Failed to restore audio control to '%s'\n",
+__igt_pm_audio_runtime_control);
+   close(fd);
+}
+
+static void strchomp(char *str)
+{
+   int len = strlen(str);
+
+   if (str[len - 1] == '\n')
+   str[len - 1] = 0;
+}
+
 /**
  * igt_pm_enable_audio_runtime_pm:
  *
@@ -78,16 +118,32 @@ void igt_pm_enable_audio_runtime_pm(void)
 {
int fd;
 
-   fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
+   /* Check if already enabled. */
+   if (__igt_pm_audio_runtime_power_save[0])
+   return;
+
+   fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_RDWR);
if (fd >= 0) {
+   igt_assert(read(fd, __igt_pm_audio_runtime_power_save,
+   sizeof(__igt_pm_audio_runtime_power_save)) > 0);
+   strchomp(__igt_pm_audio_runtime_power_save);
igt_assert_eq(write(fd, "1\n", 2), 2);
+   igt_install_exit_handler(__igt_pm_audio_runtime_exit_handler);
close(fd);
}
-   fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_WRONLY);
+   fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_RDWR);
if (fd >= 0) {
+   igt_assert(read(fd, __igt_pm_audio_runtime_control,
+   sizeof(__igt_pm_audio_runtime_control)) > 0);
+   strchomp(__igt_pm_audio_runtime_control);
igt_assert_eq(write(fd, "auto\n", 5), 5);
close(fd);
}
+
+   igt_debug("Saved audio power management as '%s' and '%s'\n",
+ __igt_pm_audio_runtime_power_save,
+ __igt_pm_audio_runtime_control);
+
/* Give some time for it to react. */
sleep(1);
 }
@@ -238,6 +294,38 @@ void igt_pm_restore_sata_link_power_management(int8_t 
*pm_data)
 /* We just leak this on exit ... */
 int pm_status_fd = -1;
 
+static char __igt_pm_runtime_autosuspend[64];
+static char __igt_pm_runtime_control[64];
+
+static void __igt_pm_runtime_exit_handler(int sig)
+{
+   int fd;
+
+   igt_debug("Restoring runtime management to '%s' and '%s'\n",
+ __igt_pm_runtime_autosuspend,
+ __igt_pm_runtime_control);
+
+   fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+   if (fd < 0)
+   return;
+   if (write(fd, __igt_pm_runtime_autosuspend,
+ strlen(__igt_pm_runtime_autosuspend)) !=
+   strlen(__igt_pm_runtime_autosuspend))
+   igt_warn("Failed to restore runtime pm autosuspend delay to 
'%s'\n",
+__igt_pm_runtime_autosuspend);
+   close(fd);
+
+   fd = open(POWER_DIR "/control", O_WRONLY);
+   if (fd < 0)
+   return;
+   if (write(fd, __igt_pm_runtime_control,
+