From c852dc6ad2dcb57a0ea848c1c36121881961bd31 Mon Sep 17 00:00:00 2001 From: Alessandro Astone Date: Sun, 29 Jan 2023 12:00:21 +0100 Subject: [PATCH 1/2] hwcomposer: Correctly synchronize buffer release * A buffer may be reused * The buffer cache may be invalidated before one of its buffers is released --- hwcomposer/hwcomposer.cpp | 33 +++++++++++++++------------------ hwcomposer/wayland-hwc.cpp | 26 ++++++++++++++++++++++---- hwcomposer/wayland-hwc.h | 10 ++++++++++ 3 files changed, 47 insertions(+), 22 deletions(-) diff --git a/hwcomposer/hwcomposer.cpp b/hwcomposer/hwcomposer.cpp index f6a9e8c4..d9a19ab0 100644 --- a/hwcomposer/hwcomposer.cpp +++ b/hwcomposer/hwcomposer.cpp @@ -188,24 +188,28 @@ static struct buffer *get_wl_buffer(struct waydroid_hwc_composer_device_1 *pdev, auto it = pdev->display->buffer_map.find(layer->handle); if (it != pdev->display->buffer_map.end()) { + std::lock_guard(pdev->display->buffers_mutex); if (it->second->isShm) { if (width != it->second->width || height != it->second->height) { - if (it->second->buffer) - wl_buffer_destroy(it->second->buffer); - delete (it->second); + it->second->refcount--; + if (it->second->refcount == 0) + destroy_buffer(it->second); pdev->display->buffer_map.erase(it); } else { + it->second->refcount++; update_shm_buffer(pdev->display, it->second); return it->second; } - } else + } else { + it->second->refcount++; return it->second; + } } struct buffer *buf; int ret = 0; - buf = new struct buffer(); + buf = create_buffer(pdev->display); if (pdev->display->gtype == GRALLOC_GBM) { struct gralloc_handle_t *drm_handle = (struct gralloc_handle_t *)layer->handle; if (pdev->display->dmabuf) { @@ -236,7 +240,7 @@ static struct buffer *get_wl_buffer(struct waydroid_hwc_composer_device_1 *pdev, return NULL; } pdev->display->buffer_map[layer->handle] = buf; - + buf->refcount++; return pdev->display->buffer_map[layer->handle]; } @@ -414,14 +418,11 @@ static int hwc_set(struct hwc_composer_device_1* dev,size_t numDisplays, int err = 0; if (pdev->display->geo_changed) { + std::lock_guard(pdev->display->buffers_mutex); for (auto it = pdev->display->buffer_map.begin(); it != pdev->display->buffer_map.end(); it++) { - if (it->second) { - if (it->second->buffer) - wl_buffer_destroy(it->second->buffer); - if (it->second->isShm) - munmap(it->second->shm_data, it->second->size); - delete (it->second); - } + it->second->refcount--; + if (it->second->refcount == 0) + destroy_buffer(it->second); } pdev->display->buffer_map.clear(); } @@ -769,12 +770,8 @@ static int hwc_set(struct hwc_composer_device_1* dev,size_t numDisplays, if (fb_layer->compositionType != HWC_FRAMEBUFFER && fb_layer->compositionType != HWC_SIDEBAND) { - int timeline_fd = sw_sync_timeline_create(); /* To be signaled when the compositor releases the buffer */ - fb_layer->releaseFenceFd = sw_sync_fence_create(timeline_fd, "wayland_release", 1); - buf->timeline_fd = timeline_fd; - } else { - buf->timeline_fd = -1; + fb_layer->releaseFenceFd = sw_sync_fence_create(buf->timeline_fd, "wayland_release", ++buf->sync_point); } struct wl_surface *surface = get_surface(pdev, fb_layer, window, pdev->use_subsurface); diff --git a/hwcomposer/wayland-hwc.cpp b/hwcomposer/wayland-hwc.cpp index 93d9f088..7308b0da 100644 --- a/hwcomposer/wayland-hwc.cpp +++ b/hwcomposer/wayland-hwc.cpp @@ -72,7 +72,23 @@ using ::android::hardware::hidl_string; -struct buffer; +struct buffer* +create_buffer(struct display* display) { + struct buffer* buf = new struct buffer(); + buf->display = display; + buf->timeline_fd = sw_sync_timeline_create(); + buf->refcount = 1; + return buf; +} + +void +destroy_buffer(struct buffer* buf) { + close(buf->timeline_fd); + wl_buffer_destroy(buf->buffer); + if (buf->isShm) + munmap(buf->shm_data, buf->size); + delete buf; +} static void buffer_release(void *data, struct wl_buffer *) @@ -80,8 +96,10 @@ buffer_release(void *data, struct wl_buffer *) struct buffer *mybuf = (struct buffer*)data; sw_sync_timeline_inc(mybuf->timeline_fd, 1); - close(mybuf->timeline_fd); - mybuf->timeline_fd = -1; + std::lock_guard(mybuf->display->buffers_mutex); + mybuf->refcount--; + if (mybuf->refcount == 0) + destroy_buffer(mybuf); } static const struct wl_buffer_listener buffer_listener = { @@ -296,7 +314,7 @@ void snapshot_inactive_app_window(struct display *display, struct window *window ALOGI("Making inactive window snapshot for %s", window->taskID.c_str()); struct buffer *old_buf = window->last_layer_buffer; - struct buffer *new_buf = new struct buffer(); + struct buffer *new_buf = create_buffer(display); // FIXME won't work as expected if there are multiple surfaces struct wl_surface *surface = window->surface; diff --git a/hwcomposer/wayland-hwc.h b/hwcomposer/wayland-hwc.h index 63ca789d..523e6bec 100644 --- a/hwcomposer/wayland-hwc.h +++ b/hwcomposer/wayland-hwc.h @@ -155,6 +155,7 @@ struct display { std::map layer_handles_ext; struct handleExt target_layer_handle_ext; std::map buffer_map; + std::mutex buffers_mutex; std::array keysDown; bool isWinResSet; @@ -174,9 +175,13 @@ struct buffer { uint32_t hal_format; int timeline_fd; + int sync_point; bool isShm; void *shm_data; int size; + + int refcount; + struct display* display; }; struct window { @@ -219,6 +224,11 @@ create_shm_wl_buffer(struct display *display, struct buffer *buffer, void snapshot_inactive_app_window(struct display *display, struct window *window); +void +destroy_buffer(struct buffer* buffer); +struct buffer* +create_buffer(struct display* display); + struct display * create_display(const char* gralloc); void From 84f46f25bebe633e31d130fe5eedbb45799acb19 Mon Sep 17 00:00:00 2001 From: Alessandro Astone Date: Wed, 1 Feb 2023 00:05:13 +0100 Subject: [PATCH 2/2] hwcomposer: Snapshot buffer is now also freed in release --- hwcomposer/hwcomposer.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hwcomposer/hwcomposer.cpp b/hwcomposer/hwcomposer.cpp index d9a19ab0..3b9aea7a 100644 --- a/hwcomposer/hwcomposer.cpp +++ b/hwcomposer/hwcomposer.cpp @@ -825,12 +825,8 @@ static int hwc_set(struct hwc_composer_device_1* dev,size_t numDisplays, wl_surface_commit(surface); - if (window->snapshot_buffer) { - // Snapshot buffer should be detached by now, clean up - wl_buffer_destroy(window->snapshot_buffer->buffer); - delete window->snapshot_buffer; - window->snapshot_buffer = nullptr; - } + // Snapshot buffer is no longer used + window->snapshot_buffer = nullptr; const int kAcquireWarningMS = 100; err = sync_wait(fb_layer->acquireFenceFd, kAcquireWarningMS);