ozone: fix HDPMLegacy - do the PF after overlays, also clear old overlays

ui/ozone/platform/dri/hardware_display_plane_manager_legacy.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/ozone/platform/dri/hardware_display_plane_manager_legacy.h"
 
 #include "base/bind.h"
 #include "ui/ozone/platform/dri/crtc_controller.h"
 #include "ui/ozone/platform/dri/dri_wrapper.h"
 #include "ui/ozone/platform/dri/scanout_buffer.h"
 
 namespace ui {
 
 HardwareDisplayPlaneManagerLegacy::HardwareDisplayPlaneManagerLegacy() {
 }
 
 HardwareDisplayPlaneManagerLegacy::~HardwareDisplayPlaneManagerLegacy() {
 }
 
 bool HardwareDisplayPlaneManagerLegacy::Commit(
     HardwareDisplayPlaneList* plane_list) {
   if (plane_list->plane_list.empty())  // No assigned planes, nothing to do.
     return true;
   bool ret = true;
-  plane_list->plane_list.swap(plane_list->old_plane_list);
-  plane_list->plane_list.clear();
   for (const auto& flip : plane_list->legacy_page_flips) {

[dnicoara, 2015/01/16 21:40:08]

Add a comment before the for-loop mentioning why this needs to happen before the
page flip.

[achaulk, 2015/01/21 21:50:53]

Done.

     // Permission Denied is a legitimate error
+    for (const auto& plane : flip.planes) {
+      if (!drm_->PageFlipOverlay(flip.crtc_id, plane.framebuffer, plane.bounds,
+                                 plane.src_rect, plane.plane)) {
+        LOG(ERROR) << "Cannot display plane on overlay: error="
+                   << strerror(errno) << "crtc=" << flip.crtc
+                   << " plane=" << plane.plane;
+        ret = false;
+        flip.crtc->PageFlipFailed();
+        break;
+      }
+    }
     if (!drm_->PageFlip(flip.crtc_id, flip.framebuffer,
                         base::Bind(&CrtcController::OnPageFlipEvent,
                                    flip.crtc->AsWeakPtr()))) {
       if (errno != EACCES) {
         LOG(ERROR) << "Cannot page flip: error='" << strerror(errno) << "'"
                    << " crtc=" << flip.crtc_id
                    << " framebuffer=" << flip.framebuffer;
         LOG(ERROR) << "Failed to commit planes";
         ret = false;
       }
       flip.crtc->PageFlipFailed();
-    } else {
-      for (const auto& plane : flip.planes) {
-        if (!drm_->PageFlipOverlay(flip.crtc_id, plane.framebuffer,
-                                   plane.bounds, plane.src_rect, plane.plane)) {
-          LOG(ERROR) << "Cannot display plane on overlay: error="
-                     << strerror(errno);
-          ret = false;
-          flip.crtc->PageFlipFailed();
-          break;
-        }
+    }
+  }
+  for (HardwareDisplayPlane* plane : plane_list->old_plane_list) {

[dnicoara, 2015/01/16 21:40:08]

Should this be done before the PageFlip()? I think that if we're doing it after
it may result in old planes being shown when they shouldn't.

Also could you add a comment right before the for-loop explaining what this
does, just to make it easier for other readers.

[achaulk, 2015/01/21 21:50:53]

No, it only changes planes that haven't been re-used elsewhere

+    bool found =
+        std::find(plane_list->plane_list.begin(), plane_list->plane_list.end(),
+                  plane) != plane_list->plane_list.end();
+    if (!found && !plane->in_use()) {
+      // This plane is being released, so we need to zero it.
+      if (!drm_->PageFlipOverlay(plane->owning_crtc(), 0, gfx::Rect(),
+                                 gfx::Rect(), plane->plane_id())) {
+        LOG(ERROR) << "Cannot free overlay: error=" << strerror(errno)
+                   << "crtc=" << plane->owning_crtc()
+                   << " plane=" << plane->plane_id();
+        ret = false;
+        break;
       }
     }
   }
+  plane_list->plane_list.swap(plane_list->old_plane_list);
+  plane_list->plane_list.clear();
   plane_list->legacy_page_flips.clear();
+  plane_list->committed = true;
   return ret;
 }
 
 bool HardwareDisplayPlaneManagerLegacy::SetPlaneData(
     HardwareDisplayPlaneList* plane_list,
     HardwareDisplayPlane* hw_plane,
     const OverlayPlane& overlay,
     uint32_t crtc_id,
     const gfx::Rect& src_rect,
     CrtcController* crtc) {
   if (plane_list->legacy_page_flips.empty() ||
       plane_list->legacy_page_flips.back().crtc_id != crtc_id) {
     plane_list->legacy_page_flips.push_back(
         HardwareDisplayPlaneList::PageFlipInfo(
             crtc_id, overlay.buffer->GetFramebufferId(), hw_plane->plane_id(),
             crtc));
   } else {
     plane_list->legacy_page_flips.back().planes.push_back(
         HardwareDisplayPlaneList::PageFlipInfo::Plane(
             hw_plane->plane_id(), overlay.buffer->GetFramebufferId(),
             overlay.display_bounds, src_rect));
   }
   return true;
 }
 
 }  // namespace ui