ozone: drm: Hold onto modeset framebuffer until first page flip

Since bb1059e5 ("ozone: drm: Clean up error handling around page flips")
we only keep one set of framebuffers alive for a group of crtcs that are
mirroring (i.e., they should always be showing the same framebuffer).

It turns out it is not true that mirrors always show the same buffer;
when we modeset a crtc on entering mirror mode, we may allocate a new
framebuffer for that purpose and only set that framebuffer on one of the
crtcs. Until the first page flip, the two crtcs will show different
images.

To avoid a crash when we try to page flip, go back to keeping a modeset
buffer for each CRTC. This crash only happens if we have to allocate a
new buffer for modesettings; in some circumstances, we're able to reuse a
buffer and there's no crash.

Bug: 888553, b/117854747, b/117863342, b/117631111
Test: ozone_unittests; boot grunt with two hardware mirrored displays

Change-Id: Ib903e00a8975aac16e40ed685b809bbba8bb9124
Reviewed-on: https://chromium-review.googlesource.com/c/1302110
Commit-Queue: Michael Spang <spang@chromium.org>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603224}

ui/ozone/platform/drm/gpu/crtc_controller.cc

@@ -53,6 +53,12 @@ bool CrtcController::Modeset(const DrmOverlayPlane& plane,
   mode_ = mode;
   is_disabled_ = false;
 
+  // Hold modeset buffer until page flip. This fixes a crash on entering
+  // hardware mirror mode in some circumstances (bug 888553).
+  // TODO(spang): Fix this better by changing how mirrors are set up (bug
+  // 899352).
+  modeset_framebuffer_ = plane.buffer;
+
   return true;
 }
 
@@ -107,6 +113,10 @@ void CrtcController::MoveCursor(const gfx::Point& location) {
   drm_->MoveCursor(crtc_, location);
 }
 
+void CrtcController::OnPageFlipComplete() {
+  modeset_framebuffer_ = nullptr;
+}
+
 void CrtcController::DisableCursor() {
   if (!drm_->SetCursor(crtc_, 0, gfx::Size())) {
     PLOG(ERROR) << "drmModeSetCursor: device " << drm_->device_path().value()

ui/ozone/platform/drm/gpu/crtc_controller.h

@@ -63,6 +63,8 @@ class CrtcController {
   void SetCursor(uint32_t handle, const gfx::Size& size);
   void MoveCursor(const gfx::Point& location);
 
+  void OnPageFlipComplete();
+
  private:
   void DisableCursor();
 
@@ -75,6 +77,8 @@ class CrtcController {
 
   drmModeModeInfo mode_ = {};
 
+  scoped_refptr<DrmFramebuffer> modeset_framebuffer_;
+
   // Keeps track of the CRTC state. If a surface has been bound, then the value
   // is set to false. Otherwise it is true.
   bool is_disabled_ = true;

ui/ozone/platform/drm/gpu/hardware_display_controller.cc

@@ -338,6 +338,8 @@ void HardwareDisplayController::OnPageFlipComplete(
     return;  // Modeset occured during this page flip.
   time_of_last_flip_ = presentation_feedback.timestamp;
   current_planes_ = std::move(pending_planes);
+  for (const auto& controller : crtc_controllers_)
+    controller->OnPageFlipComplete();
   page_flip_request_ = nullptr;
 }
 

ui/ozone/platform/drm/gpu/mock_drm_device.cc

@@ -197,6 +197,7 @@ bool MockDrmDevice::SetCrtc(uint32_t crtc_id,
                             uint32_t framebuffer,
                             std::vector<uint32_t> connectors,
                             drmModeModeInfo* mode) {
+  crtc_fb_[crtc_id] = framebuffer;
   current_framebuffer_ = framebuffer;
   set_crtc_call_count_++;
   return set_crtc_expectation_;
@@ -228,11 +229,24 @@ bool MockDrmDevice::AddFramebuffer2(uint32_t width,
                                     uint32_t flags) {
   add_framebuffer_call_count_++;
   *framebuffer = add_framebuffer_call_count_;
+  framebuffer_ids_.insert(*framebuffer);
   return add_framebuffer_expectation_;
 }
 
 bool MockDrmDevice::RemoveFramebuffer(uint32_t framebuffer) {
+  {
+    auto it = framebuffer_ids_.find(framebuffer);
+    CHECK(it != framebuffer_ids_.end());
+    framebuffer_ids_.erase(it);
+  }
   remove_framebuffer_call_count_++;
+  std::vector<uint32_t> crtcs_to_clear;
+  for (auto crtc_fb : crtc_fb_) {
+    if (crtc_fb.second == framebuffer)
+      crtcs_to_clear.push_back(crtc_fb.first);
+  }
+  for (auto crtc : crtcs_to_clear)
+    crtc_fb_[crtc] = 0;
   return true;
 }
 
@@ -245,6 +259,7 @@ bool MockDrmDevice::PageFlip(uint32_t crtc_id,
                              scoped_refptr<PageFlipRequest> page_flip_request) {
   page_flip_call_count_++;
   DCHECK(page_flip_request);
+  crtc_fb_[crtc_id] = framebuffer;
   current_framebuffer_ = framebuffer;
   if (page_flip_expectation_)
     callbacks_.push(page_flip_request->AddPageFlip());
@@ -422,6 +437,11 @@ bool MockDrmDevice::SetCapability(uint64_t capability, uint64_t value) {
   return true;
 }
 
+uint32_t MockDrmDevice::GetFramebufferForCrtc(uint32_t crtc_id) const {
+  auto it = crtc_fb_.find(crtc_id);
+  return it != crtc_fb_.end() ? it->second : 0u;
+}
+
 void MockDrmDevice::RunCallbacks() {
   while (!callbacks_.empty()) {
     PageFlipCallback callback = std::move(callbacks_.front());

ui/ozone/platform/drm/gpu/mock_drm_device.h

@@ -166,6 +166,7 @@ class MockDrmDevice : public DrmDevice {
       uint32_t crtc_id,
       const std::vector<display::GammaRampRGBEntry>& lut) override;
   bool SetCapability(uint64_t capability, uint64_t value) override;
+  uint32_t GetFramebufferForCrtc(uint32_t crtc_id) const;
 
  private:
   ~MockDrmDevice() override;
@@ -205,6 +206,9 @@ class MockDrmDevice : public DrmDevice {
 
   std::map<uint32_t, ScopedDrmPropertyBlobPtr> blob_property_map_;
 
+  std::set<uint32_t> framebuffer_ids_;
+  std::map<uint32_t, uint32_t> crtc_fb_;
+
   base::queue<PageFlipCallback> callbacks_;
 
   std::vector<CrtcProperties> crtc_properties_;

ui/ozone/platform/drm/gpu/screen_manager_unittest.cc

@@ -701,4 +701,56 @@ TEST(ScreenManagerTest2, ShouldNotHardwareMirrorDifferentDrmDevices) {
   screen_manager.RemoveWindow(3)->Shutdown();
 }
 
+// crbug.com/888553
+TEST(ScreenManagerTest2, ShouldNotUnbindFramebufferOnJoiningMirror) {
+  auto gbm_device = std::make_unique<MockGbmDevice>();
+  auto drm_device = base::MakeRefCounted<MockDrmDevice>(std::move(gbm_device));
+  DrmDeviceManager drm_device_manager(nullptr);
+  ScreenManager screen_manager;
+
+  constexpr uint32_t kCrtc39 = 39;
+  constexpr uint32_t kConnector43 = 43;
+  constexpr uint32_t kCrtc41 = 41;
+  constexpr uint32_t kConnector46 = 46;
+
+  constexpr drmModeModeInfo kMode1080p60 = {
+      /* clock= */ 148500,
+      /* hdisplay= */ 1920,
+      /* hsync_start= */ 2008,
+      /* hsync_end= */ 2052,
+      /* htotal= */ 2200,
+      /* hskew= */ 0,
+      /* vdisplay= */ 1080,
+      /* vsync_start= */ 1084,
+      /* vsync_end= */ 1089,
+      /* vtotal= */ 1125,
+      /* vscan= */ 0,
+      /* vrefresh= */ 60,
+      /* flags= */ 0xa,
+      /* type= */ 64,
+      /* name= */ "1920x1080",
+  };
+
+  // Both displays connect at startup.
+  {
+    auto window1 =
+        std::make_unique<DrmWindow>(1, &drm_device_manager, &screen_manager);
+    window1->Initialize();
+    screen_manager.AddWindow(1, std::move(window1));
+    screen_manager.GetWindow(1)->SetBounds(gfx::Rect(0, 0, 1920, 1080));
+    screen_manager.AddDisplayController(drm_device, kCrtc39, kConnector43);
+    screen_manager.AddDisplayController(drm_device, kCrtc41, kConnector46);
+    screen_manager.ConfigureDisplayController(drm_device, kCrtc39, kConnector43,
+                                              gfx::Point(0, 0), kMode1080p60);
+    screen_manager.ConfigureDisplayController(drm_device, kCrtc41, kConnector46,
+                                              gfx::Point(0, 0), kMode1080p60);
+  }
+
+  EXPECT_NE(0u, drm_device->GetFramebufferForCrtc(kCrtc39));
+  EXPECT_NE(0u, drm_device->GetFramebufferForCrtc(kCrtc41));
+
+  // Cleanup.
+  screen_manager.RemoveWindow(1)->Shutdown();
+}
+
 }  // namespace ui