Merge lp:~kdub/powerd/fix-1258655 into lp:powerd

[Kevin DuBois]

1) eliminating fb_state as a global variable and locking actual_display_state properly.
2) for earlysuspend devices, the responsibility for changing state and turning on the display/compositor via dbus is in the thread-monitoring state. for non-earlysuspend devices, that responsibility remains in the same thread that it is in now.

fixes ( LP: #1258655 )

powerd must ensure that the system is in a usable state before triggering the calls to the display server.

The problem was that for earlysuspend devices, there were two threads that thought they had responsibility for turning the display on, when really only the thread monitoring the files in /sys/power had the right to do so. Furthermore, the variables concerned with display state "actual_display_state" and "fb_state" were accessed by both the file-monitoring thread and the the request-servicing thread, causing races around the actual state of the display.

This MP fixes these issues by:
1) eliminating fb_state as a global variable and locking actual_display_state properly.
2) for earlysuspend devices, the responsibility for changing state and turning on the display/compositor via dbus is in the thread-monitoring state. for non-earlysuspend devices, that responsibility remains in the same thread that it is in now.

fixes ( LP: #1258655 )

Also, please feel free to test by jamming on the power button repeatedly and quickly. I have manually tested on my nexus 4 (an early suspend device) and my nexus 10 (a non-early suspend device).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:108
http://jenkins.qa.ubuntu.com/job/powerd-ci/132/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/2/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/powerd-ci/132/rebuild

review: Approve (continuous-integration)

[Seth Forshee (sforshee)]

I don't think this actually "fixes" anything, I think it probably just
shuffles timing around and ends up somehow hiding the problem.

On Wed, Dec 18, 2013 at 10:40:39PM -0000, Kevin DuBois wrote:
> Kevin DuBois has proposed merging lp:~kdub/powerd/fix-1258655 into lp:powerd.
>
> Commit message:
> 1) eliminating fb_state as a global variable and locking actual_display_state properly.

Previously all updates to actual_display_state happened on the main
loop. There were a couple of reads off the main loop related to power
button handling, but adding a lock doesn't eliminate any class of races
(i.e. the state changes just after the read). So actual_screen_state
doesn't *need* any locking until you start manipulating it from
wait_for_fb_thread(). Even then I find most of the locking to be
dubious, more on that later.

> powerd must ensure that the system is in a usable state before
> triggering the calls to the display server. The problem was that
> for earlysuspend devices, there were two threads that thought they had
> responsibility for turning the display on, when really only the thread
> monitoring the files in /sys/power had the right to do so. Furthermore,
> the variables concerned with display state "actual_display_state" and
> "fb_state" were accessed by both the file-monitoring thread and the the
> request-servicing thread, causing races around the actual state of the
> display.

Previously only the main thread ever turned the display on/off; the
monitoring thready only scheduled work on the main thread to do so. The
monitoring thread never checks actual_screen_state, only the work which
is scheduled on the main thread does this. Therefore there is no race in
accessing actual_screen_state, since it's always happening on the main
thread.

This change actually reduces serialization. Previously changes to the
display state and to the system power state were serialized relative to
each other by nature of both happening on the main thread. It _should_
be impossible to have earlysuspend enabled while the display was on due
the following:

 * Before the display was turned on update_display_state() requested the
   active state, which would synchronously disable earlysuspend before
   proceeding to turn on the display.
 * The display was turned off before releasing the active state request.
   Releasing this request may cause the system to be put into
   suspend/earlysuspend mode.
 * All of this happens on the main thread and is thus serialized

These changes move the display on/off to another thread, so the
guarantees of the display state being serialized wrt the system power
state are broken. There *are* races between turning the screen on on the
monitor thread and turning it off on the main thread with your changes.
Expanding the amount of code serialized by the mutex in the screen off
case could help some, but I'm not sure that it will ever be entirely
sufficient.

My guess would be that the actual problem is that unity's
setScreenPowerMode() doesn't complete synchronously wrt what powerd is
doing. So something like this might happen:

 * powerd takes an active state request to guarantee that earlysuspend
   is disabled, then calls setScre...

[Seth Forshee (sforshee)]

Disapproving as per the comments I sent via email. I'm not convinced that the premise behind these changes is correct, but I'm open to being convinced otherwise. In any case I'm almost certain that the changes will introduce regressions.

review: Disapprove

[Seth Forshee (sforshee)]

I've been going back over the code, and as long as the dbus calls are handled on the main thread (which was my understanding, but I'm no expert about dbus) then all display on/off code should happen on the main thread. As far as I can tell there are adequate protections to prevent turning on the display when earlysuspend is enabled. I really don't think there are any problems there. In particular the display code holds an active request when unity's setScreenPowerMode() is called, and it's called on the main loop, so there should be absolutely no way that earlysuspend is active when it is called and there should be absolutely no way for earlysuspend to become active until the dbus call returns (and setScreenPowerMode() *is* called synchronously).

I did just remember though bug #1208433, which may be related. I think rsalveti and I chatted about that one once and we both thought that the root cause was probably something in the display driver. maguro and grouper are both earlysuspend devices and neither have the same problem with rapidly pressing the power key. Are you able to try either of those with unity to verify that's still the case?

[Kevin DuBois (kdub)]

unity-mir gets two on's from one button press, one from http://bazaar.launchpad.net/~phablet-team/powerd/trunk/view/head:/src/display.c#L415 and one from http://bazaar.launchpad.net/~phablet-team/powerd/trunk/view/head:/src/display.c#L263. The two on's don't happen everytime, they happen like races.

This isn't a dbus race, that code all checked out (best I can tell). Its that two requests are actually sent out in response to a button press. Sometimes the FB is not ready when one of these 'on' requests happens.

From mir's perspective, all the errors emanate from the FB device not being able to post/blank/etc. Best I can tell, the FB is sometimes not available from a power perspective when one of the on calls comes.

my theory about this problem goes something like this:

power button press
update_display_state()
request_sys_state_internal()
   (request to come out of suspend)
display_set_power_mode()
                                   fb comes out of suspend
                                   fb monitor thread calls update_display_state()

I can check the other devices

[Seth Forshee (sforshee)]

On Thu, Dec 19, 2013 at 05:54:59PM -0000, Kevin DuBois wrote:
> unity-mir gets two on's from one button press, one from http://bazaar.launchpad.net/~phablet-team/powerd/trunk/view/head:/src/display.c#L415 and one from http://bazaar.launchpad.net/~phablet-team/powerd/trunk/view/head:/src/display.c#L263. The two on's don't happen everytime, they happen like races.
>
> This isn't a dbus race, that code all checked out (best I can tell). Its that two requests are actually sent out in response to a button press. Sometimes the FB is not ready when one of these 'on' requests happens.
>
> >From mir's perspective, all the errors emanate from the FB device not being able to post/blank/etc. Best I can tell, the FB is sometimes not available from a power perspective when one of the on calls comes.

With reference to my comments on the bug, I suspect the first call comes
before the earlysuspend gets around to suspending the framebuffer, and
the second comes when it has been suspended and then resumed. It's a
race with the earlysuspend code in the kernel, which turns out to be
asynchronous with writing to /sys/power/state.

[Kevin DuBois (kdub)]

Seth came up with another fix in https://code.launchpad.net/~sforshee/powerd/fix-display-timing/+merge/200431
so moving to rejected.

Unmerged revisions

108. By Kevin DuBois on 2013-12-18

merge base

107. By Kevin DuBois on 2013-12-18

further cleanup, make sure non-early suspend devices still work

106. By Kevin DuBois on 2013-12-18

bit of cleanup in fb thread

105. By Kevin DuBois on 2013-12-18

change state handling to make more robust display interaction

104. By Kevin DuBois on 2013-12-18

add some locking to the variables accessed in both therads

=== modified file 'src/display.c'
--- src/display.c	2013-10-10 15:35:12 +0000
+++ src/display.c	2013-12-18 22:40:08 +0000
@@ -54,6 +54,7 @@
 static gint target_brightness;
 static gint dim_brightness;
 static gboolean running_mir = FALSE;
+static gboolean earlysuspend_used = FALSE;
 
 /* Assume screen is off to start; we will force it on */
 static uuid_t internal_request_cookie;
@@ -64,6 +65,8 @@
     .flags = 0,
 };
 
+GMutex actual_screen_state_guard;
+
 /*
  * When using the proximity sensor, our actual state of the screen
  * may not match the requested state in internal_state. Therefore
@@ -77,19 +80,6 @@
  */
 static unsigned screen_off_overrides;
 
-enum fb_state {
-    FB_SLEEP,
-    FB_AWAKE,
-
-    NUM_FB_STATES
-};
-static enum fb_state fb_state = FB_AWAKE;
-
-static const char *fb_file_names[] = {
-    [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
-    [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
-};
-
 static const char *display_state_strs[POWERD_NUM_DISPLAY_STATES] = {
     [POWERD_DISPLAY_STATE_DONT_CARE]    = "off",
     [POWERD_DISPLAY_STATE_ON]           = "on",
@@ -107,6 +97,10 @@
     return (flags & AB_ENABLED_MASK) == AB_ENABLED;
 }
 
+
+/* the Display Server (mir or sf) is free to fire off a few more frames as 
+   part of its shutdown process. powerd must guarantee the fb_state is FB_AWAKE,
+   and the system is not suspended until the request returns. */
 gboolean display_set_power_mode(int display, char *power_mode)
 {
     GError *error = NULL;
@@ -170,7 +164,10 @@
 
 gboolean powerd_display_enabled(void)
 {
-    return actual_screen_state == POWERD_DISPLAY_STATE_ON;
+    g_mutex_lock(&actual_screen_state_guard);
+    gboolean ret = (actual_screen_state == POWERD_DISPLAY_STATE_ON);
+    g_mutex_unlock(&actual_screen_state_guard);
+    return ret;
 }
 
 static int get_target_brightness(gboolean request_bright, int hw_value)
@@ -221,8 +218,8 @@
 
     switch (applied_state) {
     case DISPLAY_STATE_OFF:
-        if (actual_screen_state == DISPLAY_STATE_OFF) {
-            /* Nothing to do */
+        if (!powerd_display_enabled()) {
+            /* Nothing to do, display is alread off */
             return;
         }
 
@@ -230,11 +227,19 @@
         if (using_ab)
             powerd_autobrightness_disable();
         powerd_set_brightness(0);
+
         if (!display_set_power_mode(0, "off")) {
             powerd_warn("failed to set display power mode, not clearing state");
             return;
         }
 
+        if (!earlysuspend_used)
+        {
+            g_mutex_lock(&actual_screen_state_guard);
+            actual_screen_state = DISPLAY_STATE_OFF;
+            g_mutex_unlock(&actual_screen_state_guard);
+        }
+
         powerd_debug("Releasing internal active state request");
         ret = clear_sys_state_internal(internal_request_cookie);
         if (!ret) {
@@ -245,7 +250,8 @@
         }
         break;
     case POWERD_DISPLAY_STATE_ON:
-        if (actual_screen_state != POWERD_DISPLAY_STATE_ON) {
+        if (!powerd_display_enabled()) {
+            /* display is off, must turn display on */
             powerd_debug("Requesting active state internally");
             ret = request_sys_state_internal("display-request",
                                              POWERD_SYS_STATE_ACTIVE,
@@ -253,16 +259,18 @@
             if (!ret)
                 powerd_warn("Request for active state failed");
 
-            /*
-             * If the current state is suspend we need to wait for
-             * notification of leaving the active state, otherwise the
-             * screen may fail to turn on for devices using earlysuspend.
-             * Otherwise we can turn on the screen right away.
-             */
-            if (fb_state == FB_AWAKE)
+            /* if the device uses earlysuspend, responsibility for turning
+               the display server and changing actual_screen_state falls
+               to wait_for_fb_thread() */
+            if (!earlysuspend_used)
+            {
                 turn_on_display(use_ab);
+                g_mutex_lock(&actual_screen_state_guard);
+                actual_screen_state = POWERD_DISPLAY_STATE_ON;
+                g_mutex_unlock(&actual_screen_state_guard);
+            }
         } else {
-            /* Only changing brightness */
+            /* display is already on, must only changing brightness */
             if (use_ab) {
                 if (!using_ab)
                     powerd_autobrightness_enable();
@@ -281,8 +289,6 @@
         powerd_warn("Invalid display state %d", applied_state);
         return;
     }
-
-    actual_screen_state = applied_state;
 }
 
 static void update_flags(guint32 flags)
@@ -389,7 +395,6 @@
     powerd_run_mainloop_sync(apply_override_worker, &ovr_data);
 }
 
-
 static int wait_for_file(const char *fname)
 {
     int fd, ret;
@@ -407,31 +412,43 @@
     return ret == -1 ? -errno : 0;
 }
 
-static gboolean fb_state_changed(gpointer data)
-{
-    fb_state = (enum fb_state)data;
-    powerd_debug("fb state %s", fb_state == FB_SLEEP ? "sleep" : "awake");
-    if (fb_state == FB_AWAKE && powerd_display_enabled())
-        turn_on_display(ab_enabled(internal_state.flags));
-    return FALSE;
-}
+enum fb_state {
+    FB_SLEEP,
+    FB_AWAKE,
+
+    NUM_FB_STATES
+};
+
+static const char *fb_file_names[] = {
+    [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
+    [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
+};
 
 static gpointer wait_for_fb_thread(gpointer unused)
 {
-    unsigned long state = fb_state;
-
-    if (state >= NUM_FB_STATES) {
-        powerd_warn("Invalid FB state");
-        return NULL;
-    }
-
+    enum fb_state fb_state = FB_AWAKE;
     while (1) {
-        state = (state == FB_SLEEP) ? FB_AWAKE : FB_SLEEP;
-        if (wait_for_file(fb_file_names[state])) {
-            powerd_warn("Error reading %s", fb_file_names[state]);
+        enum fb_state next_state = (fb_state == FB_SLEEP) ? FB_AWAKE : FB_SLEEP;
+
+        if (wait_for_file(fb_file_names[next_state])) {
+            powerd_warn("Error reading %s", fb_file_names[next_state]);
             return NULL;
         }
-        g_timeout_add(0, fb_state_changed, (gpointer)state);
+
+        fb_state = next_state;
+        powerd_debug("fb state %s", fb_state == FB_SLEEP ? "sleep" : "awake");
+
+        g_mutex_lock(&actual_screen_state_guard);
+        if (fb_state == FB_AWAKE) 
+        {
+            turn_on_display(ab_enabled(internal_state.flags));
+            actual_screen_state = POWERD_DISPLAY_STATE_ON;
+        }
+        else if (fb_state == FB_SLEEP)
+        {
+            actual_screen_state = DISPLAY_STATE_OFF;
+        }
+        g_mutex_unlock(&actual_screen_state_guard);
     }
     return NULL;
 }
@@ -489,7 +506,15 @@
 
     /* If kernel supports earlysuspend, start thread to monitor fb state */
     if (!access(fb_file_names[FB_SLEEP], F_OK))
+    {
+        powerd_debug("device uses earlysuspend");
+        earlysuspend_used = TRUE;
         g_thread_new("powerd_fb_wake_monitor", wait_for_fb_thread, &error);
+    } else
+    {
+        powerd_debug("device does not use earlysuspend");
+        earlysuspend_used = FALSE;
+    }
 
     return 0;
 }