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.
This commit is contained in:
parent
53a67e1663
commit
d1a99ec6b2
|
@ -105,7 +105,7 @@ public:
|
|||
volatile int32_t head; // server's current front buffer
|
||||
volatile int32_t available; // number of dequeue-able buffers
|
||||
volatile int32_t queued; // number of buffers waiting for post
|
||||
volatile int32_t inUse; // buffer currently in use by SF
|
||||
volatile int32_t reserved1;
|
||||
volatile status_t status; // surface's status code
|
||||
|
||||
// not part of the conditions
|
||||
|
@ -275,7 +275,6 @@ public:
|
|||
int32_t identity);
|
||||
|
||||
ssize_t retireAndLock();
|
||||
status_t unlock(int buffer);
|
||||
void setStatus(status_t status);
|
||||
status_t reallocateAll();
|
||||
status_t reallocateAllExcept(int buffer);
|
||||
|
@ -346,11 +345,6 @@ private:
|
|||
int mNumBuffers;
|
||||
BufferList mBufferList;
|
||||
|
||||
struct UnlockUpdate : public UpdateBase {
|
||||
const int lockedBuffer;
|
||||
inline UnlockUpdate(SharedBufferBase* sbb, int lockedBuffer);
|
||||
inline ssize_t operator()();
|
||||
};
|
||||
|
||||
struct RetireUpdate : public UpdateBase {
|
||||
const int numBuffers;
|
||||
|
|
|
@ -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) {
|
||||
|
|
|
@ -682,22 +682,6 @@ void Layer::unlockPageFlip(
|
|||
}
|
||||
}
|
||||
|
||||
void Layer::finishPageFlip()
|
||||
{
|
||||
ClientRef::Access sharedClient(mUserClientRef);
|
||||
SharedBufferServer* lcblk(sharedClient.get());
|
||||
if (lcblk) {
|
||||
int buf = mBufferManager.getActiveBufferIndex();
|
||||
if (buf >= 0) {
|
||||
status_t err = lcblk->unlock( buf );
|
||||
LOGE_IF(err!=NO_ERROR,
|
||||
"layer %p, buffer=%d wasn't locked!",
|
||||
this, buf);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
void Layer::dump(String8& result, char* buffer, size_t SIZE) const
|
||||
{
|
||||
LayerBaseClient::dump(result, buffer, SIZE);
|
||||
|
|
|
@ -73,7 +73,6 @@ public:
|
|||
virtual uint32_t doTransaction(uint32_t transactionFlags);
|
||||
virtual void lockPageFlip(bool& recomputeVisibleRegions);
|
||||
virtual void unlockPageFlip(const Transform& planeTransform, Region& outDirtyRegion);
|
||||
virtual void finishPageFlip();
|
||||
virtual bool needsBlending() const { return mNeedsBlending; }
|
||||
virtual bool needsDithering() const { return mNeedsDithering; }
|
||||
virtual bool needsFiltering() const;
|
||||
|
|
|
@ -273,10 +273,6 @@ void LayerBase::unlockPageFlip(
|
|||
}
|
||||
}
|
||||
|
||||
void LayerBase::finishPageFlip()
|
||||
{
|
||||
}
|
||||
|
||||
void LayerBase::invalidate()
|
||||
{
|
||||
if ((android_atomic_or(1, &mInvalidate)&1) == 0) {
|
||||
|
|
|
@ -173,11 +173,6 @@ public:
|
|||
*/
|
||||
virtual void unlockPageFlip(const Transform& planeTransform, Region& outDirtyRegion);
|
||||
|
||||
/**
|
||||
* finishPageFlip - called after all surfaces have drawn.
|
||||
*/
|
||||
virtual void finishPageFlip();
|
||||
|
||||
/**
|
||||
* needsBlending - true if this surface needs blending
|
||||
*/
|
||||
|
|
|
@ -396,10 +396,6 @@ bool SurfaceFlinger::threadLoop()
|
|||
logger.log(GraphicLog::SF_COMPOSITION_COMPLETE, index);
|
||||
hw.compositionComplete();
|
||||
|
||||
// release the clients before we flip ('cause flip might block)
|
||||
logger.log(GraphicLog::SF_UNLOCK_CLIENTS, index);
|
||||
unlockClients();
|
||||
|
||||
logger.log(GraphicLog::SF_SWAP_BUFFERS, index);
|
||||
postFramebuffer();
|
||||
|
||||
|
@ -407,7 +403,6 @@ bool SurfaceFlinger::threadLoop()
|
|||
} else {
|
||||
// pretend we did the post
|
||||
hw.compositionComplete();
|
||||
unlockClients();
|
||||
usleep(16667); // 60 fps period
|
||||
}
|
||||
return true;
|
||||
|
@ -895,17 +890,6 @@ void SurfaceFlinger::composeSurfaces(const Region& dirty)
|
|||
}
|
||||
}
|
||||
|
||||
void SurfaceFlinger::unlockClients()
|
||||
{
|
||||
const LayerVector& drawingLayers(mDrawingState.layersSortedByZ);
|
||||
const size_t count = drawingLayers.size();
|
||||
sp<LayerBase> const* const layers = drawingLayers.array();
|
||||
for (size_t i=0 ; i<count ; ++i) {
|
||||
const sp<LayerBase>& layer = layers[i];
|
||||
layer->finishPageFlip();
|
||||
}
|
||||
}
|
||||
|
||||
void SurfaceFlinger::debugFlashRegions()
|
||||
{
|
||||
const DisplayHardware& hw(graphicPlane(0).displayHardware());
|
||||
|
|
|
@ -310,7 +310,6 @@ private:
|
|||
bool handleBypassLayer();
|
||||
void postFramebuffer();
|
||||
void composeSurfaces(const Region& dirty);
|
||||
void unlockClients();
|
||||
|
||||
|
||||
ssize_t addClientLayer(const sp<Client>& client,
|
||||
|
|
Loading…
Reference in New Issue