From 4a22b28bea62852288e481f8e901939df94f87af Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 31 Oct 2018 00:00:31 -0700 Subject: minui: Refactor GRSurfaceFbdev. - Adds Create() that returns a GRSurfaceFbdev instance. - Moves away from using the copy ctor (precisely assignment operator) of GRSurfaceFbdev. - Moves the GRSurfaceFbdev deallocation code into GRSurfaceFbdev's dtor. - Manages MinuiBackendFbdev::gr_framebuffer with std::unique_ptr. Test: mmma -j bootable/recovery Test: `Run graphics test` on taimen. Change-Id: I8e67cda7bc3a2feec0790124d035caa36fb58a89 --- minui/graphics_fbdev.cpp | 68 +++++++++++++++++++++++------------------------- minui/graphics_fbdev.h | 29 +++++++++++++++------ 2 files changed, 53 insertions(+), 44 deletions(-) diff --git a/minui/graphics_fbdev.cpp b/minui/graphics_fbdev.cpp index f958d62d4..4da5613af 100644 --- a/minui/graphics_fbdev.cpp +++ b/minui/graphics_fbdev.cpp @@ -26,21 +26,27 @@ #include #include +#include + #include "minui/minui.h" -MinuiBackendFbdev::MinuiBackendFbdev() : gr_draw(nullptr), fb_fd(-1) {} +std::unique_ptr GRSurfaceFbdev::Create(int width, int height, int row_bytes, + int pixel_bytes) { + // Cannot use std::make_unique to access non-public ctor. + return std::unique_ptr(new GRSurfaceFbdev(width, height, row_bytes, pixel_bytes)); +} void MinuiBackendFbdev::Blank(bool blank) { int ret = ioctl(fb_fd, FBIOBLANK, blank ? FB_BLANK_POWERDOWN : FB_BLANK_UNBLANK); if (ret < 0) perror("ioctl(): blank"); } -void MinuiBackendFbdev::SetDisplayedFramebuffer(unsigned n) { +void MinuiBackendFbdev::SetDisplayedFramebuffer(size_t n) { if (n > 1 || !double_buffered) return; - vi.yres_virtual = gr_framebuffer[0].height * 2; - vi.yoffset = n * gr_framebuffer[0].height; - vi.bits_per_pixel = gr_framebuffer[0].pixel_bytes * 8; + vi.yres_virtual = gr_framebuffer[0]->height * 2; + vi.yoffset = n * gr_framebuffer[0]->height; + vi.bits_per_pixel = gr_framebuffer[0]->pixel_bytes * 8; if (ioctl(fb_fd, FBIOPUT_VSCREENINFO, &vi) < 0) { perror("active fb swap failed"); } @@ -96,35 +102,31 @@ GRSurface* MinuiBackendFbdev::Init() { memset(bits, 0, fi.smem_len); - gr_framebuffer[0].width = vi.xres; - gr_framebuffer[0].height = vi.yres; - gr_framebuffer[0].row_bytes = fi.line_length; - gr_framebuffer[0].pixel_bytes = vi.bits_per_pixel / 8; - gr_framebuffer[0].buffer_ = static_cast(bits); - memset(gr_framebuffer[0].buffer_, 0, gr_framebuffer[0].height * gr_framebuffer[0].row_bytes); + gr_framebuffer[0] = + GRSurfaceFbdev::Create(vi.xres, vi.yres, fi.line_length, vi.bits_per_pixel / 8); + gr_framebuffer[0]->buffer_ = static_cast(bits); + memset(gr_framebuffer[0]->buffer_, 0, gr_framebuffer[0]->height * gr_framebuffer[0]->row_bytes); + + gr_framebuffer[1] = + GRSurfaceFbdev::Create(gr_framebuffer[0]->width, gr_framebuffer[0]->height, + gr_framebuffer[0]->row_bytes, gr_framebuffer[0]->pixel_bytes); /* check if we can use double buffering */ if (vi.yres * fi.line_length * 2 <= fi.smem_len) { double_buffered = true; - gr_framebuffer[1] = gr_framebuffer[0]; - gr_framebuffer[1].buffer_ = - gr_framebuffer[0].buffer_ + gr_framebuffer[0].height * gr_framebuffer[0].row_bytes; - - gr_draw = gr_framebuffer + 1; - + gr_framebuffer[1]->buffer_ = + gr_framebuffer[0]->buffer_ + gr_framebuffer[0]->height * gr_framebuffer[0]->row_bytes; } else { double_buffered = false; - // Without double-buffering, we allocate RAM for a buffer to - // draw in, and then "flipping" the buffer consists of a - // memcpy from the buffer we allocated to the framebuffer. - - gr_draw = new GRSurfaceFbdev; - *gr_draw = gr_framebuffer[0]; - gr_draw->buffer_ = new uint8_t[gr_draw->height * gr_draw->row_bytes]; + // Without double-buffering, we allocate RAM for a buffer to draw in, and then "flipping" the + // buffer consists of a memcpy from the buffer we allocated to the framebuffer. + memory_buffer.resize(gr_framebuffer[1]->height * gr_framebuffer[1]->row_bytes); + gr_framebuffer[1]->buffer_ = memory_buffer.data(); } + gr_draw = gr_framebuffer[1].get(); memset(gr_draw->buffer_, 0, gr_draw->height * gr_draw->row_bytes); fb_fd = fd; SetDisplayedFramebuffer(0); @@ -139,25 +141,19 @@ GRSurface* MinuiBackendFbdev::Init() { GRSurface* MinuiBackendFbdev::Flip() { if (double_buffered) { - // Change gr_draw to point to the buffer currently displayed, - // then flip the driver so we're displaying the other buffer - // instead. - gr_draw = gr_framebuffer + displayed_buffer; + // Change gr_draw to point to the buffer currently displayed, then flip the driver so we're + // displaying the other buffer instead. + gr_draw = gr_framebuffer[displayed_buffer].get(); SetDisplayedFramebuffer(1 - displayed_buffer); } else { // Copy from the in-memory surface to the framebuffer. - memcpy(gr_framebuffer[0].buffer_, gr_draw->buffer_, gr_draw->height * gr_draw->row_bytes); + memcpy(gr_framebuffer[0]->buffer_, gr_draw->buffer_, gr_draw->height * gr_draw->row_bytes); } return gr_draw; } MinuiBackendFbdev::~MinuiBackendFbdev() { - close(fb_fd); - fb_fd = -1; - - if (!double_buffered && gr_draw) { - delete[] gr_draw->buffer_; - delete gr_draw; + if (fb_fd != -1) { + close(fb_fd); } - gr_draw = nullptr; } diff --git a/minui/graphics_fbdev.h b/minui/graphics_fbdev.h index be813dccb..934e584d7 100644 --- a/minui/graphics_fbdev.h +++ b/minui/graphics_fbdev.h @@ -19,37 +19,50 @@ #include #include +#include +#include + #include "graphics.h" #include "minui/minui.h" class GRSurfaceFbdev : public GRSurface { public: + // Creates and returns a GRSurfaceFbdev instance, or nullptr on error. + static std::unique_ptr Create(int width, int height, int row_bytes, + int pixel_bytes); + uint8_t* data() override { return buffer_; } + protected: + using GRSurface::GRSurface; + private: friend class MinuiBackendFbdev; // Points to the start of the buffer: either the mmap'd framebuffer or one allocated in-memory. - uint8_t* buffer_; + uint8_t* buffer_{ nullptr }; }; class MinuiBackendFbdev : public MinuiBackend { public: + MinuiBackendFbdev() = default; + ~MinuiBackendFbdev() override; + GRSurface* Init() override; GRSurface* Flip() override; void Blank(bool) override; - ~MinuiBackendFbdev() override; - MinuiBackendFbdev(); private: - void SetDisplayedFramebuffer(unsigned n); + void SetDisplayedFramebuffer(size_t n); - GRSurfaceFbdev gr_framebuffer[2]; + std::unique_ptr gr_framebuffer[2]; + // Points to the current surface (i.e. one of the two gr_framebuffer's). + GRSurfaceFbdev* gr_draw{ nullptr }; bool double_buffered; - GRSurfaceFbdev* gr_draw; - int displayed_buffer; + std::vector memory_buffer; + size_t displayed_buffer{ 0 }; fb_var_screeninfo vi; - int fb_fd; + int fb_fd{ -1 }; }; -- cgit v1.2.3 From 1b18cf56e21d038fcb03d09a13c90545cb1b6501 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 31 Oct 2018 00:12:11 -0700 Subject: minui: Refactor GRSurfaceAdf. Test: mmma -j bootable/recovery Change-Id: I14514017aace4b7043a9db1f5a93ec130a6f89c4 --- minui/graphics_adf.cpp | 89 +++++++++++++++++++++++++------------------------- minui/graphics_adf.h | 32 +++++++++++------- 2 files changed, 65 insertions(+), 56 deletions(-) diff --git a/minui/graphics_adf.cpp b/minui/graphics_adf.cpp index 6fc193f74..9eea497d6 100644 --- a/minui/graphics_adf.cpp +++ b/minui/graphics_adf.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -28,51 +29,60 @@ #include "minui/minui.h" -MinuiBackendAdf::MinuiBackendAdf() - : intf_fd(-1), dev(), current_surface(0), n_surfaces(0), surfaces() {} +GRSurfaceAdf::~GRSurfaceAdf() { + if (mmapped_buffer_) { + munmap(mmapped_buffer_, pitch * height); + } + if (fence_fd != -1) { + close(fence_fd); + } + if (fd != -1) { + close(fd); + } +} -int MinuiBackendAdf::SurfaceInit(const drm_mode_modeinfo* mode, GRSurfaceAdf* surf) { - *surf = {}; - surf->fence_fd = -1; - surf->fd = adf_interface_simple_buffer_alloc(intf_fd, mode->hdisplay, mode->vdisplay, format, - &surf->offset, &surf->pitch); - if (surf->fd < 0) { - return surf->fd; +std::unique_ptr GRSurfaceAdf::Create(int intf_fd, const drm_mode_modeinfo* mode, + __u32 format, int* err) { + __u32 offset; + __u32 pitch; + auto fd = adf_interface_simple_buffer_alloc(intf_fd, mode->hdisplay, mode->vdisplay, format, + &offset, &pitch); + + if (fd < 0) { + *err = fd; + return nullptr; } - surf->width = mode->hdisplay; - surf->height = mode->vdisplay; - surf->row_bytes = surf->pitch; - surf->pixel_bytes = (format == DRM_FORMAT_RGB565) ? 2 : 4; + std::unique_ptr surf = std::unique_ptr( + new GRSurfaceAdf(mode->hdisplay, mode->vdisplay, pitch, (format == DRM_FORMAT_RGB565 ? 2 : 4), + offset, pitch, fd)); auto mmapped = mmap(nullptr, surf->pitch * surf->height, PROT_WRITE, MAP_SHARED, surf->fd, surf->offset); if (mmapped == MAP_FAILED) { - int saved_errno = errno; - close(surf->fd); - return -saved_errno; + *err = -errno; + return nullptr; } surf->mmapped_buffer_ = static_cast(mmapped); - return 0; + return surf; } +MinuiBackendAdf::MinuiBackendAdf() : intf_fd(-1), dev(), current_surface(0), n_surfaces(0) {} + int MinuiBackendAdf::InterfaceInit() { adf_interface_data intf_data; - int err = adf_get_interface_data(intf_fd, &intf_data); - if (err < 0) return err; + if (int err = adf_get_interface_data(intf_fd, &intf_data); err < 0) return err; - int ret = 0; - err = SurfaceInit(&intf_data.current_mode, &surfaces[0]); - if (err < 0) { - fprintf(stderr, "allocating surface 0 failed: %s\n", strerror(-err)); - ret = err; + int result = 0; + surfaces[0] = GRSurfaceAdf::Create(intf_fd, &intf_data.current_mode, format, &result); + if (!surfaces[0]) { + fprintf(stderr, "Failed to allocate surface 0: %s\n", strerror(-result)); goto done; } - err = SurfaceInit(&intf_data.current_mode, &surfaces[1]); - if (err < 0) { - fprintf(stderr, "allocating surface 1 failed: %s\n", strerror(-err)); - surfaces[1] = {}; + surfaces[1] = GRSurfaceAdf::Create(intf_fd, &intf_data.current_mode, format, &result); + if (!surfaces[1]) { + fprintf(stderr, "Failed to allocate surface 1: %s\n", strerror(-result)); n_surfaces = 1; } else { n_surfaces = 2; @@ -80,7 +90,7 @@ int MinuiBackendAdf::InterfaceInit() { done: adf_free_interface_data(&intf_data); - return ret; + return result; } int MinuiBackendAdf::DeviceInit(adf_device* dev) { @@ -153,12 +163,12 @@ GRSurface* MinuiBackendAdf::Init() { } void MinuiBackendAdf::Sync(GRSurfaceAdf* surf) { - static constexpr unsigned int warningTimeout = 3000; + static constexpr unsigned int kWarningTimeout = 3000; if (surf == nullptr) return; if (surf->fence_fd >= 0) { - int err = sync_wait(surf->fence_fd, warningTimeout); + int err = sync_wait(surf->fence_fd, kWarningTimeout); if (err < 0) { perror("adf sync fence wait error\n"); } @@ -169,33 +179,22 @@ void MinuiBackendAdf::Sync(GRSurfaceAdf* surf) { } GRSurface* MinuiBackendAdf::Flip() { - GRSurfaceAdf* surf = &surfaces[current_surface]; + const auto& surf = surfaces[current_surface]; int fence_fd = adf_interface_simple_post(intf_fd, eng_id, surf->width, surf->height, format, surf->fd, surf->offset, surf->pitch, -1); if (fence_fd >= 0) surf->fence_fd = fence_fd; current_surface = (current_surface + 1) % n_surfaces; - Sync(&surfaces[current_surface]); - return &surfaces[current_surface]; + Sync(surfaces[current_surface].get()); + return surfaces[current_surface].get(); } void MinuiBackendAdf::Blank(bool blank) { adf_interface_blank(intf_fd, blank ? DRM_MODE_DPMS_OFF : DRM_MODE_DPMS_ON); } -void MinuiBackendAdf::SurfaceDestroy(GRSurfaceAdf* surf) { - if (surf->mmapped_buffer_) { - munmap(surf->mmapped_buffer_, surf->pitch * surf->height); - } - close(surf->fence_fd); - close(surf->fd); -} - MinuiBackendAdf::~MinuiBackendAdf() { adf_device_close(&dev); - for (unsigned int i = 0; i < n_surfaces; i++) { - SurfaceDestroy(&surfaces[i]); - } if (intf_fd >= 0) close(intf_fd); } diff --git a/minui/graphics_adf.h b/minui/graphics_adf.h index 099d32962..bf9842878 100644 --- a/minui/graphics_adf.h +++ b/minui/graphics_adf.h @@ -17,6 +17,9 @@ #pragma once #include +#include + +#include #include @@ -25,6 +28,11 @@ class GRSurfaceAdf : public GRSurface { public: + ~GRSurfaceAdf() override; + + static std::unique_ptr Create(int intf_fd, const drm_mode_modeinfo* mode, + __u32 format, int* err); + uint8_t* data() override { return mmapped_buffer_; } @@ -32,34 +40,36 @@ class GRSurfaceAdf : public GRSurface { private: friend class MinuiBackendAdf; - int fence_fd; - int fd; - __u32 offset; - __u32 pitch; + GRSurfaceAdf(int width, int height, int row_bytes, int pixel_bytes, __u32 offset, __u32 pitch, + int fd) + : GRSurface(width, height, row_bytes, pixel_bytes), offset(offset), pitch(pitch), fd(fd) {} + + const __u32 offset; + const __u32 pitch; + int fd; + int fence_fd{ -1 }; uint8_t* mmapped_buffer_{ nullptr }; }; class MinuiBackendAdf : public MinuiBackend { public: + MinuiBackendAdf(); + ~MinuiBackendAdf() override; GRSurface* Init() override; GRSurface* Flip() override; void Blank(bool) override; - ~MinuiBackendAdf() override; - MinuiBackendAdf(); private: - int SurfaceInit(const drm_mode_modeinfo* mode, GRSurfaceAdf* surf); int InterfaceInit(); int DeviceInit(adf_device* dev); - void SurfaceDestroy(GRSurfaceAdf* surf); void Sync(GRSurfaceAdf* surf); int intf_fd; adf_id_t eng_id; __u32 format; adf_device dev; - unsigned int current_surface; - unsigned int n_surfaces; - GRSurfaceAdf surfaces[2]; + size_t current_surface; + size_t n_surfaces; + std::unique_ptr surfaces[2]; }; -- cgit v1.2.3 From 710bc535f45570428f0c7b36bb4f20744247cb38 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 24 Oct 2018 07:59:49 -0700 Subject: minui: Remove the default and copy ctors for GRSurface. As well as all the derived classes. Instances must be created with Create(). A default copy ctor would mess up the ownership of the mapped or allocated buffer in these classes, so that has been explicitly removed. Test: mmma -j bootable/recovery Test: Run recovery_unit_test on marlin. Test: `Run graphics test` on blueline. Change-Id: I69ce001a9ec9e3ac851edb6ec4d3fa11f4aaea08 --- minui/graphics_drm.cpp | 10 +++------- minui/graphics_drm.h | 7 +++---- minui/include/minui/minui.h | 5 ++++- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/minui/graphics_drm.cpp b/minui/graphics_drm.cpp index f81fd9d5c..765e2625a 100644 --- a/minui/graphics_drm.cpp +++ b/minui/graphics_drm.cpp @@ -102,15 +102,15 @@ std::unique_ptr GRSurfaceDrm::Create(int drm_fd, int width, int he return nullptr; } - std::unique_ptr surface = std::make_unique(drm_fd); - surface->handle = create_dumb.handle; + // Cannot use std::make_unique to access non-public ctor. + auto surface = std::unique_ptr(new GRSurfaceDrm( + width, height, create_dumb.pitch, create_dumb.bpp / 8, drm_fd, create_dumb.handle)); uint32_t handles[4], pitches[4], offsets[4]; handles[0] = surface->handle; pitches[0] = create_dumb.pitch; offsets[0] = 0; - if (drmModeAddFB2(drm_fd, width, height, format, handles, pitches, offsets, &surface->fb_id, 0) != 0) { perror("Failed to drmModeAddFB2"); @@ -124,10 +124,6 @@ std::unique_ptr GRSurfaceDrm::Create(int drm_fd, int width, int he return nullptr; } - surface->height = height; - surface->width = width; - surface->row_bytes = create_dumb.pitch; - surface->pixel_bytes = create_dumb.bpp / 8; auto mmapped = mmap(nullptr, surface->height * surface->row_bytes, PROT_READ | PROT_WRITE, MAP_SHARED, drm_fd, map_dumb.offset); if (mmapped == MAP_FAILED) { diff --git a/minui/graphics_drm.h b/minui/graphics_drm.h index 02db89f05..6ba46e60b 100644 --- a/minui/graphics_drm.h +++ b/minui/graphics_drm.h @@ -20,7 +20,6 @@ #include -#include #include #include "graphics.h" @@ -28,7 +27,6 @@ class GRSurfaceDrm : public GRSurface { public: - explicit GRSurfaceDrm(int drm_fd) : drm_fd_(drm_fd) {} ~GRSurfaceDrm() override; // Creates a GRSurfaceDrm instance. @@ -41,13 +39,14 @@ class GRSurfaceDrm : public GRSurface { private: friend class MinuiBackendDrm; + GRSurfaceDrm(int width, int height, int row_bytes, int pixel_bytes, int drm_fd, uint32_t handle) + : GRSurface(width, height, row_bytes, pixel_bytes), drm_fd_(drm_fd), handle(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 { diff --git a/minui/include/minui/minui.h b/minui/include/minui/minui.h index 66d992b93..d6881e9a0 100644 --- a/minui/include/minui/minui.h +++ b/minui/include/minui/minui.h @@ -24,13 +24,14 @@ #include #include +#include + // // Graphics. // class GRSurface { public: - GRSurface() = default; virtual ~GRSurface(); // Creates and returns a GRSurface instance that's sufficient for storing an image of the given @@ -58,6 +59,8 @@ class GRSurface { private: uint8_t* data_{ nullptr }; + + DISALLOW_COPY_AND_ASSIGN(GRSurface); }; struct GRFont { -- cgit v1.2.3