* [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper
@ 2014-10-14 19:12 Paulo Zanoni
2014-10-21 15:18 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Paulo Zanoni @ 2014-10-14 19:12 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
If the signal helper is active, the usleep() calls return earlier, and
we may end up returning false way before the 10s timeout, failing the
subtests. This currently happens on the kms_flip RPM interruptible
subtests.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78893
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
lib/igt_aux.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index b32297e..01654f0 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -42,6 +42,7 @@
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
+#include <sys/time.h>
#include <sys/types.h>
#include <sys/syscall.h>
#include <sys/utsname.h>
@@ -502,21 +503,27 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
* Waits until for the driver to switch to into the desired runtime PM status,
* with a 10 second timeout.
*
+ * Some subtests call this function while the signal helper is active, so we
+ * can't assume each usleep() call will sleep for 100ms.
+ *
* Returns:
* True if the desired runtime PM status was attained, false if the operation
* timed out.
*/
bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
{
- int i;
- int hundred_ms = 100 * 1000, ten_s = 10 * 1000 * 1000;
+ struct timeval start, end, diff;
- for (i = 0; i < ten_s; i += hundred_ms) {
+ igt_assert(gettimeofday(&start, NULL) == 0);
+ do {
if (igt_get_runtime_pm_status() == status)
return true;
- usleep(hundred_ms);
- }
+ usleep(100 * 1000);
+
+ igt_assert(gettimeofday(&end, NULL) == 0);
+ timersub(&end, &start, &diff);
+ } while (diff.tv_sec < 10);
return false;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper
2014-10-14 19:12 [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper Paulo Zanoni
@ 2014-10-21 15:18 ` Daniel Vetter
2014-10-21 15:52 ` Paulo Zanoni
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-10-21 15:18 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Tue, Oct 14, 2014 at 04:12:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If the signal helper is active, the usleep() calls return earlier, and
> we may end up returning false way before the 10s timeout, failing the
> subtests. This currently happens on the kms_flip RPM interruptible
> subtests.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78893
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> lib/igt_aux.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index b32297e..01654f0 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -42,6 +42,7 @@
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/wait.h>
> +#include <sys/time.h>
> #include <sys/types.h>
> #include <sys/syscall.h>
> #include <sys/utsname.h>
> @@ -502,21 +503,27 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
> * Waits until for the driver to switch to into the desired runtime PM status,
> * with a 10 second timeout.
> *
> + * Some subtests call this function while the signal helper is active, so we
> + * can't assume each usleep() call will sleep for 100ms.
> + *
> * Returns:
> * True if the desired runtime PM status was attained, false if the operation
> * timed out.
> */
> bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
> {
> - int i;
> - int hundred_ms = 100 * 1000, ten_s = 10 * 1000 * 1000;
> + struct timeval start, end, diff;
>
> - for (i = 0; i < ten_s; i += hundred_ms) {
> + igt_assert(gettimeofday(&start, NULL) == 0);
> + do {
> if (igt_get_runtime_pm_status() == status)
> return true;
>
> - usleep(hundred_ms);
> - }
> + usleep(100 * 1000);
> +
> + igt_assert(gettimeofday(&end, NULL) == 0);
> + timersub(&end, &start, &diff);
Should we have a generic igt_usleep helper which is signal-robust? Just a
quick idea since there's probably more like these.
-Daniel
> + } while (diff.tv_sec < 10);
>
> return false;
> }
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper
2014-10-21 15:18 ` Daniel Vetter
@ 2014-10-21 15:52 ` Paulo Zanoni
2014-10-21 16:04 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Paulo Zanoni @ 2014-10-21 15:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
2014-10-21 13:18 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Oct 14, 2014 at 04:12:22PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If the signal helper is active, the usleep() calls return earlier, and
>> we may end up returning false way before the 10s timeout, failing the
>> subtests. This currently happens on the kms_flip RPM interruptible
>> subtests.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78893
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> lib/igt_aux.c | 17 ++++++++++++-----
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
>> index b32297e..01654f0 100644
>> --- a/lib/igt_aux.c
>> +++ b/lib/igt_aux.c
>> @@ -42,6 +42,7 @@
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <sys/wait.h>
>> +#include <sys/time.h>
>> #include <sys/types.h>
>> #include <sys/syscall.h>
>> #include <sys/utsname.h>
>> @@ -502,21 +503,27 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
>> * Waits until for the driver to switch to into the desired runtime PM status,
>> * with a 10 second timeout.
>> *
>> + * Some subtests call this function while the signal helper is active, so we
>> + * can't assume each usleep() call will sleep for 100ms.
>> + *
>> * Returns:
>> * True if the desired runtime PM status was attained, false if the operation
>> * timed out.
>> */
>> bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
>> {
>> - int i;
>> - int hundred_ms = 100 * 1000, ten_s = 10 * 1000 * 1000;
>> + struct timeval start, end, diff;
>>
>> - for (i = 0; i < ten_s; i += hundred_ms) {
>> + igt_assert(gettimeofday(&start, NULL) == 0);
>> + do {
>> if (igt_get_runtime_pm_status() == status)
>> return true;
>>
>> - usleep(hundred_ms);
>> - }
>> + usleep(100 * 1000);
>> +
>> + igt_assert(gettimeofday(&end, NULL) == 0);
>> + timersub(&end, &start, &diff);
>
> Should we have a generic igt_usleep helper which is signal-robust? Just a
> quick idea since there's probably more like these.
If we think we could use it in many places, it makes sense. I'm not
aware yet of where we could use it.
> -Daniel
>
>> + } while (diff.tv_sec < 10);
>>
>> return false;
>> }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper
2014-10-21 15:52 ` Paulo Zanoni
@ 2014-10-21 16:04 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-10-21 16:04 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
On Tue, Oct 21, 2014 at 01:52:46PM -0200, Paulo Zanoni wrote:
> 2014-10-21 13:18 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Oct 14, 2014 at 04:12:22PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If the signal helper is active, the usleep() calls return earlier, and
> >> we may end up returning false way before the 10s timeout, failing the
> >> subtests. This currently happens on the kms_flip RPM interruptible
> >> subtests.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78893
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >> lib/igt_aux.c | 17 ++++++++++++-----
> >> 1 file changed, 12 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> >> index b32297e..01654f0 100644
> >> --- a/lib/igt_aux.c
> >> +++ b/lib/igt_aux.c
> >> @@ -42,6 +42,7 @@
> >> #include <stdlib.h>
> >> #include <unistd.h>
> >> #include <sys/wait.h>
> >> +#include <sys/time.h>
> >> #include <sys/types.h>
> >> #include <sys/syscall.h>
> >> #include <sys/utsname.h>
> >> @@ -502,21 +503,27 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
> >> * Waits until for the driver to switch to into the desired runtime PM status,
> >> * with a 10 second timeout.
> >> *
> >> + * Some subtests call this function while the signal helper is active, so we
> >> + * can't assume each usleep() call will sleep for 100ms.
> >> + *
> >> * Returns:
> >> * True if the desired runtime PM status was attained, false if the operation
> >> * timed out.
> >> */
> >> bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
> >> {
> >> - int i;
> >> - int hundred_ms = 100 * 1000, ten_s = 10 * 1000 * 1000;
> >> + struct timeval start, end, diff;
> >>
> >> - for (i = 0; i < ten_s; i += hundred_ms) {
> >> + igt_assert(gettimeofday(&start, NULL) == 0);
> >> + do {
> >> if (igt_get_runtime_pm_status() == status)
> >> return true;
> >>
> >> - usleep(hundred_ms);
> >> - }
> >> + usleep(100 * 1000);
> >> +
> >> + igt_assert(gettimeofday(&end, NULL) == 0);
> >> + timersub(&end, &start, &diff);
> >
> > Should we have a generic igt_usleep helper which is signal-robust? Just a
> > quick idea since there's probably more like these.
>
> If we think we could use it in many places, it makes sense. I'm not
> aware yet of where we could use it.
Do a cocci patch to replace all the existing usleep with igt_usleep
extracted from here. Run it on tests/*.c.
We already have a cocci spatch in lib/igt.cocci with lots of default
refactorings like that to replace C functions with igt helpers where it
makes sense. Signed up? And even if it's just to learn a bit how
coccinelle works, that alone is awesome ;-)
-Daniel
>
> > -Daniel
> >
> >> + } while (diff.tv_sec < 10);
> >>
> >> return false;
> >> }
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-21 16:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-14 19:12 [PATCH] lib/igt_aux: make igt_wait_for_pm_status() resist the signal helper Paulo Zanoni
2014-10-21 15:18 ` Daniel Vetter
2014-10-21 15:52 ` Paulo Zanoni
2014-10-21 16:04 ` Daniel Vetter
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.