simplify further vsync handling

- we now clean-up "dead" connection in the main loop,
this entirely avoid the problem with the side effects of
releasing strong references. We now only hold on to strong
reference for the connection we will signal.

- also simplify how we build the list of "ready" connections, by
only adding them to the list when we did receive a vsync event

Change-Id: I2a84da431320a2af8e8a93e07622a1d258236f43

1 files changed, 75 insertions, 84 deletions

diff --git a/services/surfaceflinger/EventThread.cpp b/services/surfaceflinger/EventThread.cpp
index 59390c0..a39b7d8 100644
--- a/services/surfaceflinger/EventThread.cpp
+++ b/services/surfaceflinger/EventThread.cpp
@@ -60,14 +60,6 @@ status_t EventThread::registerDisplayEventConnection(
     return NO_ERROR;
 }
 
-status_t EventThread::unregisterDisplayEventConnection(
-        const wp<EventThread::Connection>& connection) {
-    Mutex::Autolock _l(mLock);
-    mDisplayEventConnections.remove(connection);
-    mCondition.broadcast();
-    return NO_ERROR;
-}
-
 void EventThread::removeDisplayEventConnection(
         const wp<EventThread::Connection>& connection) {
     Mutex::Autolock _l(mLock);
@@ -122,117 +114,115 @@ void EventThread::onVSyncReceived(int, nsecs_t timestamp) {
 }
 
 bool EventThread::threadLoop() {
-
-    nsecs_t timestamp;
-    size_t vsyncCount;
     DisplayEventReceiver::Event vsync;
-    Vector< sp<EventThread::Connection> > activeConnections;
     Vector< sp<EventThread::Connection> > signalConnections;
+    signalConnections = waitForEvent(&vsync);
 
-    do {
-        // release our references
-        signalConnections.clear();
-        activeConnections.clear();
+    // dispatch vsync events to listeners...
+    const size_t count = signalConnections.size();
+    for (size_t i=0 ; i<count ; i++) {
+        const sp<Connection>& conn(signalConnections[i]);
+        // now see if we still need to report this VSYNC event
+        status_t err = conn->postEvent(vsync);
+        if (err == -EAGAIN || err == -EWOULDBLOCK) {
+            // The destination doesn't accept events anymore, it's probably
+            // full. For now, we just drop the events on the floor.
+            // Note that some events cannot be dropped and would have to be
+            // re-sent later. Right-now we don't have the ability to do
+            // this, but it doesn't matter for VSYNC.
+        } else if (err < 0) {
+            // handle any other error on the pipe as fatal. the only
+            // reasonable thing to do is to clean-up this connection.
+            // The most common error we'll get here is -EPIPE.
+            removeDisplayEventConnection(signalConnections[i]);
+        }
+    }
+    return true;
+}
 
-        Mutex::Autolock _l(mLock);
+Vector< sp<EventThread::Connection> > EventThread::waitForEvent(
+        DisplayEventReceiver::Event* event)
+{
+    Mutex::Autolock _l(mLock);
+
+    size_t vsyncCount;
+    nsecs_t timestamp;
+    Vector< sp<EventThread::Connection> > signalConnections;
 
+    do {
         // latch VSYNC event if any
         bool waitForVSync = false;
         vsyncCount = mVSyncCount;
         timestamp = mVSyncTimestamp;
         mVSyncTimestamp = 0;
 
-        // find out connections waiting for VSYNC events
+        // find out connections waiting for events
         size_t count = mDisplayEventConnections.size();
         for (size_t i=0 ; i<count ; i++) {
             sp<Connection> connection(mDisplayEventConnections[i].promote());
             if (connection != NULL) {
-                activeConnections.add(connection);
                 if (connection->count >= 0) {
                     // we need vsync events because at least
                     // one connection is waiting for it
                     waitForVSync = true;
-                    if (connection->count == 0) {
-                        // fired this time around
-                        if (timestamp) {
-                            // only "consume" this event if we're going to
-                            // report it
+                    if (timestamp) {
+                        // we consume the event only if it's time
+                        // (ie: we received a vsync event)
+                        if (connection->count == 0) {
+                            // fired this time around
                             connection->count = -1;
+                            signalConnections.add(connection);
+                        } else if (connection->count == 1 ||
+                                (vsyncCount % connection->count) == 0) {
+                            // continuous event, and time to report it
+                            signalConnections.add(connection);
                         }
-                        signalConnections.add(connection);
-                    } else if (connection->count == 1 ||
-                            (vsyncCount % connection->count) == 0) {
-                        // continuous event, and time to report it
-                        signalConnections.add(connection);
                     }
                 }
+            } else {
+                // we couldn't promote this reference, the connection has
+                // died, so clean-up!
+                mDisplayEventConnections.removeAt(i);
+                --i; --count;
             }
         }
 
-        if (timestamp) {
-            // we have a vsync event we can dispatch
-            if (!waitForVSync) {
-                // we received a VSYNC but we have no clients
-                // don't report it, and disable VSYNC events
-                disableVSyncLocked();
-            } else {
-                // report VSYNC event
-                break;
-            }
-        } else {
-            // never disable VSYNC events immediately, instead
-            // we'll wait to receive the event and we'll
-            // reevaluate whether we need to dispatch it and/or
-            // disable VSYNC events then.
-            if (waitForVSync) {
-                // enable
-                enableVSyncLocked();
-            }
+        // Here we figure out if we need to enable or disable vsyncs
+        if (timestamp && !waitForVSync) {
+            // we received a VSYNC but we have no clients
+            // don't report it, and disable VSYNC events
+            disableVSyncLocked();
+        } else if (!timestamp && waitForVSync) {
+            enableVSyncLocked();
         }
 
-        // wait for something to happen
-        if (CC_UNLIKELY(mUseSoftwareVSync && waitForVSync)) {
-            // h/w vsync cannot be used (screen is off), so we use
-            // a  timeout instead. it doesn't matter how imprecise this
-            // is, we just need to make sure to serve the clients
-            if (mCondition.waitRelative(mLock, ms2ns(16)) == TIMED_OUT) {
-                mVSyncTimestamp = systemTime(SYSTEM_TIME_MONOTONIC);
-                mVSyncCount++;
-            }
-        } else {
-            if (!timestamp || signalConnections.isEmpty()) {
+        // note: !timestamp implies signalConnections.isEmpty()
+        if (!timestamp) {
+            // wait for something to happen
+            if (CC_UNLIKELY(mUseSoftwareVSync && waitForVSync)) {
+                // h/w vsync cannot be used (screen is off), so we use
+                // a  timeout instead. it doesn't matter how imprecise this
+                // is, we just need to make sure to serve the clients
+                if (mCondition.waitRelative(mLock, ms2ns(16)) == TIMED_OUT) {
+                    mVSyncTimestamp = systemTime(SYSTEM_TIME_MONOTONIC);
+                    mVSyncCount++;
+                }
+            } else {
                 // This is where we spend most of our time, waiting
                 // for a vsync events and registered clients
                 mCondition.wait(mLock);
             }
         }
-    } while (!timestamp || signalConnections.isEmpty());
+    } while (signalConnections.isEmpty());
 
-    // dispatch vsync events to listeners...
-    vsync.header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
-    vsync.header.timestamp = timestamp;
-    vsync.vsync.count = vsyncCount;
+    // here we're guaranteed to have a timestamp and some connections to signal
 
-    const size_t count = signalConnections.size();
-    for (size_t i=0 ; i<count ; i++) {
-        const sp<Connection>& conn(signalConnections[i]);
-        // now see if we still need to report this VSYNC event
-        status_t err = conn->postEvent(vsync);
-        if (err == -EAGAIN || err == -EWOULDBLOCK) {
-            // The destination doesn't accept events anymore, it's probably
-            // full. For now, we just drop the events on the floor.
-            // Note that some events cannot be dropped and would have to be
-            // re-sent later. Right-now we don't have the ability to do
-            // this, but it doesn't matter for VSYNC.
-        } else if (err < 0) {
-            // handle any other error on the pipe as fatal. the only
-            // reasonable thing to do is to clean-up this connection.
-            // The most common error we'll get here is -EPIPE.
-            removeDisplayEventConnection(signalConnections[i]);
-        }
-    }
+    // dispatch vsync events to listeners...
+    event->header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
+    event->header.timestamp = timestamp;
+    event->vsync.count = vsyncCount;
 
-    return true;
+    return signalConnections;
 }
 
 void EventThread::enableVSyncLocked() {
@@ -280,7 +270,8 @@ EventThread::Connection::Connection(
 }
 
 EventThread::Connection::~Connection() {
-    mEventThread->unregisterDisplayEventConnection(this);
+    // do nothing here -- clean-up will happen automatically
+    // when the main thread wakes up
 }
 
 void EventThread::Connection::onFirstRef() {