Revert "SurfaceFlinger: protect state members in Layer"
State update transactions must be atomic. The fine-grained lock on each
Layer implied otherwise. Revert the fine grained lock as being
unhelpful.
Unfortunately there does not seem to be a way to use Clang thread
annotations to specify the desired locking behavior.
Note that the parent CL addresses the locking problem that led to the
bug.
This reverts commit 83729883eecd31a9907bc79bc21998a90f17105c.
Bug: 119481871
Test: SurfaceFlinger unit tests
Test: go/wm-smoke
Change-Id: I361741f8d10102aeb57f164c847c6063ff93dd14
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 077be86..e0d9d23 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -72,31 +72,30 @@
}
bool BufferStateLayer::willPresentCurrentTransaction() const {
- Mutex::Autolock lock(mStateMutex);
// Returns true if the most recent Transaction applied to CurrentState will be presented.
return getSidebandStreamChanged() || getAutoRefresh() ||
- (mState.current.modified && mState.current.buffer != nullptr);
+ (mCurrentState.modified && mCurrentState.buffer != nullptr);
}
-bool BufferStateLayer::getTransformToDisplayInverseLocked() const {
- return mState.current.transformToDisplayInverse;
+bool BufferStateLayer::getTransformToDisplayInverse() const {
+ return mCurrentState.transformToDisplayInverse;
}
-void BufferStateLayer::pushPendingStateLocked() {
- if (!mState.current.modified) {
+void BufferStateLayer::pushPendingState() {
+ if (!mCurrentState.modified) {
return;
}
- mState.pending.push_back(mState.current);
- ATRACE_INT(mTransactionName.string(), mState.pending.size());
+ mPendingStates.push_back(mCurrentState);
+ ATRACE_INT(mTransactionName.string(), mPendingStates.size());
}
bool BufferStateLayer::applyPendingStates(Layer::State* stateToCommit) {
- const bool stateUpdateAvailable = !mState.pending.empty();
- while (!mState.pending.empty()) {
+ const bool stateUpdateAvailable = !mPendingStates.empty();
+ while (!mPendingStates.empty()) {
popPendingState(stateToCommit);
}
- mCurrentStateModified = stateUpdateAvailable && mState.current.modified;
- mState.current.modified = false;
+ mCurrentStateModified = stateUpdateAvailable && mCurrentState.modified;
+ mCurrentState.modified = false;
return stateUpdateAvailable;
}
@@ -106,31 +105,28 @@
}
bool BufferStateLayer::setTransform(uint32_t transform) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.transform == transform) return false;
- mState.current.sequence++;
- mState.current.transform = transform;
- mState.current.modified = true;
+ if (mCurrentState.transform == transform) return false;
+ mCurrentState.sequence++;
+ mCurrentState.transform = transform;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setTransformToDisplayInverse(bool transformToDisplayInverse) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.transformToDisplayInverse == transformToDisplayInverse) return false;
- mState.current.sequence++;
- mState.current.transformToDisplayInverse = transformToDisplayInverse;
- mState.current.modified = true;
+ if (mCurrentState.transformToDisplayInverse == transformToDisplayInverse) return false;
+ mCurrentState.sequence++;
+ mCurrentState.transformToDisplayInverse = transformToDisplayInverse;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setCrop(const Rect& crop) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.crop == crop) return false;
- mState.current.sequence++;
- mState.current.crop = crop;
- mState.current.modified = true;
+ if (mCurrentState.crop == crop) return false;
+ mCurrentState.sequence++;
+ mCurrentState.crop = crop;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
@@ -151,94 +147,86 @@
h = frame.bottom;
}
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.active.transform.tx() == x && mState.current.active.transform.ty() == y &&
- mState.current.active.w == w && mState.current.active.h == h) {
+ if (mCurrentState.active.transform.tx() == x && mCurrentState.active.transform.ty() == y &&
+ mCurrentState.active.w == w && mCurrentState.active.h == h) {
return false;
}
if (!frame.isValid()) {
x = y = w = h = 0;
}
- mState.current.active.transform.set(x, y);
- mState.current.active.w = w;
- mState.current.active.h = h;
+ mCurrentState.active.transform.set(x, y);
+ mCurrentState.active.w = w;
+ mCurrentState.active.h = h;
- mState.current.sequence++;
- mState.current.modified = true;
+ mCurrentState.sequence++;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setBuffer(const sp<GraphicBuffer>& buffer) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.buffer) {
+ if (mCurrentState.buffer) {
mReleasePreviousBuffer = true;
}
- mState.current.sequence++;
- mState.current.buffer = buffer;
- mState.current.modified = true;
+ mCurrentState.sequence++;
+ mCurrentState.buffer = buffer;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setAcquireFence(const sp<Fence>& fence) {
- Mutex::Autolock lock(mStateMutex);
// The acquire fences of BufferStateLayers have already signaled before they are set
mCallbackHandleAcquireTime = fence->getSignalTime();
- mState.current.acquireFence = fence;
- mState.current.modified = true;
+ mCurrentState.acquireFence = fence;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setDataspace(ui::Dataspace dataspace) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.dataspace == dataspace) return false;
- mState.current.sequence++;
- mState.current.dataspace = dataspace;
- mState.current.modified = true;
+ if (mCurrentState.dataspace == dataspace) return false;
+ mCurrentState.sequence++;
+ mCurrentState.dataspace = dataspace;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setHdrMetadata(const HdrMetadata& hdrMetadata) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.hdrMetadata == hdrMetadata) return false;
- mState.current.sequence++;
- mState.current.hdrMetadata = hdrMetadata;
- mState.current.modified = true;
+ if (mCurrentState.hdrMetadata == hdrMetadata) return false;
+ mCurrentState.sequence++;
+ mCurrentState.hdrMetadata = hdrMetadata;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setSurfaceDamageRegion(const Region& surfaceDamage) {
- Mutex::Autolock lock(mStateMutex);
- mState.current.sequence++;
- mState.current.surfaceDamageRegion = surfaceDamage;
- mState.current.modified = true;
+ mCurrentState.sequence++;
+ mCurrentState.surfaceDamageRegion = surfaceDamage;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setApi(int32_t api) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.api == api) return false;
- mState.current.sequence++;
- mState.current.api = api;
- mState.current.modified = true;
+ if (mCurrentState.api == api) return false;
+ mCurrentState.sequence++;
+ mCurrentState.api = api;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
bool BufferStateLayer::setSidebandStream(const sp<NativeHandle>& sidebandStream) {
- Mutex::Autolock lock(mStateMutex);
- if (mState.current.sidebandStream == sidebandStream) return false;
- mState.current.sequence++;
- mState.current.sidebandStream = sidebandStream;
- mState.current.modified = true;
+ if (mCurrentState.sidebandStream == sidebandStream) return false;
+ mCurrentState.sequence++;
+ mCurrentState.sidebandStream = sidebandStream;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
if (!mSidebandStreamChanged.exchange(true)) {
@@ -272,10 +260,7 @@
mFlinger->getTransactionCompletedThread().registerPendingLatchedCallbackHandle(handle);
// Store so latched time and release fence can be set
- {
- Mutex::Autolock lock(mStateMutex);
- mState.current.callbackHandles.push_back(handle);
- }
+ mCurrentState.callbackHandles.push_back(handle);
} else { // If this layer will NOT need to be relatched and presented this frame
// Notify the transaction completed thread this handle is done
@@ -290,9 +275,8 @@
}
bool BufferStateLayer::setTransparentRegionHint(const Region& transparent) {
- Mutex::Autolock lock(mStateMutex);
- mState.current.transparentRegionHint = transparent;
- mState.current.modified = true;
+ mCurrentState.transparentRegionHint = transparent;
+ mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
return true;
}
@@ -328,7 +312,6 @@
return true;
}
- Mutex::Autolock lock(mStateMutex);
return getDrawingState().acquireFence->getStatus() == Fence::Status::Signaled;
}
@@ -337,7 +320,7 @@
return 0;
}
-std::shared_ptr<FenceTime> BufferStateLayer::getCurrentFenceTimeLocked() const {
+std::shared_ptr<FenceTime> BufferStateLayer::getCurrentFenceTime() const {
return std::make_shared<FenceTime>(getDrawingState().acquireFence);
}
@@ -384,17 +367,14 @@
}
Region BufferStateLayer::getDrawingSurfaceDamage() const {
- Mutex::Autolock lock(mStateMutex);
return getDrawingState().surfaceDamageRegion;
}
const HdrMetadata& BufferStateLayer::getDrawingHdrMetadata() const {
- Mutex::Autolock lock(mStateMutex);
return getDrawingState().hdrMetadata;
}
int BufferStateLayer::getDrawingApi() const {
- Mutex::Autolock lock(mStateMutex);
return getDrawingState().api;
}
@@ -419,7 +399,6 @@
}
std::optional<Region> BufferStateLayer::latchSidebandStream(bool& recomputeVisibleRegions) {
- Mutex::Autolock lock(mStateMutex);
if (mSidebandStreamChanged.exchange(false)) {
const State& s(getDrawingState());
// mSidebandStreamChanged was true
@@ -431,12 +410,12 @@
}
recomputeVisibleRegions = true;
- return getTransformLocked().transform(Region(Rect(s.active.w, s.active.h)));
+ return getTransform().transform(Region(Rect(s.active.w, s.active.h)));
}
return {};
}
-bool BufferStateLayer::hasFrameUpdateLocked() const {
+bool BufferStateLayer::hasFrameUpdate() const {
return mCurrentStateModified && getCurrentState().buffer != nullptr;
}
@@ -446,10 +425,6 @@
}
status_t BufferStateLayer::bindTextureImage() {
- Mutex::Autolock lock(mStateMutex);
- return bindTextureImageLocked();
-}
-status_t BufferStateLayer::bindTextureImageLocked() {
const State& s(getDrawingState());
auto& engine(mFlinger->getRenderEngine());
@@ -554,7 +529,7 @@
auto incomingStatus = releaseFence->getStatus();
if (incomingStatus == Fence::Status::Invalid) {
ALOGE("New fence has invalid state");
- mState.drawing.acquireFence = releaseFence;
+ mDrawingState.acquireFence = releaseFence;
mFlinger->mTimeStats->onDestroy(layerID);
return BAD_VALUE;
}
@@ -565,16 +540,16 @@
char fenceName[32] = {};
snprintf(fenceName, 32, "%.28s:%d", mName.string(), mFrameNumber);
sp<Fence> mergedFence =
- Fence::merge(fenceName, mState.drawing.acquireFence, releaseFence);
+ Fence::merge(fenceName, mDrawingState.acquireFence, releaseFence);
if (!mergedFence.get()) {
ALOGE("failed to merge release fences");
// synchronization is broken, the best we can do is hope fences
// signal in order so the new fence will act like a union
- mState.drawing.acquireFence = releaseFence;
+ mDrawingState.acquireFence = releaseFence;
mFlinger->mTimeStats->onDestroy(layerID);
return BAD_VALUE;
}
- mState.drawing.acquireFence = mergedFence;
+ mDrawingState.acquireFence = mergedFence;
} else if (incomingStatus == Fence::Status::Unsignaled) {
// If one fence has signaled and the other hasn't, the unsignaled
// fence will approximately correspond with the correct timestamp.
@@ -583,7 +558,7 @@
// by this point, they will have both signaled and only the timestamp
// will be slightly off; any dependencies after this point will
// already have been met.
- mState.drawing.acquireFence = releaseFence;
+ mDrawingState.acquireFence = releaseFence;
}
} else {
// Bind the new buffer to the GL texture.
@@ -592,7 +567,7 @@
// by glEGLImageTargetTexture2DOES, which this method calls. Newer
// devices will either call this in Layer::onDraw, or (if it's not
// a GL-composited layer) not at all.
- status_t err = bindTextureImageLocked();
+ status_t err = bindTextureImage();
if (err != NO_ERROR) {
mFlinger->mTimeStats->onDestroy(layerID);
return BAD_VALUE;
@@ -601,7 +576,7 @@
// TODO(marissaw): properly support mTimeStats
mFlinger->mTimeStats->setPostTime(layerID, getFrameNumber(), getName().c_str(), latchTime);
- mFlinger->mTimeStats->setAcquireFence(layerID, getFrameNumber(), getCurrentFenceTimeLocked());
+ mFlinger->mTimeStats->setAcquireFence(layerID, getFrameNumber(), getCurrentFenceTime());
mFlinger->mTimeStats->setLatchTime(layerID, getFrameNumber(), latchTime);
return NO_ERROR;
@@ -628,7 +603,6 @@
}
void BufferStateLayer::setHwcLayerBuffer(DisplayId displayId) {
- Mutex::Autolock lock(mStateMutex);
auto& hwcInfo = getBE().mHwcLayers[displayId];
auto& hwcLayer = hwcInfo.layer;