[Intel-gfx] [PATCH i-g-t v2] lib/igt_pm: Restore runtime pm state on test exit
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 2 09:56:26 UTC 2018
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.
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list