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

Imre Deak imre.deak at intel.com
Fri Mar 2 12:29:13 UTC 2018


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 at 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 at intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Reviewed-by: Chris Wilson <chris at 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 at 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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 


More information about the Intel-gfx mailing list