2 files changed, 109 insertions, 126 deletions

diff --git a/minui/graphics_drm.cpp b/minui/graphics_drm.cpp
index 81b49fd95..f81fd9d5c 100644
--- a/minui/graphics_drm.cpp
+++ b/minui/graphics_drm.cpp
@@ -24,74 +24,37 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include <memory>
+
+#include <android-base/macros.h>
+#include <android-base/stringprintf.h>
+#include <android-base/unique_fd.h>
 #include <drm_fourcc.h>
 #include <xf86drm.h>
 #include <xf86drmMode.h>
 
 #include "minui/minui.h"
 
-#define ARRAY_SIZE(A) (sizeof(A)/sizeof(*(A)))
-
-MinuiBackendDrm::MinuiBackendDrm()
-    : GRSurfaceDrms(), main_monitor_crtc(nullptr), main_monitor_connector(nullptr), drm_fd(-1) {}
-
-void MinuiBackendDrm::DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc) {
-  if (crtc) {
-    drmModeSetCrtc(drm_fd, crtc->crtc_id,
-                   0,         // fb_id
-                   0, 0,      // x,y
-                   nullptr,   // connectors
-                   0,         // connector_count
-                   nullptr);  // mode
-  }
-}
-
-int MinuiBackendDrm::DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, GRSurfaceDrm* surface) {
-  int ret = drmModeSetCrtc(drm_fd, crtc->crtc_id, surface->fb_id, 0, 0,  // x,y
-                           &main_monitor_connector->connector_id,
-                           1,  // connector_count
-                           &main_monitor_crtc->mode);
-
-  if (ret) {
-    printf("drmModeSetCrtc failed ret=%d\n", ret);
+GRSurfaceDrm::~GRSurfaceDrm() {
+  if (mmapped_buffer_) {
+    munmap(mmapped_buffer_, row_bytes * height);
   }
 
-  return ret;
-}
-
-void MinuiBackendDrm::Blank(bool blank) {
-  if (blank) {
-    DrmDisableCrtc(drm_fd, main_monitor_crtc);
-  } else {
-    DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[current_buffer]);
-  }
-}
-
-void MinuiBackendDrm::DrmDestroySurface(GRSurfaceDrm* surface) {
-  if (!surface) return;
-
-  if (surface->mmapped_buffer_) {
-    munmap(surface->mmapped_buffer_, surface->row_bytes * surface->height);
-  }
-
-  if (surface->fb_id) {
-    int ret = drmModeRmFB(drm_fd, surface->fb_id);
-    if (ret) {
-      printf("drmModeRmFB failed ret=%d\n", ret);
+  if (fb_id) {
+    if (drmModeRmFB(drm_fd_, fb_id) != 0) {
+      perror("Failed to drmModeRmFB");
+      // Falling through to free other resources.
     }
   }
 
-  if (surface->handle) {
+  if (handle) {
     drm_gem_close gem_close = {};
-    gem_close.handle = surface->handle;
+    gem_close.handle = handle;
 
-    int ret = drmIoctl(drm_fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
-    if (ret) {
-      printf("DRM_IOCTL_GEM_CLOSE failed ret=%d\n", ret);
+    if (drmIoctl(drm_fd_, DRM_IOCTL_GEM_CLOSE, &gem_close) != 0) {
+      perror("Failed to DRM_IOCTL_GEM_CLOSE");
     }
   }
-
-  delete surface;
 }
 
 static int drm_format_to_bpp(uint32_t format) {
@@ -111,10 +74,7 @@ static int drm_format_to_bpp(uint32_t format) {
   }
 }
 
-GRSurfaceDrm* MinuiBackendDrm::DrmCreateSurface(int width, int height) {
-  GRSurfaceDrm* surface = new GRSurfaceDrm;
-  *surface = {};
-
+std::unique_ptr<GRSurfaceDrm> GRSurfaceDrm::Create(int drm_fd, int width, int height) {
   uint32_t format;
   PixelFormat pixel_format = gr_pixel_format();
   // PixelFormat comes in byte order, whereas DRM_FORMAT_* uses little-endian
@@ -137,12 +97,12 @@ GRSurfaceDrm* MinuiBackendDrm::DrmCreateSurface(int width, int height) {
   create_dumb.bpp = drm_format_to_bpp(format);
   create_dumb.flags = 0;
 
-  int ret = drmIoctl(drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &create_dumb);
-  if (ret) {
-    printf("DRM_IOCTL_MODE_CREATE_DUMB failed ret=%d\n", ret);
-    DrmDestroySurface(surface);
+  if (drmIoctl(drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &create_dumb) != 0) {
+    perror("Failed to DRM_IOCTL_MODE_CREATE_DUMB");
     return nullptr;
   }
+
+  std::unique_ptr<GRSurfaceDrm> surface = std::make_unique<GRSurfaceDrm>(drm_fd);
   surface->handle = create_dumb.handle;
 
   uint32_t handles[4], pitches[4], offsets[4];
@@ -151,20 +111,16 @@ GRSurfaceDrm* MinuiBackendDrm::DrmCreateSurface(int width, int height) {
   pitches[0] = create_dumb.pitch;
   offsets[0] = 0;
 
-  ret =
-      drmModeAddFB2(drm_fd, width, height, format, handles, pitches, offsets, &(surface->fb_id), 0);
-  if (ret) {
-    printf("drmModeAddFB2 failed ret=%d\n", ret);
-    DrmDestroySurface(surface);
+  if (drmModeAddFB2(drm_fd, width, height, format, handles, pitches, offsets, &surface->fb_id, 0) !=
+      0) {
+    perror("Failed to drmModeAddFB2");
     return nullptr;
   }
 
   drm_mode_map_dumb map_dumb = {};
   map_dumb.handle = create_dumb.handle;
-  ret = drmIoctl(drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &map_dumb);
-  if (ret) {
-    printf("DRM_IOCTL_MODE_MAP_DUMB failed ret=%d\n", ret);
-    DrmDestroySurface(surface);
+  if (drmIoctl(drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &map_dumb) != 0) {
+    perror("Failed to DRM_IOCTL_MODE_MAP_DUMB");
     return nullptr;
   }
 
@@ -175,14 +131,44 @@ GRSurfaceDrm* MinuiBackendDrm::DrmCreateSurface(int width, int height) {
   auto mmapped = mmap(nullptr, surface->height * surface->row_bytes, PROT_READ | PROT_WRITE,
                       MAP_SHARED, drm_fd, map_dumb.offset);
   if (mmapped == MAP_FAILED) {
-    perror("mmap() failed");
-    DrmDestroySurface(surface);
+    perror("Failed to mmap()");
     return nullptr;
   }
   surface->mmapped_buffer_ = static_cast<uint8_t*>(mmapped);
   return surface;
 }
 
+void MinuiBackendDrm::DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc) {
+  if (crtc) {
+    drmModeSetCrtc(drm_fd, crtc->crtc_id,
+                   0,         // fb_id
+                   0, 0,      // x,y
+                   nullptr,   // connectors
+                   0,         // connector_count
+                   nullptr);  // mode
+  }
+}
+
+bool MinuiBackendDrm::DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc,
+                                    const std::unique_ptr<GRSurfaceDrm>& surface) {
+  if (drmModeSetCrtc(drm_fd, crtc->crtc_id, surface->fb_id, 0, 0,  // x,y
+                     &main_monitor_connector->connector_id,
+                     1,  // connector_count
+                     &main_monitor_crtc->mode) != 0) {
+    perror("Failed to drmModeSetCrtc");
+    return false;
+  }
+  return true;
+}
+
+void MinuiBackendDrm::Blank(bool blank) {
+  if (blank) {
+    DrmDisableCrtc(drm_fd, main_monitor_crtc);
+  } else {
+    DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[current_buffer]);
+  }
+}
+
 static drmModeCrtc* find_crtc_for_connector(int fd, drmModeRes* resources,
                                             drmModeConnector* connector) {
   // Find the encoder. If we already have one, just use it.
@@ -264,7 +250,7 @@ drmModeConnector* MinuiBackendDrm::FindMainMonitor(int fd, drmModeRes* resources
   do {
     main_monitor_connector = find_used_connector_by_type(fd, resources, kConnectorPriority[i]);
     i++;
-  } while (!main_monitor_connector && i < ARRAY_SIZE(kConnectorPriority));
+  } while (!main_monitor_connector && i < arraysize(kConnectorPriority));
 
   /* If we didn't find a connector, grab the first one that is connected. */
   if (!main_monitor_connector) {
@@ -298,60 +284,53 @@ void MinuiBackendDrm::DisableNonMainCrtcs(int fd, drmModeRes* resources, drmMode
 
 GRSurface* MinuiBackendDrm::Init() {
   drmModeRes* res = nullptr;
+  drm_fd = -1;
 
   /* Consider DRM devices in order. */
   for (int i = 0; i < DRM_MAX_MINOR; i++) {
-    char* dev_name;
-    int ret = asprintf(&dev_name, DRM_DEV_NAME, DRM_DIR_NAME, i);
-    if (ret < 0) continue;
+    auto dev_name = android::base::StringPrintf(DRM_DEV_NAME, DRM_DIR_NAME, i);
+    android::base::unique_fd fd(open(dev_name.c_str(), O_RDWR));
+    if (fd == -1) continue;
 
-    drm_fd = open(dev_name, O_RDWR, 0);
-    free(dev_name);
-    if (drm_fd < 0) continue;
-
-    uint64_t cap = 0;
     /* We need dumb buffers. */
-    ret = drmGetCap(drm_fd, DRM_CAP_DUMB_BUFFER, &cap);
-    if (ret || cap == 0) {
-      close(drm_fd);
+    if (uint64_t cap = 0; drmGetCap(fd.get(), DRM_CAP_DUMB_BUFFER, &cap) != 0 || cap == 0) {
       continue;
     }
 
-    res = drmModeGetResources(drm_fd);
+    res = drmModeGetResources(fd.get());
     if (!res) {
-      close(drm_fd);
       continue;
     }
 
     /* Use this device if it has at least one connected monitor. */
     if (res->count_crtcs > 0 && res->count_connectors > 0) {
-      if (find_first_connected_connector(drm_fd, res)) break;
+      if (find_first_connected_connector(fd.get(), res)) {
+        drm_fd = fd.release();
+        break;
+      }
     }
 
     drmModeFreeResources(res);
-    close(drm_fd);
     res = nullptr;
   }
 
-  if (drm_fd < 0 || res == nullptr) {
-    perror("cannot find/open a drm device");
+  if (drm_fd == -1 || res == nullptr) {
+    perror("Failed to find/open a drm device");
     return nullptr;
   }
 
   uint32_t selected_mode;
   main_monitor_connector = FindMainMonitor(drm_fd, res, &selected_mode);
-
   if (!main_monitor_connector) {
-    printf("main_monitor_connector not found\n");
+    fprintf(stderr, "Failed to find main_monitor_connector\n");
     drmModeFreeResources(res);
     close(drm_fd);
     return nullptr;
   }
 
   main_monitor_crtc = find_crtc_for_connector(drm_fd, res, main_monitor_connector);
-
   if (!main_monitor_crtc) {
-    printf("main_monitor_crtc not found\n");
+    fprintf(stderr, "Failed to find main_monitor_crtc\n");
     drmModeFreeResources(res);
     close(drm_fd);
     return nullptr;
@@ -366,21 +345,20 @@ GRSurface* MinuiBackendDrm::Init() {
 
   drmModeFreeResources(res);
 
-  GRSurfaceDrms[0] = DrmCreateSurface(width, height);
-  GRSurfaceDrms[1] = DrmCreateSurface(width, height);
+  GRSurfaceDrms[0] = GRSurfaceDrm::Create(drm_fd, width, height);
+  GRSurfaceDrms[1] = GRSurfaceDrm::Create(drm_fd, width, height);
   if (!GRSurfaceDrms[0] || !GRSurfaceDrms[1]) {
-    // GRSurfaceDrms and drm_fd should be freed in d'tor.
     return nullptr;
   }
 
   current_buffer = 0;
 
   // We will likely encounter errors in the backend functions (i.e. Flip) if EnableCrtc fails.
-  if (DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[1]) != 0) {
+  if (!DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[1])) {
     return nullptr;
   }
 
-  return GRSurfaceDrms[0];
+  return GRSurfaceDrms[0].get();
 }
 
 static void page_flip_complete(__unused int fd,
@@ -393,12 +371,9 @@ static void page_flip_complete(__unused int fd,
 
 GRSurface* MinuiBackendDrm::Flip() {
   bool ongoing_flip = true;
-
-  int ret = drmModePageFlip(drm_fd, main_monitor_crtc->crtc_id,
-                            GRSurfaceDrms[current_buffer]->fb_id,
-                            DRM_MODE_PAGE_FLIP_EVENT, &ongoing_flip);
-  if (ret < 0) {
-    printf("drmModePageFlip failed ret=%d\n", ret);
+  if (drmModePageFlip(drm_fd, main_monitor_crtc->crtc_id, GRSurfaceDrms[current_buffer]->fb_id,
+                      DRM_MODE_PAGE_FLIP_EVENT, &ongoing_flip) != 0) {
+    perror("Failed to drmModePageFlip");
     return nullptr;
   }
 
@@ -408,9 +383,8 @@ GRSurface* MinuiBackendDrm::Flip() {
       .events = POLLIN
     };
 
-    ret = poll(&fds, 1, -1);
-    if (ret == -1 || !(fds.revents & POLLIN)) {
-      printf("poll() failed on drm fd\n");
+    if (poll(&fds, 1, -1) == -1 || !(fds.revents & POLLIN)) {
+      perror("Failed to poll() on drm fd");
       break;
     }
 
@@ -419,21 +393,18 @@ GRSurface* MinuiBackendDrm::Flip() {
       .page_flip_handler = page_flip_complete
     };
 
-    ret = drmHandleEvent(drm_fd, &evctx);
-    if (ret != 0) {
-      printf("drmHandleEvent failed ret=%d\n", ret);
+    if (drmHandleEvent(drm_fd, &evctx) != 0) {
+      perror("Failed to drmHandleEvent");
       break;
     }
   }
 
   current_buffer = 1 - current_buffer;
-  return GRSurfaceDrms[current_buffer];
+  return GRSurfaceDrms[current_buffer].get();
 }
 
 MinuiBackendDrm::~MinuiBackendDrm() {
   DrmDisableCrtc(drm_fd, main_monitor_crtc);
-  DrmDestroySurface(GRSurfaceDrms[0]);
-  DrmDestroySurface(GRSurfaceDrms[1]);
   drmModeFreeCrtc(main_monitor_crtc);
   drmModeFreeConnector(main_monitor_connector);
   close(drm_fd);
diff --git a/minui/graphics_drm.h b/minui/graphics_drm.h
index f3aad6bfc..02db89f05 100644
--- a/minui/graphics_drm.h
+++ b/minui/graphics_drm.h
@@ -18,6 +18,9 @@
 
 #include <stdint.h>
 
+#include <memory>
+
+#include <android-base/macros.h>
 #include <xf86drmMode.h>
 
 #include "graphics.h"
@@ -25,6 +28,12 @@
 
 class GRSurfaceDrm : public GRSurface {
  public:
+  explicit GRSurfaceDrm(int drm_fd) : drm_fd_(drm_fd) {}
+  ~GRSurfaceDrm() override;
+
+  // Creates a GRSurfaceDrm instance.
+  static std::unique_ptr<GRSurfaceDrm> Create(int drm_fd, int width, int height);
+
   uint8_t* data() override {
     return mmapped_buffer_;
   }
@@ -32,30 +41,33 @@ class GRSurfaceDrm : public GRSurface {
  private:
   friend class MinuiBackendDrm;
 
-  uint32_t fb_id;
-  uint32_t handle;
+  const int drm_fd_;
+
+  uint32_t fb_id{ 0 };
+  uint32_t handle{ 0 };
   uint8_t* mmapped_buffer_{ nullptr };
+
+  DISALLOW_COPY_AND_ASSIGN(GRSurfaceDrm);
 };
 
 class MinuiBackendDrm : public MinuiBackend {
  public:
+  MinuiBackendDrm() = default;
+  ~MinuiBackendDrm() override;
+
   GRSurface* Init() override;
   GRSurface* Flip() override;
   void Blank(bool) override;
-  ~MinuiBackendDrm() override;
-  MinuiBackendDrm();
 
  private:
   void DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc);
-  int DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, GRSurfaceDrm* surface);
-  GRSurfaceDrm* DrmCreateSurface(int width, int height);
-  void DrmDestroySurface(GRSurfaceDrm* surface);
+  bool DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, const std::unique_ptr<GRSurfaceDrm>& surface);
   void DisableNonMainCrtcs(int fd, drmModeRes* resources, drmModeCrtc* main_crtc);
   drmModeConnector* FindMainMonitor(int fd, drmModeRes* resources, uint32_t* mode_index);
 
-  GRSurfaceDrm* GRSurfaceDrms[2];
-  int current_buffer;
-  drmModeCrtc* main_monitor_crtc;
-  drmModeConnector* main_monitor_connector;
-  int drm_fd;
+  std::unique_ptr<GRSurfaceDrm> GRSurfaceDrms[2];
+  int current_buffer{ 0 };
+  drmModeCrtc* main_monitor_crtc{ nullptr };
+  drmModeConnector* main_monitor_connector{ nullptr };
+  int drm_fd{ -1 };
 };