Call validateDisplay() when skipping frames
Layer management was getting screwed up in vr flinger in the following
scenario:
1. In frame X, post a new buffer to layer L.
2. Decide to skip frame X (e.g. because we're behind our target
schedule).
3. In frame X+1, delete layer L.
When we skip the frame in step 2, we weren't calling validateDisplay()
or presentDisplay() on the hardware composer, so the composer's internal
command queue wasn't being flushed. When we called validateDisplay() for
frame X+1 the update buffer call from frame X would be run, referencing
the deleted layer L, causing a crash.
Now we always call validateDisplay() when we change the layer state,
even if we decide to skip the frame.
I also added code to explicitly clear the Composer object's internal
command buffer when we transfer control from surface flinger to vr
flinger and back. There were certain cases where there could be commands
left in the command buffer after the display handoff.
Bug: 37159844
Test: I used an app switcher script that quickly switches vr apps, which
would consistently trigger the setLayerBuffer crash. I confirmed with
this CL applied I can run the app switcher until surface flinger runs
out of file descriptors (that's a separate bug), and I never see the
setLayerBuffer crash. I also confirmed from the log output there are no
additional hardware composer errors.
Change-Id: I85832b87f393754dc6b034eb38f2937d7b58ed74
diff --git a/libs/vr/libvrflinger/hardware_composer.cpp b/libs/vr/libvrflinger/hardware_composer.cpp
index 18ff4f5..bac9872 100644
--- a/libs/vr/libvrflinger/hardware_composer.cpp
+++ b/libs/vr/libvrflinger/hardware_composer.cpp
@@ -231,6 +231,8 @@
native_display_metrics_.width, native_display_metrics_.height, format,
usage);
+ hwc2_hidl_->resetCommands();
+
// Associate each Layer instance with a hardware composer layer.
for (auto layer : layers_) {
layer->Initialize(hwc2_hidl_.get(), &native_display_metrics_);
@@ -289,6 +291,8 @@
compositor_.Shutdown();
+ hwc2_hidl_->resetCommands();
+
// Trigger target-specific performance mode change.
property_set(kDvrPerformanceProperty, "idle");
}
@@ -427,6 +431,12 @@
layers_[i]->Prepare();
}
+ int32_t ret = Validate(HWC_DISPLAY_PRIMARY);
+ if (ret) {
+ ALOGE("HardwareComposer::Validate failed; ret=%d", ret);
+ return;
+ }
+
// Now that we have taken in a frame from the application, we have a chance
// to drop the frame before passing the frame along to HWC.
// If the display driver has become backed up, we detect it here and then
@@ -467,22 +477,6 @@
layers_[i]->GetCompositionType());
#endif
- int32_t ret = HWC2_ERROR_NONE;
-
- std::vector<Hwc2::IComposerClient::Rect> full_region(1);
- full_region[0].left = 0;
- full_region[0].top = 0;
- full_region[0].right = framebuffer_target_->width();
- full_region[0].bottom = framebuffer_target_->height();
-
- ALOGE_IF(ret, "Error setting client target : %d", ret);
-
- ret = Validate(HWC_DISPLAY_PRIMARY);
- if (ret) {
- ALOGE("HardwareComposer::Validate failed; ret=%d", ret);
- return;
- }
-
ret = Present(HWC_DISPLAY_PRIMARY);
if (ret) {
ALOGE("HardwareComposer::Present failed; ret=%d", ret);
@@ -912,6 +906,13 @@
frame_time_backlog_.clear();
DebugHudData::data.hwc_frame_stats.SkipFrame();
+ if (layer_config_changed) {
+ // If the layer config changed we need to validateDisplay() even if
+ // we're going to drop the frame, to flush the Composer object's
+ // internal command buffer and apply our layer changes.
+ Validate(HWC_DISPLAY_PRIMARY);
+ }
+
continue;
} else {
// Make the transition more obvious in systrace when the frame skip
@@ -923,8 +924,15 @@
error = SleepUntil(display_time_est - frame_time_estimate);
ALOGE_IF(error < 0, "HardwareComposer::PostThread: Failed to sleep: %s",
strerror(-error));
- if (error == kPostThreadInterrupted)
+ if (error == kPostThreadInterrupted) {
+ if (layer_config_changed) {
+ // If the layer config changed we need to validateDisplay() even if
+ // we're going to drop the frame, to flush the Composer object's
+ // internal command buffer and apply our layer changes.
+ Validate(HWC_DISPLAY_PRIMARY);
+ }
continue;
+ }
}
}
diff --git a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
index 262ab62..439adc4 100644
--- a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
+++ b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp
@@ -212,6 +212,10 @@
}
}
+void Composer::resetCommands() {
+ mWriter.reset();
+}
+
uint32_t Composer::getMaxVirtualDisplayCount()
{
auto ret = mClient->getMaxVirtualDisplayCount();
diff --git a/services/surfaceflinger/DisplayHardware/ComposerHal.h b/services/surfaceflinger/DisplayHardware/ComposerHal.h
index 37b7766..68d6e6f 100644
--- a/services/surfaceflinger/DisplayHardware/ComposerHal.h
+++ b/services/surfaceflinger/DisplayHardware/ComposerHal.h
@@ -137,6 +137,10 @@
void registerCallback(const sp<IComposerCallback>& callback);
+ // Reset all pending commands in the command buffer. Useful if you want to
+ // skip a frame but have already queued some commands.
+ void resetCommands();
+
uint32_t getMaxVirtualDisplayCount();
bool isUsingVrComposer() const { return mIsUsingVrComposer; }
Error createVirtualDisplay(uint32_t width, uint32_t height,