[Ozone-DRI] Move framebuffer initialization into the buffer

Surface/buffer lifetimes are no longer bound by the controller. In addition,
it is possible for multiple controllers to be linked against the same surface,
making it impossible for a controller to know if the framebuffers need to be
unregistered. Thus move framebuffer initialization into the buffer object.

NOTRY=true

Review URL: https://codereview.chromium.org/276753003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269531 0039d316-1c4b-4281-b951-d872f2087c98

ui/ozone/platform/dri/dri_buffer.cc

@@ -17,6 +17,27 @@ namespace ui {
 
 namespace {
 
+// Modesetting cannot happen from a buffer with transparencies. Return the size
+// of a pixel without alpha.
+uint8_t GetColorDepth(SkColorType type) {
+  switch (type) {
+    case kUnknown_SkColorType:
+    case kAlpha_8_SkColorType:
+      return 0;
+    case kIndex_8_SkColorType:
+      return 8;
+    case kRGB_565_SkColorType:
+      return 16;
+    case kARGB_4444_SkColorType:
+      return 12;
+    case kPMColor_SkColorType:
+      return 24;
+    default:
+      NOTREACHED();
+      return 0;
+  }
+}
+
 void DestroyDumbBuffer(int fd, uint32_t handle) {
   struct drm_mode_destroy_dumb destroy_request;
   destroy_request.handle = handle;
@@ -77,6 +98,9 @@ DriBuffer::~DriBuffer() {
   if (!surface_)
     return;
 
+  if (framebuffer_)
+    dri_->RemoveFramebuffer(framebuffer_);
+
   SkImageInfo info;
   void* pixels = const_cast<void*>(surface_->peekPixels(&info, NULL));
   if (!pixels)
@@ -93,6 +117,17 @@ bool DriBuffer::Initialize(const SkImageInfo& info) {
     return false;
   }
 
+  if (!dri_->AddFramebuffer(info.width(),
+                            info.height(),
+                            GetColorDepth(info.colorType()),
+                            info.bytesPerPixel() << 3,
+                            stride_,
+                            handle_,
+                            &framebuffer_)) {
+    DLOG(ERROR) << "Failed to register framebuffer: " << strerror(errno);
+    return false;
+  }
+
   surface_ = skia::AdoptRef(SkSurface::NewRasterDirect(info, pixels, stride_));
   if (!surface_) {
     DLOG(ERROR) << "Cannot install Skia pixels for drm buffer";
@@ -102,23 +137,4 @@ bool DriBuffer::Initialize(const SkImageInfo& info) {
   return true;
 }
 
-uint8_t DriBuffer::GetColorDepth() const {
-  switch (surface_->getCanvas()->imageInfo().colorType()) {
-    case kUnknown_SkColorType:
-    case kAlpha_8_SkColorType:
-      return 0;
-    case kIndex_8_SkColorType:
-      return 8;
-    case kRGB_565_SkColorType:
-      return 16;
-    case kARGB_4444_SkColorType:
-      return 12;
-    case kPMColor_SkColorType:
-      return 24;
-    default:
-      NOTREACHED();
-      return 0;
-  }
-}
-
 }  // namespace ui

ui/ozone/platform/dri/dri_buffer.h

@@ -29,18 +29,11 @@ class OZONE_EXPORT DriBuffer {
   uint32_t framebuffer() const { return framebuffer_; }
   SkCanvas* canvas() { return surface_->getCanvas(); }
 
-  // Return the color depth of a pixel in this buffer.
-  uint8_t GetColorDepth() const;
-
   // Allocates the backing pixels and wraps them in |surface_|. |info| is used
   // to describe the buffer characteristics (size, color format).
   virtual bool Initialize(const SkImageInfo& info);
 
  protected:
-  friend class HardwareDisplayController;
-
-  void set_framebuffer(uint32_t framebuffer) { framebuffer_ = framebuffer; }
-
   DriWrapper* dri_;  // Not owned.
 
   // Wrapper around the native pixel memory.

ui/ozone/platform/dri/dri_surface_factory_unittest.cc

@@ -63,7 +63,8 @@ class MockDriWrapper : public ui::DriWrapper {
     return true;
   }
 
-  virtual bool AddFramebuffer(const drmModeModeInfo& mode,
+  virtual bool AddFramebuffer(uint32_t width,
+                              uint32_t height,
                               uint8_t depth,
                               uint8_t bpp,
                               uint32_t stride,
@@ -274,18 +275,6 @@ TEST_F(DriSurfaceFactoryTest, FailSurfaceInitialization) {
   EXPECT_FALSE(factory_->CreateCanvasForWidget(w));
 }
 
-TEST_F(DriSurfaceFactoryTest, FailBindingSurfaceToController) {
-  EXPECT_EQ(gfx::SurfaceFactoryOzone::INITIALIZED,
-            factory_->InitializeHardware());
-
-  factory_->get_drm()->set_add_framebuffer_expectation(false);
-
-  gfx::AcceleratedWidget w = factory_->GetAcceleratedWidget();
-  EXPECT_EQ(kDefaultWidgetHandle, w);
-
-  EXPECT_FALSE(factory_->CreateCanvasForWidget(w));
-}
-
 TEST_F(DriSurfaceFactoryTest, SuccessfulWidgetRealization) {
   EXPECT_EQ(gfx::SurfaceFactoryOzone::INITIALIZED,
             factory_->InitializeHardware());

ui/ozone/platform/dri/dri_surface_unittest.cc

@@ -27,7 +27,7 @@ const uint32_t kCrtcId = 1;
 
 class MockDriWrapper : public ui::DriWrapper {
  public:
-  MockDriWrapper() : DriWrapper(""), id_(1) { fd_ = kFd; }
+  MockDriWrapper() : DriWrapper("") { fd_ = kFd; }
   virtual ~MockDriWrapper() { fd_ = -1; }
 
   virtual drmModeCrtc* GetCrtc(uint32_t crtc_id) OVERRIDE { return NULL; }
@@ -39,15 +39,13 @@ class MockDriWrapper : public ui::DriWrapper {
   virtual bool SetCrtc(drmModeCrtc* crtc, uint32_t* connectors) OVERRIDE {
     return true;
   }
-  virtual bool AddFramebuffer(const drmModeModeInfo& mode,
+  virtual bool AddFramebuffer(uint32_t width,
+                              uint32_t height,
                               uint8_t depth,
                               uint8_t bpp,
                               uint32_t stride,
                               uint32_t handle,
-                              uint32_t* framebuffer) OVERRIDE {
-    *framebuffer = id_++;
-    return true;
-  }
+                              uint32_t* framebuffer) OVERRIDE { return true; }
   virtual bool RemoveFramebuffer(uint32_t framebuffer) OVERRIDE { return true; }
   virtual bool PageFlip(uint32_t crtc_id,
                         uint32_t framebuffer,
@@ -67,14 +65,17 @@ class MockDriWrapper : public ui::DriWrapper {
   }
 
  private:
-  int id_;
   DISALLOW_COPY_AND_ASSIGN(MockDriWrapper);
 };
 
 class MockDriBuffer : public ui::DriBuffer {
  public:
-  MockDriBuffer(ui::DriWrapper* dri, bool initialize_expectation)
-      : DriBuffer(dri), initialize_expectation_(initialize_expectation) {}
+  MockDriBuffer(ui::DriWrapper* dri,
+                bool initialize_expectation,
+                int framebuffer)
+      : DriBuffer(dri), initialize_expectation_(initialize_expectation) {
+    framebuffer_ = framebuffer;
+  }
   virtual ~MockDriBuffer() {
     surface_.clear();
   }
@@ -98,7 +99,10 @@ class MockDriBuffer : public ui::DriBuffer {
 class MockDriSurface : public ui::DriSurface {
  public:
   MockDriSurface(ui::DriWrapper* dri, const gfx::Size& size)
-      : DriSurface(dri, size), dri_(dri), initialize_expectation_(true) {}
+      : DriSurface(dri, size),
+        dri_(dri),
+        initialize_expectation_(true),
+        framebuffer_(0) {}
   virtual ~MockDriSurface() {}
 
   void set_initialize_expectation(bool state) {
@@ -107,11 +111,12 @@ class MockDriSurface : public ui::DriSurface {
 
  private:
   virtual ui::DriBuffer* CreateBuffer() OVERRIDE {
-    return new MockDriBuffer(dri_, initialize_expectation_);
+    return new MockDriBuffer(dri_, initialize_expectation_, ++framebuffer_);
   }
 
   ui::DriWrapper* dri_;
   bool initialize_expectation_;
+  int framebuffer_;
 
   DISALLOW_COPY_AND_ASSIGN(MockDriSurface);
 };

ui/ozone/platform/dri/dri_wrapper.cc

@@ -55,16 +55,17 @@ bool DriWrapper::DisableCrtc(uint32_t crtc_id) {
   return !drmModeSetCrtc(fd_, crtc_id, 0, 0, 0, NULL, 0, NULL);
 }
 
-bool DriWrapper::AddFramebuffer(const drmModeModeInfo& mode,
-                                     uint8_t depth,
-                                     uint8_t bpp,
-                                     uint32_t stride,
-                                     uint32_t handle,
-                                     uint32_t* framebuffer) {
+bool DriWrapper::AddFramebuffer(uint32_t width,
+                                uint32_t height,
+                                uint8_t depth,
+                                uint8_t bpp,
+                                uint32_t stride,
+                                uint32_t handle,
+                                uint32_t* framebuffer) {
   CHECK(fd_ >= 0);
   return !drmModeAddFB(fd_,
-                       mode.hdisplay,
-                       mode.vdisplay,
+                       width,
+                       height,
                        depth,
                        bpp,
                        stride,
@@ -78,8 +79,8 @@ bool DriWrapper::RemoveFramebuffer(uint32_t framebuffer) {
 }
 
 bool DriWrapper::PageFlip(uint32_t crtc_id,
-                               uint32_t framebuffer,
-                               void* data) {
+                          uint32_t framebuffer,
+                          void* data) {
   CHECK(fd_ >= 0);
   return !drmModePageFlip(fd_,
                           crtc_id,

ui/ozone/platform/dri/dri_wrapper.h

@@ -7,7 +7,7 @@
 
 #include <stdint.h>
 
-#include "base/basictypes.h"
+#include "base/macros.h"
 #include "ui/ozone/ozone_export.h"
 
 typedef struct _drmModeConnector drmModeConnector;
@@ -52,7 +52,8 @@ class OZONE_EXPORT DriWrapper {
 
   // Register a buffer with the CRTC. On successful registration, the CRTC will
   // assign a framebuffer ID to |framebuffer|.
-  virtual bool AddFramebuffer(const drmModeModeInfo& mode,
+  virtual bool AddFramebuffer(uint32_t width,
+                              uint32_t height,
                               uint8_t depth,
                               uint8_t bpp,
                               uint32_t stride,

ui/ozone/platform/dri/hardware_display_controller.cc

@@ -40,9 +40,6 @@ HardwareDisplayController::BindSurfaceToController(
     scoped_ptr<DriSurface> surface, drmModeModeInfo mode) {
   CHECK(surface);
 
-  if (!RegisterFramebuffers(surface.get(), mode))
-    return false;
-
   if (!drm_->SetCrtc(crtc_id_,
                      surface->GetFramebufferId(),
                      &connector_id_,
@@ -59,8 +56,6 @@ HardwareDisplayController::BindSurfaceToController(
 }
 
 void HardwareDisplayController::UnbindSurfaceFromController() {
-  if (surface_)
-    UnregisterFramebuffers(surface_.get());
   surface_.reset();
 }
 
@@ -104,33 +99,4 @@ bool HardwareDisplayController::MoveCursor(const gfx::Point& location) {
   return drm_->MoveCursor(crtc_id_, location.x(), location.y());
 }
 
-bool HardwareDisplayController::RegisterFramebuffers(DriSurface* surface,
-                                                     drmModeModeInfo mode) {
-  // Register the buffers.
-  for (size_t i = 0; i < arraysize(surface->bitmaps_); ++i) {
-    uint32_t fb_id;
-    if (!drm_->AddFramebuffer(
-            mode,
-            surface->bitmaps_[i]->GetColorDepth(),
-            surface->bitmaps_[i]->canvas()->imageInfo().bytesPerPixel() << 3,
-            surface->bitmaps_[i]->stride(),
-            surface->bitmaps_[i]->handle(),
-            &fb_id)) {
-      DLOG(ERROR) << "Failed to register framebuffer: " << strerror(errno);
-      return false;
-    }
-    surface->bitmaps_[i]->set_framebuffer(fb_id);
-  }
-
-  return true;
-}
-
-void HardwareDisplayController::UnregisterFramebuffers(DriSurface* surface) {
-  // Unregister the buffers.
-  for (size_t i = 0; i < arraysize(surface->bitmaps_); ++i) {
-    if (!drm_->RemoveFramebuffer(surface->bitmaps_[i]->framebuffer()))
-      DLOG(ERROR) << "Failed to remove FB: " << strerror(errno);
-  }
-}
-
 }  // namespace ui

ui/ozone/platform/dri/hardware_display_controller.h

@@ -142,9 +142,6 @@ class OZONE_EXPORT HardwareDisplayController {
   };
 
  private:
-  bool RegisterFramebuffers(DriSurface* surface, drmModeModeInfo mode);
-  void UnregisterFramebuffers(DriSurface* surface);
-
   // Object containing the connection to the graphics device and wraps the API
   // calls to control it.
   DriWrapper* drm_;

ui/ozone/platform/dri/hardware_display_controller_unittest.cc

@@ -65,7 +65,8 @@ class MockDriWrapper : public ui::DriWrapper {
     return true;
   }
 
-  virtual bool AddFramebuffer(const drmModeModeInfo& mode,
+  virtual bool AddFramebuffer(uint32_t width,
+                              uint32_t height,
                               uint8_t depth,
                               uint8_t bpp,
                               uint32_t stride,
@@ -226,24 +227,9 @@ TEST_F(HardwareDisplayControllerTest, CheckStateAfterSurfaceIsBound) {
   EXPECT_TRUE(controller_->BindSurfaceToController(surface.Pass(),
                                                    kDefaultMode));
 
-  EXPECT_EQ(2, drm_->get_add_framebuffer_call_count());
   EXPECT_TRUE(controller_->get_surface() != NULL);
 }
 
-TEST_F(HardwareDisplayControllerTest, CheckStateIfBindingFails) {
-  drm_->set_add_framebuffer_expectation(false);
-
-  scoped_ptr<ui::DriSurface> surface(
-      new MockDriSurface(drm_.get(), kDefaultModeSize));
-
-  EXPECT_TRUE(surface->Initialize());
-  EXPECT_FALSE(controller_->BindSurfaceToController(surface.Pass(),
-                                                    kDefaultMode));
-
-  EXPECT_EQ(1, drm_->get_add_framebuffer_call_count());
-  EXPECT_EQ(NULL, controller_->get_surface());
-}
-
 TEST_F(HardwareDisplayControllerTest, CheckStateAfterPageFlip) {
   scoped_ptr<ui::DriSurface> surface(
       new MockDriSurface(drm_.get(), kDefaultModeSize));
@@ -279,17 +265,3 @@ TEST_F(HardwareDisplayControllerTest, CheckStateIfPageFlipFails) {
                                                    kDefaultMode));
   EXPECT_FALSE(controller_->SchedulePageFlip());
 }
-
-TEST_F(HardwareDisplayControllerTest, CheckProperDestruction) {
-  scoped_ptr<ui::DriSurface> surface(
-      new MockDriSurface(drm_.get(), kDefaultModeSize));
-
-  EXPECT_TRUE(surface->Initialize());
-  EXPECT_TRUE(controller_->BindSurfaceToController(surface.Pass(),
-                                                   kDefaultMode));
-  EXPECT_TRUE(controller_->get_surface() != NULL);
-
-  controller_.reset();
-
-  EXPECT_EQ(2, drm_->get_remove_framebuffer_call_count());
-}