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

[Imre Deak]

On Fri, Mar 02, 2018 at 09:56:26AM +, Tvrtko Ursulin wrote:
> 
> On 02/03/2018 09:29, Imre Deak wrote:
> > On Wed, Feb 28, 2018 at 03:35:06PM +, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > Some tests (the ones which call igt_setup_runtime_pm and
> > > igt_pm_enable_audio_runtime_pm) change default system configuration and
> > > never restore it.
> > > 
> > > The configured runtime suspend is aggressive and may influence behaviour
> > > of subsequent tests, so it is better to restore to previous values on test
> > > exit.
> > > 
> > > This way system behaviour, with regards to a random sequence of executed
> > > tests, will be more consistent from one run to another.
> > > 
> > > v2: Read failure means no runtime pm support so don't assert on it.
> > > 
> > > Signed-off-by: Tvrtko Ursulin 
> > > Cc: Imre Deak 
> > > Reviewed-by: Chris Wilson  # 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 
> > 
> > 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/: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 

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

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

[Tvrtko Ursulin]

On 02/03/2018 09:29, Imre Deak wrote:

On Wed, Feb 28, 2018 at 03:35:06PM +, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

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

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

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

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

Signed-off-by: Tvrtko Ursulin 
Cc: Imre Deak 
Reviewed-by: Chris Wilson  # 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 

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/: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/:00:03.0/power/control", O_WRONLY);
+   fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_RDWR);
if (fd >= 0) {
+   igt_assert(read(fd, __igt_pm_audio_runtime_control,
+   sizeof(__igt_pm_audio_runtime_control)) > 0);
+   strchomp(__igt_pm_audio_runtime_control);
igt_assert_eq(write(fd, "auto\n", 5), 5);
close(fd);
}
+
+   igt_debug("Saved audio power management as '%s' and '%s'\n",
+

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

[Imre Deak]

On Wed, Feb 28, 2018 at 03:35:06PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Some tests (the ones which call igt_setup_runtime_pm and
> igt_pm_enable_audio_runtime_pm) change default system configuration and
> never restore it.
> 
> The configured runtime suspend is aggressive and may influence behaviour
> of subsequent tests, so it is better to restore to previous values on test
> exit.
> 
> This way system behaviour, with regards to a random sequence of executed
> tests, will be more consistent from one run to another.
> 
> v2: Read failure means no runtime pm support so don't assert on it.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Imre Deak 
> Reviewed-by: Chris Wilson  # 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 

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_STRLENstrlen(MAX_PERFORMANCE_STR)
>  
> +static char __igt_pm_audio_runtime_power_save[64];
> +static char __igt_pm_audio_runtime_control[64];
> +
> +static void __igt_pm_audio_runtime_exit_handler(int sig)
> +{
> + int fd;
> +
> + igt_debug("Restoring audio power management to '%s' and '%s'\n",
> +   __igt_pm_audio_runtime_power_save,
> +   __igt_pm_audio_runtime_control);
> +
> + fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
> + if (fd < 0)
> + return;
> + if (write(fd, __igt_pm_audio_runtime_power_save,
> +   strlen(__igt_pm_audio_runtime_power_save)) !=
> + strlen(__igt_pm_audio_runtime_power_save))
> + igt_warn("Failed to restore audio power_save to '%s'\n",
> +  __igt_pm_audio_runtime_power_save);
> + close(fd);
> +
> + fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_WRONLY);
> + if (fd < 0)
> + return;
> + if (write(fd, __igt_pm_audio_runtime_control,
> +   strlen(__igt_pm_audio_runtime_control)) !=
> + strlen(__igt_pm_audio_runtime_control))
> + igt_warn("Failed to restore audio control to '%s'\n",
> +  __igt_pm_audio_runtime_control);
> + close(fd);
> +}
> +
> +static void strchomp(char *str)
> +{
> + int len = strlen(str);
> +
> + if (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/:00:03.0/power/control", O_WRONLY);
> + fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_RDWR);
>   if (fd >= 0) {
> + igt_assert(read(fd, __igt_pm_audio_runtime_control,
> + sizeof(__igt_pm_audio_runtime_control)) > 0);
> + strchomp(__igt_pm_audio_runtime_control);
>   igt_assert_eq(write(fd, "auto\n", 5), 5);
>   close(fd);
>   }
> +
> + igt_debug("Saved audio power management as '%s' and '%s'\n",
> +   __igt_pm_audio_runtime_power_save,
> +   __igt_pm_audio_runtime_control);
> +
>   /* Give some time for it to react. */
>   sleep(1);
>  }
> @@ -238,6 +294,38 @@ void igt_pm_restore_sata_link_power_management(int8_t 
> *pm_data)
>  /* We just leak this on exit ... */
>  int pm_status_fd = -1;
>  
> +static char __igt_pm_runtime_autosuspend[64];
> +static char __igt_pm_runtime_control[64];
> +
> +static void __igt_pm_runtime_exit_handler(int sig)
> +{
> 

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

[Chris Wilson]

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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin 

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

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

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

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

Signed-off-by: Tvrtko Ursulin 
Cc: Imre Deak 
Reviewed-by: Chris Wilson  # 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/: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/:00:03.0/power/control", O_WRONLY);
+   fd = open("/sys/bus/pci/devices/:00:03.0/power/control", O_RDWR);
if (fd >= 0) {
+   igt_assert(read(fd, __igt_pm_audio_runtime_control,
+   sizeof(__igt_pm_audio_runtime_control)) > 0);
+   strchomp(__igt_pm_audio_runtime_control);
igt_assert_eq(write(fd, "auto\n", 5), 5);
close(fd);
}
+
+   igt_debug("Saved audio power management as '%s' and '%s'\n",
+ __igt_pm_audio_runtime_power_save,
+ __igt_pm_audio_runtime_control);
+
/* Give some time for it to react. */
sleep(1);
 }
@@ -238,6 +294,38 @@ void igt_pm_restore_sata_link_power_management(int8_t 
*pm_data)
 /* We just leak this on exit ... */
 int pm_status_fd = -1;
 
+static char __igt_pm_runtime_autosuspend[64];
+static char __igt_pm_runtime_control[64];
+
+static void __igt_pm_runtime_exit_handler(int sig)
+{
+   int fd;
+
+   igt_debug("Restoring runtime management to '%s' and '%s'\n",
+ __igt_pm_runtime_autosuspend,
+ __igt_pm_runtime_control);
+
+   fd = open(POWER_DIR "/autosuspend_delay_ms", O_WRONLY);
+   if (fd < 0)
+   return;
+   if (write(fd, __igt_pm_runtime_autosuspend,
+ strlen(__igt_pm_runtime_autosuspend)) !=
+   strlen(__igt_pm_runtime_autosuspend))
+   igt_warn("Failed to restore runtime pm autosuspend delay to 
'%s'\n",
+__igt_pm_runtime_autosuspend);
+   close(fd);
+
+   fd = open(POWER_DIR "/control",