All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 11:08 ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 11:08 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 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/0000: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/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,10 +349,16 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
+	/* Save previous values and install exit handler to restore them. */
+	igt_assert(read(fd, __igt_pm_runtime_autosuspend,
+			sizeof(__igt_pm_runtime_autosuspend)) > 0);
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	/* If we fail to write to the file, it means this system doesn't support
 	 * runtime PM. */
 	size = write(fd, "0\n", 2);
@@ -278,6 +372,13 @@ bool igt_setup_runtime_pm(void)
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [igt-dev] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 11:08 ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 11:08 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx, Tvrtko Ursulin

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 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/0000: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/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,10 +349,16 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
+	/* Save previous values and install exit handler to restore them. */
+	igt_assert(read(fd, __igt_pm_runtime_autosuspend,
+			sizeof(__igt_pm_runtime_autosuspend)) > 0);
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	/* If we fail to write to the file, it means this system doesn't support
 	 * runtime PM. */
 	size = write(fd, "0\n", 2);
@@ -278,6 +372,13 @@ bool igt_setup_runtime_pm(void)
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
@ 2018-02-28 11:12   ` Chris Wilson
  -1 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 11:12 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 11:12   ` Chris Wilson
  0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 11:12 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:12   ` [igt-dev] [Intel-gfx] " Chris Wilson
@ 2018-02-28 11:38     ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 11:38 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx


On 28/02/2018 11:12, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 11:38     ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 11:38 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx


On 28/02/2018 11:12, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:38     ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-02-28 12:27       ` Chris Wilson
  -1 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 12:27 UTC (permalink / raw)
  To: Tvrtko Ursulin, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

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 <tvrtko.ursulin@intel.com>
> >>
> >> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 12:27       ` Chris Wilson
  0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 12:27 UTC (permalink / raw)
  To: Tvrtko Ursulin, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

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 <tvrtko.ursulin@intel.com>
> >>
> >> 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
  (?)
  (?)
@ 2018-02-28 12:50 ` Patchwork
  2018-02-28 15:17   ` Tvrtko Ursulin
  -1 siblings, 1 reply; 31+ messages in thread
From: Patchwork @ 2018-02-28 12:50 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit
URL   : https://patchwork.freedesktop.org/series/39106/
State : failure

== Summary ==

IGT patchset tested on top of latest successful build
d3dc9c619791c8cb88d176fc7b3d8aa5e802055d lib/igt_draw: Fix bo leak in gpu draw routines

with latest DRM-Tip kernel build CI_DRM_3846
9a02ae14ae02 drm-tip: 2018y-02m-28d-10h-13m-18s UTC integration manifest

No testlist changes.

---- Possible new issues:

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                skip       -> FAIL       (fi-bdw-gvtdvm)
                skip       -> FAIL       (fi-blb-e6850)
                skip       -> FAIL       (fi-bwr-2160)
                skip       -> FAIL       (fi-elk-e7500)
                skip       -> FAIL       (fi-gdg-551)
                skip       -> FAIL       (fi-ilk-650)
                skip       -> FAIL       (fi-ivb-3520m)
                skip       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-pnv-d510)
                skip       -> FAIL       (fi-skl-gvtdvm)
                skip       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-snb-2600)
        Subgroup basic-rte:
                skip       -> FAIL       (fi-bdw-gvtdvm)
                skip       -> FAIL       (fi-blb-e6850)
                skip       -> FAIL       (fi-bwr-2160)
                skip       -> FAIL       (fi-elk-e7500)
                skip       -> FAIL       (fi-gdg-551)
                skip       -> FAIL       (fi-ilk-650)
                skip       -> FAIL       (fi-ivb-3520m)
                skip       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-pnv-d510)
                skip       -> FAIL       (fi-skl-gvtdvm)
                skip       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-snb-2600)

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:2   skip:22  time:428s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:2   skip:62  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:489s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:2   skip:103 time:285s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:471s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:457s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:394s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:574s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:2   skip:57  time:415s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:3   skip:106 time:285s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:507s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:2   skip:58  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:2   skip:27  time:453s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:2   skip:31  time:410s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:484s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:455s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:496s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:2   skip:63  time:587s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:427s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:518s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:2   skip:21  time:429s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:518s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:393s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1021/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
@ 2018-02-28 14:23   ` Chris Wilson
  -1 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 14:23 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 14:23   ` Chris Wilson
  0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-02-28 14:23 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 11:08:29)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 12:27       ` [igt-dev] [Intel-gfx] " Chris Wilson
@ 2018-02-28 14:33         ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 14:33 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx


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 <tvrtko.ursulin@intel.com>
>>>>
>>>> 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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 14:33         ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 14:33 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx


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 <tvrtko.ursulin@intel.com>
>>>>
>>>> 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 12:50 ` [igt-dev] ✗ Fi.CI.BAT: failure for " Patchwork
@ 2018-02-28 15:17   ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 15:17 UTC (permalink / raw)
  To: igt-dev, Patchwork, Tvrtko Ursulin


On 28/02/2018 12:50, Patchwork wrote:
> == Series Details ==
> 
> Series: lib/igt_pm: Restore runtime pm state on test exit
> URL   : https://patchwork.freedesktop.org/series/39106/
> State : failure
> 
> == Summary ==
> 
> IGT patchset tested on top of latest successful build
> d3dc9c619791c8cb88d176fc7b3d8aa5e802055d lib/igt_draw: Fix bo leak in gpu draw routines
> 
> with latest DRM-Tip kernel build CI_DRM_3846
> 9a02ae14ae02 drm-tip: 2018y-02m-28d-10h-13m-18s UTC integration manifest
> 
> No testlist changes.
> 
> ---- Possible new issues:
> 
> Test pm_rpm:
>          Subgroup basic-pci-d3-state:
>                  skip       -> FAIL       (fi-bdw-gvtdvm)
>                  skip       -> FAIL       (fi-blb-e6850)
>                  skip       -> FAIL       (fi-bwr-2160)
>                  skip       -> FAIL       (fi-elk-e7500)
>                  skip       -> FAIL       (fi-gdg-551)
>                  skip       -> FAIL       (fi-ilk-650)
>                  skip       -> FAIL       (fi-ivb-3520m)
>                  skip       -> FAIL       (fi-ivb-3770)
>                  skip       -> FAIL       (fi-pnv-d510)
>                  skip       -> FAIL       (fi-skl-gvtdvm)
>                  skip       -> FAIL       (fi-snb-2520m)
>                  skip       -> FAIL       (fi-snb-2600)
>          Subgroup basic-rte:
>                  skip       -> FAIL       (fi-bdw-gvtdvm)
>                  skip       -> FAIL       (fi-blb-e6850)
>                  skip       -> FAIL       (fi-bwr-2160)
>                  skip       -> FAIL       (fi-elk-e7500)
>                  skip       -> FAIL       (fi-gdg-551)
>                  skip       -> FAIL       (fi-ilk-650)
>                  skip       -> FAIL       (fi-ivb-3520m)
>                  skip       -> FAIL       (fi-ivb-3770)
>                  skip       -> FAIL       (fi-pnv-d510)
>                  skip       -> FAIL       (fi-skl-gvtdvm)
>                  skip       -> FAIL       (fi-snb-2520m)
>                  skip       -> FAIL       (fi-snb-2600)

Must not assert on reads if runtime pm is not supported...

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
@ 2018-02-28 15:35   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 15:35 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v2: Read failure means no runtime pm support so don't assert on it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
---
 lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 5 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
-	/* If we fail to write to the file, it means this system doesn't support
-	 * runtime PM. */
+	/*
+	 * Save previous values to be able to  install exit handler to restore
+	 * them on test exit.
+	 */
+	size = read(fd, __igt_pm_runtime_autosuspend,
+		    sizeof(__igt_pm_runtime_autosuspend));
+
+	/*
+	 * If we fail to read from the file, it means this system doesn't
+	 * support runtime PM.
+	 */
+	if (size <= 0) {
+		close(fd);
+		return false;
+	}
+
 	size = write(fd, "0\n", 2);
 
 	close(fd);
@@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
 	if (size != 2)
 		return false;
 
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	/* We know we support runtime PM, let's try to enable it now. */
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Intel-gfx] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-02-28 15:35   ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-02-28 15:35 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v2: Read failure means no runtime pm support so don't assert on it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
---
 lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 5 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
-	/* If we fail to write to the file, it means this system doesn't support
-	 * runtime PM. */
+	/*
+	 * Save previous values to be able to  install exit handler to restore
+	 * them on test exit.
+	 */
+	size = read(fd, __igt_pm_runtime_autosuspend,
+		    sizeof(__igt_pm_runtime_autosuspend));
+
+	/*
+	 * If we fail to read from the file, it means this system doesn't
+	 * support runtime PM.
+	 */
+	if (size <= 0) {
+		close(fd);
+		return false;
+	}
+
 	size = write(fd, "0\n", 2);
 
 	close(fd);
@@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
 	if (size != 2)
 		return false;
 
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	/* We know we support runtime PM, let's try to enable it now. */
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_pm: Restore runtime pm state on test exit (rev2)
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  (?)
@ 2018-02-28 16:33 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2018-02-28 16:33 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit (rev2)
URL   : https://patchwork.freedesktop.org/series/39106/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
0d47ec161b4eca1b41c5348604aa05b105e5d1cf tests/perf: Fix build warning

with latest DRM-Tip kernel build CI_DRM_3849
4d8ce7ac845a drm-tip: 2018y-02m-28d-14h-17m-44s UTC integration manifest

No testlist changes.

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi) fdo#103927
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:483s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:285s
fi-bxt-dsi       total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:461s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:394s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:557s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:590s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:283s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:407s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:456s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:420s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:489s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:491s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:586s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:434s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:522s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:409s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:435s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:528s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:395s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1025/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [igt-dev] ✗ Fi.CI.IGT: failure for lib/igt_pm: Restore runtime pm state on test exit (rev2)
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
                   ` (5 preceding siblings ...)
  (?)
@ 2018-02-28 20:22 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2018-02-28 20:22 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit (rev2)
URL   : https://patchwork.freedesktop.org/series/39106/
State : failure

== Summary ==

---- Possible new issues:

Test kms_color:
        Subgroup pipe-a-ctm-0-25:
                pass       -> FAIL       (shard-apl)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-contexts:
                pass       -> INCOMPLETE (shard-apl) fdo#104945
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-left-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test kms_flip:
        Subgroup 2x-flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887
        Subgroup 2x-plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368 +1
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-pri-indfb-multidraw:
                fail       -> PASS       (shard-snb) fdo#103167
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                pass       -> FAIL       (shard-snb) fdo#103925
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test kms_vblank:
        Subgroup pipe-a-accuracy-idle:
                pass       -> FAIL       (shard-hsw) fdo#102583

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583

shard-apl        total:3425 pass:1801 dwarn:1   dfail:0   fail:8   skip:1613 time:11927s
shard-hsw        total:3460 pass:1763 dwarn:1   dfail:0   fail:5   skip:1690 time:11556s
shard-snb        total:3460 pass:1358 dwarn:1   dfail:0   fail:2   skip:2099 time:6698s
Blacklisted hosts:
shard-kbl        total:3425 pass:1920 dwarn:1   dfail:0   fail:9   skip:1494 time:9160s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1025/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 15:35   ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-03-01  8:12     ` Chris Wilson
  -1 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-03-01  8:12 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 15:35:06)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> v2: Read failure means no runtime pm support so don't assert on it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-03-01  8:12     ` Chris Wilson
  0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2018-03-01  8:12 UTC (permalink / raw)
  To: Tvrtko Ursulin, igt-dev; +Cc: Intel-gfx

Quoting Tvrtko Ursulin (2018-02-28 15:35:06)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> v2: Read failure means no runtime pm support so don't assert on it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
  2018-02-28 15:35   ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-03-02  9:29     ` Imre Deak
  -1 siblings, 0 replies; 31+ messages in thread
From: Imre Deak @ 2018-03-02  9:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> v2: Read failure means no runtime pm support so don't assert on it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1

Agreed about having a consistent expected state for each test, not sure
why we didn't restore these settings :/ Btw, I feel somewhat the same
about test results being affected by previous tests, but not sure if
anything should/can be done about that.

Acked-by: Imre Deak <imre.deak@intel.com>

Some nits below.

> ---
>  lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 117 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);

Read/install_handler/write would avoid a potential race with ^C. There's also
link_power_management_policy which is only restored during normal exit.

>  		close(fd);
>  	}
> -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> +	fd = open("/sys/bus/pci/devices/0000: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,
> +		  strlen(__igt_pm_runtime_control)) !=
> +	    strlen(__igt_pm_runtime_control))
> +		igt_warn("Failed to restore runtime pm control to '%s'\n",
> +			 __igt_pm_runtime_control);
> +	close(fd);
> +}
> +
>  /**
>   * igt_setup_runtime_pm:
>   *
> @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
>  	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
>  	 * suite goes faster and we have a higher probability of triggering race
>  	 * conditions. */
> -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
> +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
>  	igt_assert_f(fd >= 0,
>  		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
>  
> -	/* If we fail to write to the file, it means this system doesn't support
> -	 * runtime PM. */
> +	/*
> +	 * Save previous values to be able to  install exit handler to restore
> +	 * them on test exit.
> +	 */
> +	size = read(fd, __igt_pm_runtime_autosuspend,
> +		    sizeof(__igt_pm_runtime_autosuspend));
> +
> +	/*
> +	 * If we fail to read from the file, it means this system doesn't
> +	 * support runtime PM.
> +	 */
> +	if (size <= 0) {
> +		close(fd);
> +		return false;
> +	}
> +
>  	size = write(fd, "0\n", 2);
>  
>  	close(fd);
> @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
>  	if (size != 2)
>  		return false;
>  
> +	strchomp(__igt_pm_runtime_autosuspend);
> +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> +
>  	/* We know we support runtime PM, let's try to enable it now. */
>  	fd = open(POWER_DIR "/control", O_RDWR);
>  	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
>  
> +	igt_assert(read(fd, __igt_pm_runtime_control,
> +			sizeof(__igt_pm_runtime_control)) > 0);
> +	strchomp(__igt_pm_runtime_control);
> +
> +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
> +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
> +
>  	size = write(fd, "auto\n", 5);
>  	igt_assert(size == 5);
>  
> -- 
> 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-03-02  9:29     ` Imre Deak
  0 siblings, 0 replies; 31+ messages in thread
From: Imre Deak @ 2018-03-02  9:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx, Tvrtko Ursulin

On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> v2: Read failure means no runtime pm support so don't assert on it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1

Agreed about having a consistent expected state for each test, not sure
why we didn't restore these settings :/ Btw, I feel somewhat the same
about test results being affected by previous tests, but not sure if
anything should/can be done about that.

Acked-by: Imre Deak <imre.deak@intel.com>

Some nits below.

> ---
>  lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 117 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);

Read/install_handler/write would avoid a potential race with ^C. There's also
link_power_management_policy which is only restored during normal exit.

>  		close(fd);
>  	}
> -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> +	fd = open("/sys/bus/pci/devices/0000: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,
> +		  strlen(__igt_pm_runtime_control)) !=
> +	    strlen(__igt_pm_runtime_control))
> +		igt_warn("Failed to restore runtime pm control to '%s'\n",
> +			 __igt_pm_runtime_control);
> +	close(fd);
> +}
> +
>  /**
>   * igt_setup_runtime_pm:
>   *
> @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
>  	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
>  	 * suite goes faster and we have a higher probability of triggering race
>  	 * conditions. */
> -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
> +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
>  	igt_assert_f(fd >= 0,
>  		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
>  
> -	/* If we fail to write to the file, it means this system doesn't support
> -	 * runtime PM. */
> +	/*
> +	 * Save previous values to be able to  install exit handler to restore
> +	 * them on test exit.
> +	 */
> +	size = read(fd, __igt_pm_runtime_autosuspend,
> +		    sizeof(__igt_pm_runtime_autosuspend));
> +
> +	/*
> +	 * If we fail to read from the file, it means this system doesn't
> +	 * support runtime PM.
> +	 */
> +	if (size <= 0) {
> +		close(fd);
> +		return false;
> +	}
> +
>  	size = write(fd, "0\n", 2);
>  
>  	close(fd);
> @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
>  	if (size != 2)
>  		return false;
>  
> +	strchomp(__igt_pm_runtime_autosuspend);
> +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> +
>  	/* We know we support runtime PM, let's try to enable it now. */
>  	fd = open(POWER_DIR "/control", O_RDWR);
>  	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
>  
> +	igt_assert(read(fd, __igt_pm_runtime_control,
> +			sizeof(__igt_pm_runtime_control)) > 0);
> +	strchomp(__igt_pm_runtime_control);
> +
> +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
> +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
> +
>  	size = write(fd, "auto\n", 5);
>  	igt_assert(size == 5);
>  
> -- 
> 2.14.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH i-g-t v3] lib/igt_pm: Restore runtime pm state on test exit
  2018-03-02  9:29     ` [igt-dev] " Imre Deak
@ 2018-03-02  9:51       ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-03-02  9:51 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v2: Read failure means no runtime pm support so don't assert on it.
v3: Install exit handler before the write to close the Ctrl^C race. (Imre)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v2
Acked-by: Imre Deak <imre.deak@intel.com>
---
 lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 5 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 5bf5b2e23cdc..8ac132269d79 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/0000: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 (len && 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_install_exit_handler(__igt_pm_audio_runtime_exit_handler);
 		igt_assert_eq(write(fd, "1\n", 2), 2);
 		close(fd);
 	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,12 +349,29 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
-	/* If we fail to write to the file, it means this system doesn't support
-	 * runtime PM. */
+	/*
+	 * Save previous values to be able to  install exit handler to restore
+	 * them on test exit.
+	 */
+	size = read(fd, __igt_pm_runtime_autosuspend,
+		    sizeof(__igt_pm_runtime_autosuspend));
+
+	/*
+	 * If we fail to read from the file, it means this system doesn't
+	 * support runtime PM.
+	 */
+	if (size <= 0) {
+		close(fd);
+		return false;
+	}
+
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	size = write(fd, "0\n", 2);
 
 	close(fd);
@@ -278,6 +383,13 @@ bool igt_setup_runtime_pm(void)
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Intel-gfx] [PATCH i-g-t v3] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-03-02  9:51       ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-03-02  9:51 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v2: Read failure means no runtime pm support so don't assert on it.
v3: Install exit handler before the write to close the Ctrl^C race. (Imre)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v2
Acked-by: Imre Deak <imre.deak@intel.com>
---
 lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 5 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 5bf5b2e23cdc..8ac132269d79 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/0000: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 (len && 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_install_exit_handler(__igt_pm_audio_runtime_exit_handler);
 		igt_assert_eq(write(fd, "1\n", 2), 2);
 		close(fd);
 	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
+	fd = open("/sys/bus/pci/devices/0000: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,
+		  strlen(__igt_pm_runtime_control)) !=
+	    strlen(__igt_pm_runtime_control))
+		igt_warn("Failed to restore runtime pm control to '%s'\n",
+			 __igt_pm_runtime_control);
+	close(fd);
+}
+
 /**
  * igt_setup_runtime_pm:
  *
@@ -261,12 +349,29 @@ bool igt_setup_runtime_pm(void)
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
 	 * conditions. */
-	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
 	igt_assert_f(fd >= 0,
 		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
 
-	/* If we fail to write to the file, it means this system doesn't support
-	 * runtime PM. */
+	/*
+	 * Save previous values to be able to  install exit handler to restore
+	 * them on test exit.
+	 */
+	size = read(fd, __igt_pm_runtime_autosuspend,
+		    sizeof(__igt_pm_runtime_autosuspend));
+
+	/*
+	 * If we fail to read from the file, it means this system doesn't
+	 * support runtime PM.
+	 */
+	if (size <= 0) {
+		close(fd);
+		return false;
+	}
+
+	strchomp(__igt_pm_runtime_autosuspend);
+	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+
 	size = write(fd, "0\n", 2);
 
 	close(fd);
@@ -278,6 +383,13 @@ bool igt_setup_runtime_pm(void)
 	fd = open(POWER_DIR "/control", O_RDWR);
 	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
 
+	igt_assert(read(fd, __igt_pm_runtime_control,
+			sizeof(__igt_pm_runtime_control)) > 0);
+	strchomp(__igt_pm_runtime_control);
+
+	igt_debug("Saved runtime power management as '%s' and '%s'\n",
+		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
+
 	size = write(fd, "auto\n", 5);
 	igt_assert(size == 5);
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
  2018-03-02  9:29     ` [igt-dev] " Imre Deak
@ 2018-03-02  9:56       ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-03-02  9:56 UTC (permalink / raw)
  To: imre.deak, Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx


On 02/03/2018 09:29, Imre Deak wrote:
> On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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.
>>
>> v2: Read failure means no runtime pm support so don't assert on it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> 
> Agreed about having a consistent expected state for each test, not sure
> why we didn't restore these settings :/ Btw, I feel somewhat the same
> about test results being affected by previous tests, but not sure if
> anything should/can be done about that.
> 
> Acked-by: Imre Deak <imre.deak@intel.com>
> 
> Some nits below.
> 
>> ---
>>   lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 117 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>> index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);
> 
> Read/install_handler/write would avoid a potential race with ^C. There's also

Well spotted, done in v3.

> link_power_management_policy which is only restored during normal exit.

This one already has code to restore 
(igt_pm_restore_sata_link_power_management) so maybe best to decide 
where to put the responsibility of installing the exit handler in a 
follow up patch? Make igt_pm_enable_sata_link_power_management do it, or 
have the caller do it? Former would be inline with this patch, and then 
probably we can unexport igt_pm_restore_sata_link_power_management. Or 
does it already handle this for normal exit since it is calling it from 
igt_fixture?

Regards,

Tvrtko

> 
>>   		close(fd);
>>   	}
>> -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
>> +	fd = open("/sys/bus/pci/devices/0000: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,
>> +		  strlen(__igt_pm_runtime_control)) !=
>> +	    strlen(__igt_pm_runtime_control))
>> +		igt_warn("Failed to restore runtime pm control to '%s'\n",
>> +			 __igt_pm_runtime_control);
>> +	close(fd);
>> +}
>> +
>>   /**
>>    * igt_setup_runtime_pm:
>>    *
>> @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
>>   	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
>>   	 * suite goes faster and we have a higher probability of triggering race
>>   	 * conditions. */
>> -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
>> +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
>>   	igt_assert_f(fd >= 0,
>>   		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
>>   
>> -	/* If we fail to write to the file, it means this system doesn't support
>> -	 * runtime PM. */
>> +	/*
>> +	 * Save previous values to be able to  install exit handler to restore
>> +	 * them on test exit.
>> +	 */
>> +	size = read(fd, __igt_pm_runtime_autosuspend,
>> +		    sizeof(__igt_pm_runtime_autosuspend));
>> +
>> +	/*
>> +	 * If we fail to read from the file, it means this system doesn't
>> +	 * support runtime PM.
>> +	 */
>> +	if (size <= 0) {
>> +		close(fd);
>> +		return false;
>> +	}
>> +
>>   	size = write(fd, "0\n", 2);
>>   
>>   	close(fd);
>> @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
>>   	if (size != 2)
>>   		return false;
>>   
>> +	strchomp(__igt_pm_runtime_autosuspend);
>> +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
>> +
>>   	/* We know we support runtime PM, let's try to enable it now. */
>>   	fd = open(POWER_DIR "/control", O_RDWR);
>>   	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
>>   
>> +	igt_assert(read(fd, __igt_pm_runtime_control,
>> +			sizeof(__igt_pm_runtime_control)) > 0);
>> +	strchomp(__igt_pm_runtime_control);
>> +
>> +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
>> +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
>> +
>>   	size = write(fd, "auto\n", 5);
>>   	igt_assert(size == 5);
>>   
>> -- 
>> 2.14.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-03-02  9:56       ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2018-03-02  9:56 UTC (permalink / raw)
  To: imre.deak, Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx


On 02/03/2018 09:29, Imre Deak wrote:
> On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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.
>>
>> v2: Read failure means no runtime pm support so don't assert on it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> 
> Agreed about having a consistent expected state for each test, not sure
> why we didn't restore these settings :/ Btw, I feel somewhat the same
> about test results being affected by previous tests, but not sure if
> anything should/can be done about that.
> 
> Acked-by: Imre Deak <imre.deak@intel.com>
> 
> Some nits below.
> 
>> ---
>>   lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 117 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>> index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);
> 
> Read/install_handler/write would avoid a potential race with ^C. There's also

Well spotted, done in v3.

> link_power_management_policy which is only restored during normal exit.

This one already has code to restore 
(igt_pm_restore_sata_link_power_management) so maybe best to decide 
where to put the responsibility of installing the exit handler in a 
follow up patch? Make igt_pm_enable_sata_link_power_management do it, or 
have the caller do it? Former would be inline with this patch, and then 
probably we can unexport igt_pm_restore_sata_link_power_management. Or 
does it already handle this for normal exit since it is calling it from 
igt_fixture?

Regards,

Tvrtko

> 
>>   		close(fd);
>>   	}
>> -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
>> +	fd = open("/sys/bus/pci/devices/0000: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,
>> +		  strlen(__igt_pm_runtime_control)) !=
>> +	    strlen(__igt_pm_runtime_control))
>> +		igt_warn("Failed to restore runtime pm control to '%s'\n",
>> +			 __igt_pm_runtime_control);
>> +	close(fd);
>> +}
>> +
>>   /**
>>    * igt_setup_runtime_pm:
>>    *
>> @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
>>   	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
>>   	 * suite goes faster and we have a higher probability of triggering race
>>   	 * conditions. */
>> -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
>> +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
>>   	igt_assert_f(fd >= 0,
>>   		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
>>   
>> -	/* If we fail to write to the file, it means this system doesn't support
>> -	 * runtime PM. */
>> +	/*
>> +	 * Save previous values to be able to  install exit handler to restore
>> +	 * them on test exit.
>> +	 */
>> +	size = read(fd, __igt_pm_runtime_autosuspend,
>> +		    sizeof(__igt_pm_runtime_autosuspend));
>> +
>> +	/*
>> +	 * If we fail to read from the file, it means this system doesn't
>> +	 * support runtime PM.
>> +	 */
>> +	if (size <= 0) {
>> +		close(fd);
>> +		return false;
>> +	}
>> +
>>   	size = write(fd, "0\n", 2);
>>   
>>   	close(fd);
>> @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
>>   	if (size != 2)
>>   		return false;
>>   
>> +	strchomp(__igt_pm_runtime_autosuspend);
>> +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
>> +
>>   	/* We know we support runtime PM, let's try to enable it now. */
>>   	fd = open(POWER_DIR "/control", O_RDWR);
>>   	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
>>   
>> +	igt_assert(read(fd, __igt_pm_runtime_control,
>> +			sizeof(__igt_pm_runtime_control)) > 0);
>> +	strchomp(__igt_pm_runtime_control);
>> +
>> +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
>> +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
>> +
>>   	size = write(fd, "auto\n", 5);
>>   	igt_assert(size == 5);
>>   
>> -- 
>> 2.14.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
  2018-03-02  9:56       ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
@ 2018-03-02 12:29         ` Imre Deak
  -1 siblings, 0 replies; 31+ messages in thread
From: Imre Deak @ 2018-03-02 12:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Fri, Mar 02, 2018 at 09:56:26AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/03/2018 09:29, Imre Deak wrote:
> > On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > 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.
> > > 
> > > v2: Read failure means no runtime pm support so don't assert on it.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> > 
> > Agreed about having a consistent expected state for each test, not sure
> > why we didn't restore these settings :/ Btw, I feel somewhat the same
> > about test results being affected by previous tests, but not sure if
> > anything should/can be done about that.
> > 
> > Acked-by: Imre Deak <imre.deak@intel.com>
> > 
> > Some nits below.
> > 
> > > ---
> > >   lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 117 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > > index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);
> > 
> > Read/install_handler/write would avoid a potential race with ^C. There's also
> 
> Well spotted, done in v3.

Ok, for that one:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> > link_power_management_policy which is only restored during normal exit.
> 
> This one already has code to restore
> (igt_pm_restore_sata_link_power_management) so maybe best to decide where to
> put the responsibility of installing the exit handler in a follow up patch?
> Make igt_pm_enable_sata_link_power_management do it, or have the caller do
> it? Former would be inline with this patch, and then probably we can
> unexport igt_pm_restore_sata_link_power_management. Or does it already
> handle this for normal exit since it is calling it from igt_fixture?

Yes, it's handled for normal exit via igt_fixture, but I think it should
be restored the same way as you did here. But yea, it's fine to do that
as a follow-up.

> 
> Regards,
> 
> Tvrtko
> 
> > 
> > >   		close(fd);
> > >   	}
> > > -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> > > +	fd = open("/sys/bus/pci/devices/0000: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,
> > > +		  strlen(__igt_pm_runtime_control)) !=
> > > +	    strlen(__igt_pm_runtime_control))
> > > +		igt_warn("Failed to restore runtime pm control to '%s'\n",
> > > +			 __igt_pm_runtime_control);
> > > +	close(fd);
> > > +}
> > > +
> > >   /**
> > >    * igt_setup_runtime_pm:
> > >    *
> > > @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
> > >   	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
> > >   	 * suite goes faster and we have a higher probability of triggering race
> > >   	 * conditions. */
> > > -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
> > > +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
> > >   	igt_assert_f(fd >= 0,
> > >   		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
> > > -	/* If we fail to write to the file, it means this system doesn't support
> > > -	 * runtime PM. */
> > > +	/*
> > > +	 * Save previous values to be able to  install exit handler to restore
> > > +	 * them on test exit.
> > > +	 */
> > > +	size = read(fd, __igt_pm_runtime_autosuspend,
> > > +		    sizeof(__igt_pm_runtime_autosuspend));
> > > +
> > > +	/*
> > > +	 * If we fail to read from the file, it means this system doesn't
> > > +	 * support runtime PM.
> > > +	 */
> > > +	if (size <= 0) {
> > > +		close(fd);
> > > +		return false;
> > > +	}
> > > +
> > >   	size = write(fd, "0\n", 2);
> > >   	close(fd);
> > > @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
> > >   	if (size != 2)
> > >   		return false;
> > > +	strchomp(__igt_pm_runtime_autosuspend);
> > > +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> > > +
> > >   	/* We know we support runtime PM, let's try to enable it now. */
> > >   	fd = open(POWER_DIR "/control", O_RDWR);
> > >   	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
> > > +	igt_assert(read(fd, __igt_pm_runtime_control,
> > > +			sizeof(__igt_pm_runtime_control)) > 0);
> > > +	strchomp(__igt_pm_runtime_control);
> > > +
> > > +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
> > > +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
> > > +
> > >   	size = write(fd, "auto\n", 5);
> > >   	igt_assert(size == 5);
> > > -- 
> > > 2.14.1
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
@ 2018-03-02 12:29         ` Imre Deak
  0 siblings, 0 replies; 31+ messages in thread
From: Imre Deak @ 2018-03-02 12:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Fri, Mar 02, 2018 at 09:56:26AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/03/2018 09:29, Imre Deak wrote:
> > On Wed, Feb 28, 2018 at 03:35:06PM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > 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.
> > > 
> > > v2: Read failure means no runtime pm support so don't assert on it.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> > 
> > Agreed about having a consistent expected state for each test, not sure
> > why we didn't restore these settings :/ Btw, I feel somewhat the same
> > about test results being affected by previous tests, but not sure if
> > anything should/can be done about that.
> > 
> > Acked-by: Imre Deak <imre.deak@intel.com>
> > 
> > Some nits below.
> > 
> > > ---
> > >   lib/igt_pm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 117 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > > index 5bf5b2e23cdc..04e2b89cca95 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/0000: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 (len && 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);
> > 
> > Read/install_handler/write would avoid a potential race with ^C. There's also
> 
> Well spotted, done in v3.

Ok, for that one:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> > link_power_management_policy which is only restored during normal exit.
> 
> This one already has code to restore
> (igt_pm_restore_sata_link_power_management) so maybe best to decide where to
> put the responsibility of installing the exit handler in a follow up patch?
> Make igt_pm_enable_sata_link_power_management do it, or have the caller do
> it? Former would be inline with this patch, and then probably we can
> unexport igt_pm_restore_sata_link_power_management. Or does it already
> handle this for normal exit since it is calling it from igt_fixture?

Yes, it's handled for normal exit via igt_fixture, but I think it should
be restored the same way as you did here. But yea, it's fine to do that
as a follow-up.

> 
> Regards,
> 
> Tvrtko
> 
> > 
> > >   		close(fd);
> > >   	}
> > > -	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> > > +	fd = open("/sys/bus/pci/devices/0000: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,
> > > +		  strlen(__igt_pm_runtime_control)) !=
> > > +	    strlen(__igt_pm_runtime_control))
> > > +		igt_warn("Failed to restore runtime pm control to '%s'\n",
> > > +			 __igt_pm_runtime_control);
> > > +	close(fd);
> > > +}
> > > +
> > >   /**
> > >    * igt_setup_runtime_pm:
> > >    *
> > > @@ -261,12 +349,26 @@ bool igt_setup_runtime_pm(void)
> > >   	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
> > >   	 * suite goes faster and we have a higher probability of triggering race
> > >   	 * conditions. */
> > > -	fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
> > > +	fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
> > >   	igt_assert_f(fd >= 0,
> > >   		     "Can't open " POWER_DIR "/autosuspend_delay_ms\n");
> > > -	/* If we fail to write to the file, it means this system doesn't support
> > > -	 * runtime PM. */
> > > +	/*
> > > +	 * Save previous values to be able to  install exit handler to restore
> > > +	 * them on test exit.
> > > +	 */
> > > +	size = read(fd, __igt_pm_runtime_autosuspend,
> > > +		    sizeof(__igt_pm_runtime_autosuspend));
> > > +
> > > +	/*
> > > +	 * If we fail to read from the file, it means this system doesn't
> > > +	 * support runtime PM.
> > > +	 */
> > > +	if (size <= 0) {
> > > +		close(fd);
> > > +		return false;
> > > +	}
> > > +
> > >   	size = write(fd, "0\n", 2);
> > >   	close(fd);
> > > @@ -274,10 +376,20 @@ bool igt_setup_runtime_pm(void)
> > >   	if (size != 2)
> > >   		return false;
> > > +	strchomp(__igt_pm_runtime_autosuspend);
> > > +	igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> > > +
> > >   	/* We know we support runtime PM, let's try to enable it now. */
> > >   	fd = open(POWER_DIR "/control", O_RDWR);
> > >   	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
> > > +	igt_assert(read(fd, __igt_pm_runtime_control,
> > > +			sizeof(__igt_pm_runtime_control)) > 0);
> > > +	strchomp(__igt_pm_runtime_control);
> > > +
> > > +	igt_debug("Saved runtime power management as '%s' and '%s'\n",
> > > +		  __igt_pm_runtime_autosuspend, __igt_pm_runtime_control);
> > > +
> > >   	size = write(fd, "auto\n", 5);
> > >   	igt_assert(size == 5);
> > > -- 
> > > 2.14.1
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_pm: Restore runtime pm state on test exit (rev3)
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
                   ` (6 preceding siblings ...)
  (?)
@ 2018-03-02 18:21 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2018-03-02 18:21 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit (rev3)
URL   : https://patchwork.freedesktop.org/series/39106/
State : failure

== Summary ==

IGT patchset tested on top of latest successful build
bddfb8dd3c1767f13d2af578d5c3d897fddf0dcd igt/gem_ctx_switch: Exercise all engines at once

with latest DRM-Tip kernel build CI_DRM_3867
4f4e4dd52a30 drm-tip: 2018y-03m-02d-16h-28m-21s UTC integration manifest

No testlist changes.

---- Possible new issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-hsw-4770)

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:429s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:493s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:478s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:457s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:395s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:560s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:495s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:587s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:289s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:260  dwarn:0   dfail:0   fail:1   skip:27  time:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:454s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:414s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:491s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:446s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:496s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:583s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:429s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:517s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:393s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1043/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_pm: Restore runtime pm state on test exit (rev3)
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
                   ` (7 preceding siblings ...)
  (?)
@ 2018-03-05  9:04 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2018-03-05  9:04 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit (rev3)
URL   : https://patchwork.freedesktop.org/series/39106/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
ec872b7dc1fe68d153aaceaf74d55865719a76da tests/perf_pmu: Handle CPU hotplug failures better

with latest DRM-Tip kernel build CI_DRM_3868
c07ef2c77d14 drm-tip: 2018y-03m-03d-08h-39m-05s UTC integration manifest

No testlist changes.

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713 +1
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_chamelium:
        Subgroup dp-edid-read:
                fail       -> PASS       (fi-kbl-7500u) fdo#102505

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#102505 https://bugs.freedesktop.org/show_bug.cgi?id=102505

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:423s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:490s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:280s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:463s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:395s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:418s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:290s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:512s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:391s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:451s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:413s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:493s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:494s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:585s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:423s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:499s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:524s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:409s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:437s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:406s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1050/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for lib/igt_pm: Restore runtime pm state on test exit (rev3)
  2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
                   ` (8 preceding siblings ...)
  (?)
@ 2018-03-05 10:20 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2018-03-05 10:20 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev

== Series Details ==

Series: lib/igt_pm: Restore runtime pm state on test exit (rev3)
URL   : https://patchwork.freedesktop.org/series/39106/
State : success

== Summary ==

---- Possible new issues:

Test kms_vblank:
        Subgroup pipe-a-ts-continuation-suspend:
                skip       -> PASS       (shard-snb)
Test pm_rc6_residency:
        Subgroup rc6-accuracy:
                skip       -> PASS       (shard-snb)

---- Known issues:

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (shard-hsw) fdo#103928
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047
Test kms_vblank:
        Subgroup pipe-a-ts-continuation-suspend:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540

fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540

shard-apl        total:3392 pass:1780 dwarn:1   dfail:0   fail:7   skip:1603 time:11820s
shard-hsw        total:3377 pass:1727 dwarn:1   dfail:0   fail:1   skip:1646 time:11555s
shard-snb        total:3468 pass:1365 dwarn:2   dfail:0   fail:1   skip:2100 time:7099s
Blacklisted hosts:
shard-kbl        total:3392 pass:1901 dwarn:4   dfail:0   fail:7   skip:1479 time:9022s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1050/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2018-03-05 10:20 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 11:08 [PATCH i-g-t] lib/igt_pm: Restore runtime pm state on test exit Tvrtko Ursulin
2018-02-28 11:08 ` [igt-dev] " Tvrtko Ursulin
2018-02-28 11:12 ` Chris Wilson
2018-02-28 11:12   ` [igt-dev] [Intel-gfx] " Chris Wilson
2018-02-28 11:38   ` Tvrtko Ursulin
2018-02-28 11:38     ` [Intel-gfx] " Tvrtko Ursulin
2018-02-28 12:27     ` Chris Wilson
2018-02-28 12:27       ` [igt-dev] [Intel-gfx] " Chris Wilson
2018-02-28 14:33       ` Tvrtko Ursulin
2018-02-28 14:33         ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
2018-02-28 12:50 ` [igt-dev] ✗ Fi.CI.BAT: failure for " Patchwork
2018-02-28 15:17   ` Tvrtko Ursulin
2018-02-28 14:23 ` [PATCH i-g-t] " Chris Wilson
2018-02-28 14:23   ` [igt-dev] [Intel-gfx] " Chris Wilson
2018-02-28 15:35 ` [PATCH i-g-t v2] " Tvrtko Ursulin
2018-02-28 15:35   ` [Intel-gfx] " Tvrtko Ursulin
2018-03-01  8:12   ` Chris Wilson
2018-03-01  8:12     ` [igt-dev] [Intel-gfx] " Chris Wilson
2018-03-02  9:29   ` Imre Deak
2018-03-02  9:29     ` [igt-dev] " Imre Deak
2018-03-02  9:51     ` [PATCH i-g-t v3] " Tvrtko Ursulin
2018-03-02  9:51       ` [Intel-gfx] " Tvrtko Ursulin
2018-03-02  9:56     ` [PATCH i-g-t v2] " Tvrtko Ursulin
2018-03-02  9:56       ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
2018-03-02 12:29       ` Imre Deak
2018-03-02 12:29         ` [igt-dev] [Intel-gfx] " Imre Deak
2018-02-28 16:33 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/igt_pm: Restore runtime pm state on test exit (rev2) Patchwork
2018-02-28 20:22 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
2018-03-02 18:21 ` [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_pm: Restore runtime pm state on test exit (rev3) Patchwork
2018-03-05  9:04 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
2018-03-05 10:20 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.