drm_hwcomposer: Rework HwcDisplay disposal to avoid races

The code prior to this commit has a flaw:

    HwcDisplay::~HwcDisplay() {
      ...
      auto &main_lock = hwc2_->GetResMan().GetMainLock();
      /* Unlock to allow pending vsync callbacks to finish */
      main_lock.unlock();

At this point display is no longer in displays_[] list. After
lock is released, hwc2 API thread starts to process transactions
which may fail with BAD_SIAPLAY responce and cause SF to crash.

      vsync_worker_.VSyncControl(false);
      vsync_worker_.Exit();
      main_lock.lock();
    }

1. Rework the logic in order to avoid such scenariuos:
1.a. Temporary switch non-primary unplugged displays to headless state
     allowing remaining transactions to succeed without impacting the
     pipeline.
1.b. Give 100mSec delay before destroying / removing display from the
     displays_[] list to allow all pending hwc2 transactions to complete.

2. Support hotswap of the DrmDisplayPipeline, which makes primary display
   reattaching process smoother.
   Now SF should be able to gracefully remove all layers.

Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>

4 files changed, 75 insertions, 68 deletions

diff --git a/hwc2_device/DrmHwcTwo.cpp b/hwc2_device/DrmHwcTwo.cpp
index f1a5490..e689419 100644
--- a/hwc2_device/DrmHwcTwo.cpp
+++ b/hwc2_device/DrmHwcTwo.cpp
@@ -30,26 +30,22 @@ DrmHwcTwo::DrmHwcTwo() : resource_manager_(this){};
 /* Must be called after every display attach/detach cycle */
 void DrmHwcTwo::FinalizeDisplayBinding() {
   if (displays_.count(kPrimaryDisplay) == 0) {
-    /* Create/update new headless display if no other displays exists
-     * or reattach different display to make it primary
-     */
+    /* Primary display MUST always exist */
+    ALOGI("No pipelines available. Creating null-display for headless mode");
+    displays_[kPrimaryDisplay] = std::make_unique<
+        HwcDisplay>(kPrimaryDisplay, HWC2::DisplayType::Physical, this);
+    /* Initializes null-display */
+    displays_[kPrimaryDisplay]->SetPipeline(nullptr);
+  }
 
-    if (display_handles_.empty()) {
-      /* Enable headless mode */
-      ALOGI("No pipelines available. Creating null-display for headless mode");
-      displays_[kPrimaryDisplay] = std::make_unique<
-          HwcDisplay>(nullptr, kPrimaryDisplay, HWC2::DisplayType::Physical,
-                      this);
-    } else {
-      auto *pipe = display_handles_.begin()->first;
-      ALOGI("Primary display was disconnected, reattaching '%s' as new primary",
-            pipe->connector->Get()->GetName().c_str());
-      UnbindDisplay(pipe);
-      BindDisplay(pipe);
-      if (displays_.count(kPrimaryDisplay) == 0) {
-        ALOGE("FIXME!!! Still no primary display after reattaching...");
-      }
-    }
+  if (displays_[kPrimaryDisplay]->IsInHeadlessMode() &&
+      !display_handles_.empty()) {
+    /* Reattach first secondary display to take place of the primary */
+    auto *pipe = display_handles_.begin()->first;
+    ALOGI("Primary display was disconnected, reattaching '%s' as new primary",
+          pipe->connector->Get()->GetName().c_str());
+    UnbindDisplay(pipe);
+    BindDisplay(pipe);
   }
 
   // Finally, send hotplug events to the client
@@ -57,6 +53,24 @@ void DrmHwcTwo::FinalizeDisplayBinding() {
     SendHotplugEventToClient(dhe.first, dhe.second);
   }
   deferred_hotplug_events_.clear();
+
+  /* Wait 0.2s before removing the displays to flush pending HWC2 transactions
+   */
+  auto &mutex = GetResMan().GetMainLock();
+  mutex.unlock();
+  const int kTimeForSFToDisposeDisplayUs = 200000;
+  usleep(kTimeForSFToDisposeDisplayUs);
+  mutex.lock();
+  std::vector<std::unique_ptr<HwcDisplay>> for_disposal;
+  for (auto handle : displays_for_removal_list_) {
+    for_disposal.emplace_back(
+        std::unique_ptr<HwcDisplay>(displays_[handle].release()));
+    displays_.erase(handle);
+  }
+  /* Destroy HwcDisplays while unlocked to avoid vsyncworker deadlocks */
+  mutex.unlock();
+  for_disposal.clear();
+  mutex.lock();
 }
 
 bool DrmHwcTwo::BindDisplay(DrmDisplayPipeline *pipeline) {
@@ -73,18 +87,17 @@ bool DrmHwcTwo::BindDisplay(DrmDisplayPipeline *pipeline) {
     disp_handle = ++last_display_handle_;
   }
 
-  auto disp = std::make_unique<HwcDisplay>(pipeline, disp_handle,
-                                           HWC2::DisplayType::Physical, this);
-
-  if (disp_handle == kPrimaryDisplay) {
-    displays_.erase(disp_handle);
+  if (displays_.count(disp_handle) == 0) {
+    auto disp = std::make_unique<HwcDisplay>(disp_handle,
+                                             HWC2::DisplayType::Physical, this);
+    displays_[disp_handle] = std::move(disp);
   }
 
   ALOGI("Attaching pipeline '%s' to the display #%d%s",
         pipeline->connector->Get()->GetName().c_str(), (int)disp_handle,
         disp_handle == kPrimaryDisplay ? " (Primary)" : "");
 
-  displays_[disp_handle] = std::move(disp);
+  displays_[disp_handle]->SetPipeline(pipeline);
   display_handles_[pipeline] = disp_handle;
 
   return true;
@@ -96,17 +109,25 @@ bool DrmHwcTwo::UnbindDisplay(DrmDisplayPipeline *pipeline) {
     return false;
   }
   auto handle = display_handles_[pipeline];
+  display_handles_.erase(pipeline);
 
   ALOGI("Detaching the pipeline '%s' from the display #%i%s",
         pipeline->connector->Get()->GetName().c_str(), (int)handle,
         handle == kPrimaryDisplay ? " (Primary)" : "");
 
-  display_handles_.erase(pipeline);
   if (displays_.count(handle) == 0) {
     ALOGE("%s, can't find the display, handle: %" PRIu64, __func__, handle);
     return false;
   }
-  displays_.erase(handle);
+  displays_[handle]->SetPipeline(nullptr);
+
+  /* We must defer display disposal and removal, since it may still have pending
+   * HWC_API calls scheduled and waiting until ueventlistener thread releases
+   * main lock, otherwise transaction may fail and SF may crash
+   */
+  if (handle != kPrimaryDisplay) {
+    displays_for_removal_list_.emplace_back(handle);
+  }
   return true;
 }
 
diff --git a/hwc2_device/DrmHwcTwo.h b/hwc2_device/DrmHwcTwo.h
index 0e72251..2b8a74f 100644
--- a/hwc2_device/DrmHwcTwo.h
+++ b/hwc2_device/DrmHwcTwo.h
@@ -81,6 +81,7 @@ class DrmHwcTwo : public PipelineToFrontendBindingInterface {
   std::string mDumpString;
 
   std::map<hwc2_display_t, bool> deferred_hotplug_events_;
+  std::vector<hwc2_display_t> displays_for_removal_list_;
 
   uint32_t last_display_handle_ = kPrimaryDisplay;
 };
diff --git a/hwc2_device/HwcDisplay.cpp b/hwc2_device/HwcDisplay.cpp
index 3f00fbf..cedac19 100644
--- a/hwc2_device/HwcDisplay.cpp
+++ b/hwc2_device/HwcDisplay.cpp
@@ -20,6 +20,7 @@
 #include "HwcDisplay.h"
 
 #include "DrmHwcTwo.h"
+#include "backend/Backend.h"
 #include "backend/BackendManager.h"
 #include "bufferinfo/BufferInfoGetter.h"
 #include "utils/log.h"
@@ -27,9 +28,6 @@
 
 namespace android {
 
-// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
-uint32_t HwcDisplay::layer_idx_ = 2; /* Start from 2. See destroyLayer() */
-
 std::string HwcDisplay::DumpDelta(HwcDisplay::Stats delta) {
   if (delta.total_pixops_ == 0)
     return "No stats yet";
@@ -87,10 +85,9 @@ std::string HwcDisplay::Dump() {
   return ss.str();
 }
 
-HwcDisplay::HwcDisplay(DrmDisplayPipeline *pipeline, hwc2_display_t handle,
-                       HWC2::DisplayType type, DrmHwcTwo *hwc2)
+HwcDisplay::HwcDisplay(hwc2_display_t handle, HWC2::DisplayType type,
+                       DrmHwcTwo *hwc2)
     : hwc2_(hwc2),
-      pipeline_(pipeline),
       handle_(handle),
       type_(type),
       color_transform_hint_(HAL_COLOR_TRANSFORM_IDENTITY) {
@@ -100,24 +97,26 @@ HwcDisplay::HwcDisplay(DrmDisplayPipeline *pipeline, hwc2_display_t handle,
                              0.0, 0.0, 1.0, 0.0,
                              0.0, 0.0, 0.0, 1.0};
   // clang-format on
+}
 
-  ChosePreferredConfig();
-  Init();
+HwcDisplay::~HwcDisplay() = default;
 
-  hwc2_->ScheduleHotplugEvent(handle_, /*connected = */ true);
-}
+void HwcDisplay::SetPipeline(DrmDisplayPipeline *pipeline) {
+  pipeline_ = pipeline;
 
-HwcDisplay::~HwcDisplay() {
-  if (handle_ != kPrimaryDisplay) {
-    hwc2_->ScheduleHotplugEvent(handle_, /*connected = */ false);
-  }
+  if (pipeline != nullptr) {
+    ChosePreferredConfig();
+    Init();
 
-  auto &main_lock = hwc2_->GetResMan().GetMainLock();
-  /* Unlock to allow pending vsync callbacks to finish */
-  main_lock.unlock();
-  vsync_worker_.VSyncControl(false);
-  vsync_worker_.Exit();
-  main_lock.lock();
+    hwc2_->ScheduleHotplugEvent(handle_, /*connected = */ true);
+  } else {
+    backend_.reset();
+    vsync_worker_.Init(nullptr, [](int64_t) {});
+    SetClientTarget(nullptr, -1, 0, {});
+    if (handle_ != kPrimaryDisplay) {
+      hwc2_->ScheduleHotplugEvent(handle_, /*connected = */ false);
+    }
+  }
 }
 
 HWC2::Error HwcDisplay::Init() {
@@ -138,7 +137,7 @@ HWC2::Error HwcDisplay::Init() {
       vsync_worker_.VSyncControl(false);
     }
   });
-  if (ret) {
+  if (ret && ret != -EALREADY) {
     ALOGE("Failed to create event worker for d=%d %d\n", int(handle_), ret);
     return HWC2::Error::BadDisplay;
   }
@@ -185,21 +184,6 @@ HWC2::Error HwcDisplay::CreateLayer(hwc2_layer_t *layer) {
 
 HWC2::Error HwcDisplay::DestroyLayer(hwc2_layer_t layer) {
   if (!get_layer(layer)) {
-    /* Primary display don't send unplug event, instead it replaces
-     * display to headless or to another one and sends Plug event to the
-     * SF. SF can't distinguish this case from virtualized display size
-     * change case and will destroy previously used layers. If we will return
-     * BadLayer, service will print errors to the logcat.
-     *
-     * Nevertheless VTS is trying to destroy 1st layer without adding any
-     * layers prior to that, than it checks for BadLayer result. So we
-     * numbering the layers starting from 2, and use index 1 to catch VTS client
-     * to return BadLayer, making VTS pass.
-     */
-    if (layers_.empty() && layer != 1) {
-      return HWC2::Error::None;
-    }
-
     return HWC2::Error::BadLayer;
   }
 
diff --git a/hwc2_device/HwcDisplay.h b/hwc2_device/HwcDisplay.h
index ae5d980..98d8e9b 100644
--- a/hwc2_device/HwcDisplay.h
+++ b/hwc2_device/HwcDisplay.h
@@ -37,11 +37,13 @@ inline constexpr uint32_t kPrimaryDisplay = 0;
 
 class HwcDisplay {
  public:
-  HwcDisplay(DrmDisplayPipeline *pipeline, hwc2_display_t handle,
-             HWC2::DisplayType type, DrmHwcTwo *hwc2);
+  HwcDisplay(hwc2_display_t handle, HWC2::DisplayType type, DrmHwcTwo *hwc2);
   HwcDisplay(const HwcDisplay &) = delete;
   ~HwcDisplay();
 
+  /* SetPipeline should be carefully used only by DrmHwcTwo hotplug handlers */
+  void SetPipeline(DrmDisplayPipeline *pipeline);
+
   HWC2::Error CreateComposition(AtomicCommitArgs &a_args);
   std::vector<HwcLayer *> GetOrderLayersByZPos();
 
@@ -199,7 +201,7 @@ class HwcDisplay {
   int64_t staged_mode_change_time_{};
   uint32_t staged_mode_config_id_{};
 
-  DrmDisplayPipeline *const pipeline_;
+  DrmDisplayPipeline *pipeline_{};
 
   std::unique_ptr<Backend> backend_;
 
@@ -212,8 +214,7 @@ class HwcDisplay {
   const hwc2_display_t handle_;
   HWC2::DisplayType type_;
 
-  // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
-  static uint32_t layer_idx_;
+  uint32_t layer_idx_{};
 
   std::map<hwc2_layer_t, HwcLayer> layers_;
   HwcLayer client_layer_;