Fix locking issues with proto dumps
Proto dumps are generated from:
- binder threads when generating bugreports or triggering dumpstate
- main thread when mLayerStats is enabled
- tracing thread when winscope tracing is enabled.
The binder thread reads current state while the other threads reads drawing state.
The writeToProto function accesses a mix of current and drawing states. mPendingState should
only be accessed with the mStateLock held and the visible regions should be read from the main
or tracing threads. This causes some invalid access issues.
To make the locking requirements clear, this change
1. moves drawing specific data to a new function
2. copies mPendingState so we can dump the copy safely in main thread
3. dumps drawing data from binder threads by posting a message onto the main thread
Bug: 138318680
Test: adb shell dumpsys SurfaceFlinger, winscope
Change-Id: Ib2b49aedf06ee5a262d3162366ddf75d08432e05
Merged-In: I8bb93e9b9f81faec59585b770eb7ba0fbcd9b51b
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 1318bc0..f4284fe 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -153,7 +153,6 @@
mRemoteSyncPoints.clear();
{
- Mutex::Autolock pendingStateLock(mPendingStateMutex);
for (State pendingState : mPendingStates) {
pendingState.barrierLayer_legacy = nullptr;
}
@@ -907,6 +906,7 @@
// Commit the transaction
commitTransaction(c);
+ mPendingStatesSnapshot = mPendingStates;
mCurrentState.callbackHandles = {};
return flags;
}
@@ -1874,14 +1874,61 @@
setTransactionFlags(eTransactionNeeded);
}
-void Layer::writeToProto(LayerProto* layerInfo, LayerVector::StateSet stateSet,
- uint32_t traceFlags) {
+void Layer::writeToProtoDrawingState(LayerProto* layerInfo, uint32_t traceFlags) const {
+ ui::Transform transform = getTransform();
+
+ if (traceFlags & SurfaceTracing::TRACE_CRITICAL) {
+ for (const auto& pendingState : mPendingStatesSnapshot) {
+ auto barrierLayer = pendingState.barrierLayer_legacy.promote();
+ if (barrierLayer != nullptr) {
+ BarrierLayerProto* barrierLayerProto = layerInfo->add_barrier_layer();
+ barrierLayerProto->set_id(barrierLayer->sequence);
+ barrierLayerProto->set_frame_number(pendingState.frameNumber_legacy);
+ }
+ }
+
+ auto buffer = mActiveBuffer;
+ if (buffer != nullptr) {
+ LayerProtoHelper::writeToProto(buffer,
+ [&]() { return layerInfo->mutable_active_buffer(); });
+ LayerProtoHelper::writeToProto(ui::Transform(mCurrentTransform),
+ layerInfo->mutable_buffer_transform());
+ }
+ layerInfo->set_invalidate(contentDirty);
+ layerInfo->set_is_protected(isProtected());
+ layerInfo->set_dataspace(
+ dataspaceDetails(static_cast<android_dataspace>(mCurrentDataSpace)));
+ layerInfo->set_queued_frames(getQueuedFrameCount());
+ layerInfo->set_refresh_pending(isBufferLatched());
+ layerInfo->set_curr_frame(mCurrentFrameNumber);
+ layerInfo->set_effective_scaling_mode(getEffectiveScalingMode());
+
+ layerInfo->set_corner_radius(getRoundedCornerState().radius);
+ LayerProtoHelper::writeToProto(transform, layerInfo->mutable_transform());
+ LayerProtoHelper::writePositionToProto(transform.tx(), transform.ty(),
+ [&]() { return layerInfo->mutable_position(); });
+ LayerProtoHelper::writeToProto(mBounds, [&]() { return layerInfo->mutable_bounds(); });
+ LayerProtoHelper::writeToProto(visibleRegion,
+ [&]() { return layerInfo->mutable_visible_region(); });
+ LayerProtoHelper::writeToProto(surfaceDamageRegion,
+ [&]() { return layerInfo->mutable_damage_region(); });
+ }
+
+ if (traceFlags & SurfaceTracing::TRACE_EXTRA) {
+ LayerProtoHelper::writeToProto(mSourceBounds,
+ [&]() { return layerInfo->mutable_source_bounds(); });
+ LayerProtoHelper::writeToProto(mScreenBounds,
+ [&]() { return layerInfo->mutable_screen_bounds(); });
+ }
+}
+
+void Layer::writeToProtoCommonState(LayerProto* layerInfo, LayerVector::StateSet stateSet,
+ uint32_t traceFlags) const {
const bool useDrawing = stateSet == LayerVector::StateSet::Drawing;
const LayerVector& children = useDrawing ? mDrawingChildren : mCurrentChildren;
const State& state = useDrawing ? mDrawingState : mCurrentState;
ui::Transform requestedTransform = state.active_legacy.transform;
- ui::Transform transform = getTransform();
if (traceFlags & SurfaceTracing::TRACE_CRITICAL) {
layerInfo->set_id(sequence);
@@ -1901,17 +1948,10 @@
LayerProtoHelper::writeToProto(state.activeTransparentRegion_legacy,
[&]() { return layerInfo->mutable_transparent_region(); });
- LayerProtoHelper::writeToProto(visibleRegion,
- [&]() { return layerInfo->mutable_visible_region(); });
- LayerProtoHelper::writeToProto(surfaceDamageRegion,
- [&]() { return layerInfo->mutable_damage_region(); });
layerInfo->set_layer_stack(getLayerStack());
layerInfo->set_z(state.z);
- LayerProtoHelper::writePositionToProto(transform.tx(), transform.ty(),
- [&]() { return layerInfo->mutable_position(); });
-
LayerProtoHelper::writePositionToProto(requestedTransform.tx(), requestedTransform.ty(),
[&]() {
return layerInfo->mutable_requested_position();
@@ -1922,15 +1962,9 @@
LayerProtoHelper::writeToProto(state.crop_legacy,
[&]() { return layerInfo->mutable_crop(); });
- layerInfo->set_corner_radius(getRoundedCornerState().radius);
layerInfo->set_is_opaque(isOpaque(state));
- layerInfo->set_invalidate(contentDirty);
- layerInfo->set_is_protected(isProtected());
- // XXX (b/79210409) mCurrentDataSpace is not protected
- layerInfo->set_dataspace(
- dataspaceDetails(static_cast<android_dataspace>(mCurrentDataSpace)));
layerInfo->set_pixel_format(decodePixelFormat(getPixelFormat()));
LayerProtoHelper::writeToProto(getColor(), [&]() { return layerInfo->mutable_color(); });
@@ -1938,7 +1972,6 @@
[&]() { return layerInfo->mutable_requested_color(); });
layerInfo->set_flags(state.flags);
- LayerProtoHelper::writeToProto(transform, layerInfo->mutable_transform());
LayerProtoHelper::writeToProto(requestedTransform,
layerInfo->mutable_requested_transform());
@@ -1955,29 +1988,6 @@
} else {
layerInfo->set_z_order_relative_of(-1);
}
-
- auto buffer = mActiveBuffer;
- if (buffer != nullptr) {
- LayerProtoHelper::writeToProto(buffer,
- [&]() { return layerInfo->mutable_active_buffer(); });
- LayerProtoHelper::writeToProto(ui::Transform(mCurrentTransform),
- layerInfo->mutable_buffer_transform());
- }
-
- layerInfo->set_queued_frames(getQueuedFrameCount());
- layerInfo->set_refresh_pending(isBufferLatched());
- layerInfo->set_curr_frame(mCurrentFrameNumber);
- layerInfo->set_effective_scaling_mode(getEffectiveScalingMode());
-
- for (const auto& pendingState : mPendingStates) {
- auto barrierLayer = pendingState.barrierLayer_legacy.promote();
- if (barrierLayer != nullptr) {
- BarrierLayerProto* barrierLayerProto = layerInfo->add_barrier_layer();
- barrierLayerProto->set_id(barrierLayer->sequence);
- barrierLayerProto->set_frame_number(pendingState.frameNumber_legacy);
- }
- }
- LayerProtoHelper::writeToProto(mBounds, [&]() { return layerInfo->mutable_bounds(); });
}
if (traceFlags & SurfaceTracing::TRACE_INPUT) {
@@ -1990,23 +2000,19 @@
for (const auto& entry : state.metadata.mMap) {
(*protoMap)[entry.first] = std::string(entry.second.cbegin(), entry.second.cend());
}
- LayerProtoHelper::writeToProto(mEffectiveTransform,
- layerInfo->mutable_effective_transform());
- LayerProtoHelper::writeToProto(mSourceBounds,
- [&]() { return layerInfo->mutable_source_bounds(); });
- LayerProtoHelper::writeToProto(mScreenBounds,
- [&]() { return layerInfo->mutable_screen_bounds(); });
}
}
-void Layer::writeToProto(LayerProto* layerInfo, const sp<DisplayDevice>& displayDevice,
- uint32_t traceFlags) {
+void Layer::writeToProtoCompositionState(LayerProto* layerInfo,
+ const sp<DisplayDevice>& displayDevice,
+ uint32_t traceFlags) const {
auto outputLayer = findOutputLayerForDisplay(displayDevice);
if (!outputLayer) {
return;
}
- writeToProto(layerInfo, LayerVector::StateSet::Drawing, traceFlags);
+ writeToProtoDrawingState(layerInfo, traceFlags);
+ writeToProtoCommonState(layerInfo, LayerVector::StateSet::Drawing, traceFlags);
const auto& compositionState = outputLayer->getState();
@@ -2024,13 +2030,6 @@
static_cast<int32_t>(compositionState.hwc ? (*compositionState.hwc).hwcCompositionType
: Hwc2::IComposerClient::Composition::CLIENT);
layerInfo->set_hwc_composition_type(compositionType);
-
- if (std::strcmp(getTypeId(), "BufferLayer") == 0 &&
- static_cast<BufferLayer*>(this)->isProtected()) {
- layerInfo->set_is_protected(true);
- } else {
- layerInfo->set_is_protected(false);
- }
}
bool Layer::isRemovedFromCurrentState() const {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 8a80e15..132b4cf 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -438,11 +438,21 @@
bool isRemovedFromCurrentState() const;
- void writeToProto(LayerProto* layerInfo, LayerVector::StateSet stateSet,
- uint32_t traceFlags = SurfaceTracing::TRACE_ALL);
-
- void writeToProto(LayerProto* layerInfo, const sp<DisplayDevice>& displayDevice,
- uint32_t traceFlags = SurfaceTracing::TRACE_ALL);
+ // Write states that are modified by the main thread. This includes drawing
+ // state as well as buffer data. This should be called in the main or tracing
+ // thread.
+ void writeToProtoDrawingState(LayerProto* layerInfo,
+ uint32_t traceFlags = SurfaceTracing::TRACE_ALL) const;
+ // Write states that are modified by the main thread. This includes drawing
+ // state as well as buffer data and composition data for layers on the specified
+ // display. This should be called in the main or tracing thread.
+ void writeToProtoCompositionState(LayerProto* layerInfo, const sp<DisplayDevice>& displayDevice,
+ uint32_t traceFlags = SurfaceTracing::TRACE_ALL) const;
+ // Write drawing or current state. If writing current state, the caller should hold the
+ // external mStateLock. If writing drawing state, this function should be called on the
+ // main or tracing thread.
+ void writeToProtoCommonState(LayerProto* layerInfo, LayerVector::StateSet stateSet,
+ uint32_t traceFlags = SurfaceTracing::TRACE_ALL) const;
virtual Geometry getActiveGeometry(const Layer::State& s) const { return s.active_legacy; }
virtual uint32_t getActiveWidth(const Layer::State& s) const { return s.active_legacy.w; }
@@ -832,13 +842,15 @@
bool mPrimaryDisplayOnly = false;
- // these are protected by an external lock
- State mCurrentState;
+ // These are only accessed by the main thread or the tracing thread.
State mDrawingState;
- std::atomic<uint32_t> mTransactionFlags{0};
+ // Store a copy of the pending state so that the drawing thread can access the
+ // states without a lock.
+ Vector<State> mPendingStatesSnapshot;
- // Accessed from main thread and binder threads
- Mutex mPendingStateMutex;
+ // these are protected by an external lock (mStateLock)
+ State mCurrentState;
+ std::atomic<uint32_t> mTransactionFlags{0};
Vector<State> mPendingStates;
// Timestamp history for UIAutomation. Thread safe.
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index ad0ac6b..ca33bb8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4559,18 +4559,22 @@
if (const auto it = dumpers.find(flag); it != dumpers.end()) {
(it->second)(args, asProto, result);
- } else {
- if (asProto) {
- LayersProto layersProto = dumpProtoInfo(LayerVector::StateSet::Current);
- result.append(layersProto.SerializeAsString().c_str(), layersProto.ByteSize());
- } else {
- dumpAllLocked(args, result);
- }
+ } else if (!asProto) {
+ dumpAllLocked(args, result);
}
if (locked) {
mStateLock.unlock();
}
+
+ LayersProto layersProto = dumpProtoFromMainThread();
+ if (asProto) {
+ result.append(layersProto.SerializeAsString().c_str(), layersProto.ByteSize());
+ } else {
+ auto layerTree = LayerProtoParser::generateLayerTree(layersProto);
+ result.append(LayerProtoParser::layerTreeToString(layerTree));
+ result.append("\n");
+ }
}
write(fd, result.c_str(), result.size());
return NO_ERROR;
@@ -4803,19 +4807,23 @@
result.append("\n");
}
-LayersProto SurfaceFlinger::dumpProtoInfo(LayerVector::StateSet stateSet,
- uint32_t traceFlags) const {
+LayersProto SurfaceFlinger::dumpDrawingStateProto(uint32_t traceFlags) const {
LayersProto layersProto;
- const bool useDrawing = stateSet == LayerVector::StateSet::Drawing;
- const State& state = useDrawing ? mDrawingState : mCurrentState;
- state.traverseInZOrder([&](Layer* layer) {
+ mDrawingState.traverseInZOrder([&](Layer* layer) {
LayerProto* layerProto = layersProto.add_layers();
- layer->writeToProto(layerProto, stateSet, traceFlags);
+ layer->writeToProtoDrawingState(layerProto, traceFlags);
+ layer->writeToProtoCommonState(layerProto, LayerVector::StateSet::Drawing, traceFlags);
});
return layersProto;
}
+LayersProto SurfaceFlinger::dumpProtoFromMainThread(uint32_t traceFlags) {
+ LayersProto layersProto;
+ postMessageSync(new LambdaMessage([&]() { layersProto = dumpDrawingStateProto(traceFlags); }));
+ return layersProto;
+}
+
LayersProto SurfaceFlinger::dumpVisibleLayersProtoInfo(
const sp<DisplayDevice>& displayDevice) const {
LayersProto layersProto;
@@ -4836,7 +4844,7 @@
mDrawingState.traverseInZOrder([&](Layer* layer) {
if (!layer->visibleRegion.isEmpty() && !display->getOutputLayersOrderedByZ().empty()) {
LayerProto* layerProto = layersProto.add_layers();
- layer->writeToProto(layerProto, displayDevice);
+ layer->writeToProtoCompositionState(layerProto, displayDevice);
}
});
@@ -4901,13 +4909,6 @@
colorizer.reset(result);
{
- LayersProto layersProto = dumpProtoInfo(LayerVector::StateSet::Current);
- auto layerTree = LayerProtoParser::generateLayerTree(layersProto);
- result.append(LayerProtoParser::layerTreeToString(layerTree));
- result.append("\n");
- }
-
- {
StringAppendF(&result, "Composition layers\n");
mDrawingState.traverseInZOrder([&](Layer* layer) {
auto compositionLayer = layer->getCompositionLayer();
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 63e74f5..aa3095e 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -263,7 +263,8 @@
status_t postMessageAsync(const sp<MessageBase>& msg, nsecs_t reltime = 0, uint32_t flags = 0);
// post a synchronous message to the main thread
- status_t postMessageSync(const sp<MessageBase>& msg, nsecs_t reltime = 0, uint32_t flags = 0);
+ status_t postMessageSync(const sp<MessageBase>& msg, nsecs_t reltime = 0, uint32_t flags = 0)
+ EXCLUDES(mStateLock);
// force full composition on all displays
void repaintEverything();
@@ -883,8 +884,9 @@
void dumpBufferingStats(std::string& result) const;
void dumpDisplayIdentificationData(std::string& result) const;
void dumpWideColorInfo(std::string& result) const;
- LayersProto dumpProtoInfo(LayerVector::StateSet stateSet,
- uint32_t traceFlags = SurfaceTracing::TRACE_ALL) const;
+ LayersProto dumpDrawingStateProto(uint32_t traceFlags = SurfaceTracing::TRACE_ALL) const;
+ LayersProto dumpProtoFromMainThread(uint32_t traceFlags = SurfaceTracing::TRACE_ALL)
+ EXCLUDES(mStateLock);
void withTracingLock(std::function<void()> operation) REQUIRES(mStateLock);
LayersProto dumpVisibleLayersProtoInfo(const sp<DisplayDevice>& display) const;
diff --git a/services/surfaceflinger/SurfaceTracing.cpp b/services/surfaceflinger/SurfaceTracing.cpp
index c4ab066..9053f2c 100644
--- a/services/surfaceflinger/SurfaceTracing.cpp
+++ b/services/surfaceflinger/SurfaceTracing.cpp
@@ -162,7 +162,7 @@
LayersTraceProto entry;
entry.set_elapsed_realtime_nanos(elapsedRealtimeNano());
entry.set_where(where);
- LayersProto layers(mFlinger.dumpProtoInfo(LayerVector::StateSet::Drawing, mTraceFlags));
+ LayersProto layers(mFlinger.dumpDrawingStateProto(mTraceFlags));
entry.mutable_layers()->Swap(&layers);
return entry;