From 0d07cdd593f982e5f6e75706c7f9a99a3c5a9264 Mon Sep 17 00:00:00 2001 From: Corey Tabaka Date: Thu, 28 Sep 2017 11:15:50 -0700 Subject: Use the HWC caching mechanism to avoid stalls in the ion driver. HWC supports caching buffers for layers using "slot" assignments. Use this in VrFlinger to avoid importing a buffer handle every frame. The avoids periodic stalls we observe in the ion driver when mapping a buffer into the HWC address space. Bug: 66459419 Test: Observe systraces no longer have MapBuffer in HWC in steady state; system does not drop frames. Change-Id: Iba4161b33561322bfbccbfafe600b432a6fa7c44 --- libs/vr/libvrflinger/acquired_buffer.cpp | 23 ++++++++++--------- libs/vr/libvrflinger/acquired_buffer.h | 9 ++++++-- libs/vr/libvrflinger/display_surface.cpp | 2 +- libs/vr/libvrflinger/hardware_composer.cpp | 30 ++++++++++++++++++++---- libs/vr/libvrflinger/hardware_composer.h | 37 ++++++++++++++++++++++-------- 5 files changed, 73 insertions(+), 28 deletions(-) diff --git a/libs/vr/libvrflinger/acquired_buffer.cpp b/libs/vr/libvrflinger/acquired_buffer.cpp index fda9585dc4..9614c6d306 100644 --- a/libs/vr/libvrflinger/acquired_buffer.cpp +++ b/libs/vr/libvrflinger/acquired_buffer.cpp @@ -9,8 +9,8 @@ namespace android { namespace dvr { AcquiredBuffer::AcquiredBuffer(const std::shared_ptr& buffer, - LocalHandle acquire_fence) - : buffer_(buffer), acquire_fence_(std::move(acquire_fence)) {} + LocalHandle acquire_fence, std::size_t slot) + : buffer_(buffer), acquire_fence_(std::move(acquire_fence)), slot_(slot) {} AcquiredBuffer::AcquiredBuffer(const std::shared_ptr& buffer, int* error) { @@ -31,18 +31,20 @@ AcquiredBuffer::AcquiredBuffer(const std::shared_ptr& buffer, } } -AcquiredBuffer::AcquiredBuffer(AcquiredBuffer&& other) - : buffer_(std::move(other.buffer_)), - acquire_fence_(std::move(other.acquire_fence_)) {} +AcquiredBuffer::AcquiredBuffer(AcquiredBuffer&& other) { + *this = std::move(other); +} AcquiredBuffer::~AcquiredBuffer() { Release(LocalHandle(kEmptyFence)); } AcquiredBuffer& AcquiredBuffer::operator=(AcquiredBuffer&& other) { if (this != &other) { - Release(LocalHandle(kEmptyFence)); + Release(); - buffer_ = std::move(other.buffer_); - acquire_fence_ = std::move(other.acquire_fence_); + using std::swap; + swap(buffer_, other.buffer_); + swap(acquire_fence_, other.acquire_fence_); + swap(slot_, other.slot_); } return *this; } @@ -81,8 +83,6 @@ int AcquiredBuffer::Release(LocalHandle release_fence) { ALOGD_IF(TRACE, "AcquiredBuffer::Release: buffer_id=%d release_fence=%d", buffer_ ? buffer_->id() : -1, release_fence.Get()); if (buffer_) { - // Close the release fence since we can't transfer it with an async release. - release_fence.Close(); const int ret = buffer_->ReleaseAsync(); if (ret < 0) { ALOGE("AcquiredBuffer::Release: Failed to release buffer %d: %s", @@ -92,9 +92,10 @@ int AcquiredBuffer::Release(LocalHandle release_fence) { } buffer_ = nullptr; - acquire_fence_.Close(); } + acquire_fence_.Close(); + slot_ = 0; return 0; } diff --git a/libs/vr/libvrflinger/acquired_buffer.h b/libs/vr/libvrflinger/acquired_buffer.h index e0dc9f2ac2..32e912a65d 100644 --- a/libs/vr/libvrflinger/acquired_buffer.h +++ b/libs/vr/libvrflinger/acquired_buffer.h @@ -21,7 +21,7 @@ class AcquiredBuffer { // this constructor; the constructor does not attempt to ACQUIRE the buffer // itself. AcquiredBuffer(const std::shared_ptr& buffer, - pdx::LocalHandle acquire_fence); + pdx::LocalHandle acquire_fence, std::size_t slot = 0); // Constructs an AcquiredBuffer from a BufferConsumer. The BufferConsumer MUST // be in the POSTED state prior to calling this constructor, as this @@ -64,13 +64,18 @@ class AcquiredBuffer { // to the producer. On success, the BufferConsumer and acquire fence are set // to empty state; if release fails, the BufferConsumer and acquire fence are // left in place and a negative error code is returned. - int Release(pdx::LocalHandle release_fence); + int Release(pdx::LocalHandle release_fence = {}); + + // Returns the slot in the queue this buffer belongs to. Buffers that are not + // part of a queue return 0. + std::size_t slot() const { return slot_; } private: std::shared_ptr buffer_; // Mutable so that the fence can be closed when it is determined to be // signaled during IsAvailable(). mutable pdx::LocalHandle acquire_fence_; + std::size_t slot_{0}; AcquiredBuffer(const AcquiredBuffer&) = delete; void operator=(const AcquiredBuffer&) = delete; diff --git a/libs/vr/libvrflinger/display_surface.cpp b/libs/vr/libvrflinger/display_surface.cpp index 685378133e..3d132c95ea 100644 --- a/libs/vr/libvrflinger/display_surface.cpp +++ b/libs/vr/libvrflinger/display_surface.cpp @@ -382,7 +382,7 @@ void DirectDisplaySurface::DequeueBuffersLocked() { } acquired_buffers_.Append( - AcquiredBuffer(buffer_consumer, std::move(acquire_fence))); + AcquiredBuffer(buffer_consumer, std::move(acquire_fence), slot)); } } diff --git a/libs/vr/libvrflinger/hardware_composer.cpp b/libs/vr/libvrflinger/hardware_composer.cpp index de6477b1ef..b3da120c74 100644 --- a/libs/vr/libvrflinger/hardware_composer.cpp +++ b/libs/vr/libvrflinger/hardware_composer.cpp @@ -605,7 +605,7 @@ int HardwareComposer::PostThreadPollInterruptible( } else if (pfd[0].revents != 0) { return 0; } else if (pfd[1].revents != 0) { - ALOGI("VrHwcPost thread interrupted"); + ALOGI("VrHwcPost thread interrupted: revents=%x", pfd[1].revents); return kPostThreadInterrupted; } else { return 0; @@ -1069,6 +1069,7 @@ void Layer::Reset() { acquire_fence_.Close(); surface_rect_functions_applied_ = false; pending_visibility_settings_ = true; + cached_buffer_map_.clear(); } Layer::Layer(const std::shared_ptr& surface, @@ -1112,6 +1113,7 @@ Layer& Layer::operator=(Layer&& other) { swap(surface_rect_functions_applied_, other.surface_rect_functions_applied_); swap(pending_visibility_settings_, other.pending_visibility_settings_); + swap(cached_buffer_map_, other.cached_buffer_map_); } return *this; } @@ -1205,15 +1207,30 @@ void Layer::CommonLayerSetup() { UpdateLayerSettings(); } +bool Layer::CheckAndUpdateCachedBuffer(std::size_t slot, int buffer_id) { + auto search = cached_buffer_map_.find(slot); + if (search != cached_buffer_map_.end() && search->second == buffer_id) + return true; + + // Assign or update the buffer slot. + if (buffer_id >= 0) + cached_buffer_map_[slot] = buffer_id; + return false; +} + void Layer::Prepare() { - int right, bottom; + int right, bottom, id; sp handle; + std::size_t slot; // Acquire the next buffer according to the type of source. IfAnyOf::Call(&source_, [&](auto& source) { - std::tie(right, bottom, handle, acquire_fence_) = source.Acquire(); + std::tie(right, bottom, id, handle, acquire_fence_, slot) = + source.Acquire(); }); + TRACE_FORMAT("Layer::Prepare|buffer_id=%d;slot=%zu|", id, slot); + // Update any visibility (blending, z-order) changes that occurred since // last prepare. UpdateVisibilitySettings(); @@ -1243,10 +1260,15 @@ void Layer::Prepare() { composition_type_.cast()); } + // See if the HWC cache already has this buffer. + const bool cached = CheckAndUpdateCachedBuffer(slot, id); + if (cached) + handle = nullptr; + HWC::Error error{HWC::Error::None}; error = composer_->setLayerBuffer(HWC_DISPLAY_PRIMARY, hardware_composer_layer_, - 0, handle, acquire_fence_.Get()); + slot, handle, acquire_fence_.Get()); ALOGE_IF(error != HWC::Error::None, "Layer::Prepare: Error setting layer buffer: %s", diff --git a/libs/vr/libvrflinger/hardware_composer.h b/libs/vr/libvrflinger/hardware_composer.h index 8131e50989..7010db9630 100644 --- a/libs/vr/libvrflinger/hardware_composer.h +++ b/libs/vr/libvrflinger/hardware_composer.h @@ -161,6 +161,14 @@ class Layer { // Applies visibility settings that may have changed. void UpdateVisibilitySettings(); + // Checks whether the buffer, given by id, is associated with the given slot + // in the HWC buffer cache. If the slot is not associated with the given + // buffer the cache is updated to establish the association and the buffer + // should be sent to HWC using setLayerBuffer. Returns true if the association + // was already established, false if not. A buffer_id of -1 is never + // associated and always returns false. + bool CheckAndUpdateCachedBuffer(std::size_t slot, int buffer_id); + // Composer instance shared by all instances of Layer. This must be set // whenever a new instance of the Composer is created. This may be set to // nullptr as long as there are no instances of Layer that might need to use @@ -198,19 +206,21 @@ class Layer { // the previous buffer is returned or an empty value if no buffer has ever // been posted. When a new buffer is acquired the previous buffer's release // fence is passed out automatically. - std::tuple, pdx::LocalHandle> Acquire() { + std::tuple, pdx::LocalHandle, std::size_t> + Acquire() { if (surface->IsBufferAvailable()) { acquired_buffer.Release(std::move(release_fence)); acquired_buffer = surface->AcquireCurrentBuffer(); ATRACE_ASYNC_END("BufferPost", acquired_buffer.buffer()->id()); } if (!acquired_buffer.IsEmpty()) { - return std::make_tuple(acquired_buffer.buffer()->width(), - acquired_buffer.buffer()->height(), - acquired_buffer.buffer()->buffer()->buffer(), - acquired_buffer.ClaimAcquireFence()); + return std::make_tuple( + acquired_buffer.buffer()->width(), + acquired_buffer.buffer()->height(), acquired_buffer.buffer()->id(), + acquired_buffer.buffer()->buffer()->buffer(), + acquired_buffer.ClaimAcquireFence(), acquired_buffer.slot()); } else { - return std::make_tuple(0, 0, nullptr, pdx::LocalHandle{}); + return std::make_tuple(0, 0, -1, nullptr, pdx::LocalHandle{}, 0); } } @@ -242,12 +252,13 @@ class Layer { struct SourceBuffer { std::shared_ptr buffer; - std::tuple, pdx::LocalHandle> Acquire() { + std::tuple, pdx::LocalHandle, std::size_t> + Acquire() { if (buffer) - return std::make_tuple(buffer->width(), buffer->height(), - buffer->buffer(), pdx::LocalHandle{}); + return std::make_tuple(buffer->width(), buffer->height(), -1, + buffer->buffer(), pdx::LocalHandle{}, 0); else - return std::make_tuple(0, 0, nullptr, pdx::LocalHandle{}); + return std::make_tuple(0, 0, -1, nullptr, pdx::LocalHandle{}, 0); } void Finish(pdx::LocalHandle /*fence*/) {} @@ -266,6 +277,12 @@ class Layer { bool surface_rect_functions_applied_ = false; bool pending_visibility_settings_ = true; + // Map of buffer slot assignments that have already been established with HWC: + // slot -> buffer_id. When this map contains a matching slot and buffer_id the + // buffer argument to setLayerBuffer may be nullptr to avoid the cost of + // importing a buffer HWC already knows about. + std::map cached_buffer_map_; + Layer(const Layer&) = delete; void operator=(const Layer&) = delete; }; -- cgit v1.2.3