fix [3361121] hang in glClear() - device unresponsive, OTA fails (DO NOT MERGE)

Generally we never want to lock a buffer for write access if it is at
the "head" on the surfaceflinger side. The only exception (1) is when
the buffer is not currently in use AND there is at least one queued
buffer -- in which case, SurfaceFlinger will never use said buffer
anymore, because on the next composition around, it will be able to
retire the first queued buffer.

The logic above relies on SurfaceFlinger always retiring
and locking a buffer before composition -- unfortunately this
didn't happen during a screenshot.

This could leave us in a situation where a buffer is locked by the
application for write, and used by SurfaceFlinger for texturing,
causing a hang.

Here, we fix this issue by never assuming the exception (1), it was
intended as an optimization allowing ANativeWindow::lockBuffer() to
return sooner and was justified when most of SF composition was
done in software. The actual buffer locking is now ensured by
gralloc. We could have handled screenshots in a similar way to
a regular composition, but it could have caused glitches on screen,
essentially, taking a screenshot could cause to skip a frame.

now that we removed the notion of a "inUse" buffer in surfaceflinger
a lot of code can be simplified / removed.

noteworthy, the whole concept of "unlockClient" wrt. "compositionComplete"
is also gone.

1 files changed, 3 insertions, 32 deletions

diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp
index 4bc5d9e..b45e43f 100644
--- a/libs/surfaceflinger_client/SharedBufferStack.cpp
+++ b/libs/surfaceflinger_client/SharedBufferStack.cpp
@@ -58,7 +58,6 @@ SharedBufferStack::SharedBufferStack()
 
 void SharedBufferStack::init(int32_t i)
 {
-    inUse = -2;
     status = NO_ERROR;
     identity = i;
 }
@@ -199,9 +198,9 @@ String8 SharedBufferBase::dump(char const* prefix) const
     SharedBufferStack& stack( *mSharedStack );
     snprintf(buffer, SIZE, 
             "%s[ head=%2d, available=%2d, queued=%2d ] "
-            "reallocMask=%08x, inUse=%2d, identity=%d, status=%d",
+            "reallocMask=%08x, identity=%d, status=%d",
             prefix, stack.head, stack.available, stack.queued,
-            stack.reallocMask, stack.inUse, stack.identity, stack.status);
+            stack.reallocMask, stack.identity, stack.status);
     result.append(buffer);
     result.append("\n");
     return result;
@@ -261,8 +260,7 @@ bool SharedBufferClient::LockCondition::operator()() const {
     // NOTE: if stack.head is messed up, we could crash the client
     // or cause some drawing artifacts. This is okay, as long as it is
     // limited to the client.
-    return (buf != stack.index[stack.head] ||
-            (stack.queued > 0 && stack.inUse != buf));
+    return (buf != stack.index[stack.head]);
 }
 
 // ----------------------------------------------------------------------------
@@ -295,22 +293,6 @@ ssize_t SharedBufferClient::CancelUpdate::operator()() {
     return NO_ERROR;
 }
 
-SharedBufferServer::UnlockUpdate::UnlockUpdate(
-        SharedBufferBase* sbb, int lockedBuffer)
-    : UpdateBase(sbb), lockedBuffer(lockedBuffer) {
-}
-ssize_t SharedBufferServer::UnlockUpdate::operator()() {
-    if (stack.inUse != lockedBuffer) {
-        LOGE("unlocking %d, but currently locked buffer is %d "
-             "(identity=%d, token=%d)",
-                lockedBuffer, stack.inUse,
-                stack.identity, stack.token);
-        return BAD_VALUE;
-    }
-    android_atomic_write(-1, &stack.inUse);
-    return NO_ERROR;
-}
-
 SharedBufferServer::RetireUpdate::RetireUpdate(
         SharedBufferBase* sbb, int numBuffers)
     : UpdateBase(sbb), numBuffers(numBuffers) {
@@ -320,9 +302,6 @@ ssize_t SharedBufferServer::RetireUpdate::operator()() {
     if (uint32_t(head) >= SharedBufferStack::NUM_BUFFER_MAX)
         return BAD_VALUE;
 
-    // Preventively lock the current buffer before updating queued.
-    android_atomic_write(stack.headBuf, &stack.inUse);
-
     // Decrement the number of queued buffers 
     int32_t queued;
     do {
@@ -338,7 +317,6 @@ ssize_t SharedBufferServer::RetireUpdate::operator()() {
     head = (head + 1) % numBuffers;
     const int8_t headBuf = stack.index[head];
     stack.headBuf = headBuf;
-    android_atomic_write(headBuf, &stack.inUse);
 
     // head is only modified here, so we don't need to use cmpxchg
     android_atomic_write(head, &stack.head);
@@ -542,13 +520,6 @@ ssize_t SharedBufferServer::retireAndLock()
     return buf;
 }
 
-status_t SharedBufferServer::unlock(int buf)
-{
-    UnlockUpdate update(this, buf);
-    status_t err = updateCondition( update );
-    return err;
-}
-
 void SharedBufferServer::setStatus(status_t status)
 {
     if (status < NO_ERROR) {