From 2370109af70b31937b44c663ab4882d2c74fbc53 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Sat, 26 Sep 2020 02:08:41 +0300 Subject: [PATCH] drm_hwcomposer: Fix RegisterCallback() function - Fixes segfault during client switch. - Allows to run VTS on Android-11. VTS Results: ============================================ arm64-v8a VtsHalGraphicsComposerV2_1TargetTest: [53 tests / 42808 msec] armeabi-v7a VtsHalGraphicsComposerV2_1TargetTest: [53 tests / 33353 msec] =============== Summary =============== 2/2 modules completed Total Tests : 106 PASSED : 106 FAILED : 0 ============================================ Signed-off-by: Roman Stratiienko --- DrmHwcTwo.cpp | 51 ++++++++----------------------------- DrmHwcTwo.h | 24 +++++++++-------- compositor/DrmDisplayCompositor.cpp | 6 +++-- compositor/DrmDisplayCompositor.h | 12 ++++++--- drm/VSyncWorker.cpp | 42 +++++++++++------------------- drm/VSyncWorker.h | 6 +++++ 6 files changed, 58 insertions(+), 83 deletions(-) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 65a317c..81bb96d 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -32,22 +32,6 @@ namespace android { -class DrmVsyncCallback : public VsyncCallback { - public: - DrmVsyncCallback(hwc2_callback_data_t data, hwc2_function_pointer_t hook) - : data_(data), hook_(hook) { - } - - void Callback(int display, int64_t timestamp) { - auto hook = reinterpret_cast(hook_); - hook(data_, display, timestamp); - } - - private: - hwc2_callback_data_t data_; - hwc2_function_pointer_t hook_; -}; - DrmHwcTwo::DrmHwcTwo() { common.tag = HARDWARE_DEVICE_TAG; common.version = HWC_DEVICE_API_VERSION_2_0; @@ -194,17 +178,10 @@ HWC2::Error DrmHwcTwo::RegisterCallback(int32_t descriptor, hwc2_callback_data_t data, hwc2_function_pointer_t function) { supported(__func__); - auto callback = static_cast(descriptor); - if (!function) { - callbacks_.erase(callback); - return HWC2::Error::None; - } - - callbacks_.emplace(callback, HwcCallback(data, function)); - - switch (callback) { + switch (static_cast(descriptor)) { case HWC2::Callback::Hotplug: { + SetHotplugCallback(data, function); auto &drmDevices = resource_manager_.getDrmDevices(); for (auto &device : drmDevices) HandleInitialHotplugState(device.get()); @@ -317,21 +294,16 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ChosePreferredConfig() { return SetActiveConfig(connector_->get_preferred_mode_id()); } -HWC2::Error DrmHwcTwo::HwcDisplay::RegisterVsyncCallback( +void DrmHwcTwo::HwcDisplay::RegisterVsyncCallback( hwc2_callback_data_t data, hwc2_function_pointer_t func) { supported(__func__); - auto callback = std::make_shared(data, func); - vsync_worker_.RegisterCallback(std::move(callback)); - return HWC2::Error::None; + vsync_worker_.RegisterClientCallback(data, func); } void DrmHwcTwo::HwcDisplay::RegisterRefreshCallback( hwc2_callback_data_t data, hwc2_function_pointer_t func) { supported(__func__); - auto hook = reinterpret_cast(func); - compositor_.SetRefreshCallback([data, hook](int display) { - hook(data, static_cast(display)); - }); + compositor_.SetRefreshCallback(data, func); } HWC2::Error DrmHwcTwo::HwcDisplay::AcceptDisplayChanges() { @@ -1110,14 +1082,13 @@ void DrmHwcTwo::HwcLayer::PopulateDrmLayer(DrmHwcLayer *layer) { } void DrmHwcTwo::HandleDisplayHotplug(hwc2_display_t displayid, int state) { - auto cb = callbacks_.find(HWC2::Callback::Hotplug); - if (cb == callbacks_.end()) - return; + const std::lock_guard lock(hotplug_callback_lock); - auto hotplug = reinterpret_cast(cb->second.func); - hotplug(cb->second.data, displayid, - (state == DRM_MODE_CONNECTED ? HWC2_CONNECTION_CONNECTED - : HWC2_CONNECTION_DISCONNECTED)); + if (hotplug_callback_hook_ && hotplug_callback_data_) + hotplug_callback_hook_(hotplug_callback_data_, displayid, + state == DRM_MODE_CONNECTED + ? HWC2_CONNECTION_CONNECTED + : HWC2_CONNECTION_DISCONNECTED); } void DrmHwcTwo::HandleInitialHotplugState(DrmDevice *drmDevice) { diff --git a/DrmHwcTwo.h b/DrmHwcTwo.h index ca7a00b..7c3b856 100644 --- a/DrmHwcTwo.h +++ b/DrmHwcTwo.h @@ -42,6 +42,17 @@ class DrmHwcTwo : public hwc2_device_t { HWC2::Error Init(); + hwc2_callback_data_t hotplug_callback_data_ = NULL; + HWC2_PFN_HOTPLUG hotplug_callback_hook_ = NULL; + std::mutex hotplug_callback_lock; + + void SetHotplugCallback(hwc2_callback_data_t data, + hwc2_function_pointer_t hook) { + const std::lock_guard lock(hotplug_callback_lock); + hotplug_callback_data_ = data; + hotplug_callback_hook_ = reinterpret_cast(hook); + } + class HwcLayer { public: HWC2::Composition sf_type() const { @@ -149,14 +160,6 @@ class DrmHwcTwo : public hwc2_device_t { android_dataspace_t dataspace_ = HAL_DATASPACE_UNKNOWN; }; - struct HwcCallback { - HwcCallback(hwc2_callback_data_t d, hwc2_function_pointer_t f) - : data(d), func(f) { - } - hwc2_callback_data_t data; - hwc2_function_pointer_t func; - }; - class HwcDisplay { public: HwcDisplay(ResourceManager *resource_manager, DrmDevice *drm, @@ -165,8 +168,8 @@ class DrmHwcTwo : public hwc2_device_t { HwcDisplay(const HwcDisplay &) = delete; HWC2::Error Init(std::vector *planes); - HWC2::Error RegisterVsyncCallback(hwc2_callback_data_t data, - hwc2_function_pointer_t func); + void RegisterVsyncCallback(hwc2_callback_data_t data, + hwc2_function_pointer_t func); void RegisterRefreshCallback(hwc2_callback_data_t data, hwc2_function_pointer_t func); HWC2::Error CreateComposition(bool test); @@ -422,7 +425,6 @@ class DrmHwcTwo : public hwc2_device_t { ResourceManager resource_manager_; std::map displays_; - std::map callbacks_; std::string mDumpString; }; diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp index 467f8ba..ba0d56b 100644 --- a/compositor/DrmDisplayCompositor.cpp +++ b/compositor/DrmDisplayCompositor.cpp @@ -813,7 +813,9 @@ bool DrmDisplayCompositor::IsFlatteningNeeded() const { } int DrmDisplayCompositor::FlattenOnClient() { - if (refresh_display_cb_) { + const std::lock_guard lock(refresh_callback_lock); + + if (refresh_callback_hook_ && refresh_callback_data_) { { AutoLock lock(&lock_, __func__); if (!IsFlatteningNeeded()) { @@ -829,7 +831,7 @@ int DrmDisplayCompositor::FlattenOnClient() { "No writeback connector available, " "falling back to client composition"); SetFlattening(FlatteningState::kClientRequested); - refresh_display_cb_(display_); + refresh_callback_hook_(refresh_callback_data_, display_); return 0; } else { ALOGV("No writeback connector available"); diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h index e500c9e..29afc66 100644 --- a/compositor/DrmDisplayCompositor.h +++ b/compositor/DrmDisplayCompositor.h @@ -59,9 +59,15 @@ class DrmDisplayCompositor { int Init(ResourceManager *resource_manager, int display); - template - void SetRefreshCallback(Fn &&refresh_cb) { - refresh_display_cb_ = std::forward(refresh_cb); + hwc2_callback_data_t refresh_callback_data_ = NULL; + HWC2_PFN_REFRESH refresh_callback_hook_ = NULL; + std::mutex refresh_callback_lock; + + void SetRefreshCallback(hwc2_callback_data_t data, + hwc2_function_pointer_t hook) { + const std::lock_guard lock(refresh_callback_lock); + refresh_callback_data_ = data; + refresh_callback_hook_ = reinterpret_cast(hook); } std::unique_ptr CreateComposition() const; diff --git a/drm/VSyncWorker.cpp b/drm/VSyncWorker.cpp index dfbf8ce..b2f7e5f 100644 --- a/drm/VSyncWorker.cpp +++ b/drm/VSyncWorker.cpp @@ -50,6 +50,14 @@ void VSyncWorker::RegisterCallback(std::shared_ptr callback) { Unlock(); } +void VSyncWorker::RegisterClientCallback(hwc2_callback_data_t data, + hwc2_function_pointer_t hook) { + Lock(); + vsync_callback_data_ = data; + vsync_callback_hook_ = reinterpret_cast(hook); + Unlock(); +} + void VSyncWorker::VSyncControl(bool enabled) { Lock(); enabled_ = enabled; @@ -151,37 +159,17 @@ void VSyncWorker::Routine() { (int64_t)vblank.reply.tval_usec * 1000; } - /* - * VSync could be disabled during routine execution so it could potentially - * lead to crash since callback's inner hook could be invalid anymore. We have - * no control over lifetime of this hook, therefore we can't rely that it'll - * be valid after vsync disabling. - * - * Blocking VSyncControl to wait until routine - * will finish execution is logically correct way to fix this issue, but it - * creates visible lags and stutters, so we have to resort to other ways of - * mitigating this issue. - * - * Doing check before attempt to invoke callback drastically shortens the - * window when such situation could happen and that allows us to practically - * avoid this issue. - * - * Please note that issue described below is different one and it is related - * to RegisterCallback, not to disabling vsync via VSyncControl. - */ if (!enabled_) return; - /* - * There's a race here where a change in callback_ will not take effect until - * the next subsequent requested vsync. This is unavoidable since we can't - * call the vsync hook while holding the thread lock. - * - * We could shorten the race window by caching callback_ right before calling - * the hook. However, in practice, callback_ is only updated once, so it's not - * worth the overhead. - */ + if (callback) callback->Callback(display, timestamp); + + Lock(); + if (enabled_ && vsync_callback_hook_ && vsync_callback_data_) + vsync_callback_hook_(vsync_callback_data_, display, timestamp); + Unlock(); + last_timestamp_ = timestamp; } } // namespace android diff --git a/drm/VSyncWorker.h b/drm/VSyncWorker.h index 0121a6c..7454b51 100644 --- a/drm/VSyncWorker.h +++ b/drm/VSyncWorker.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -42,6 +43,8 @@ class VSyncWorker : public Worker { int Init(DrmDevice *drm, int display); void RegisterCallback(std::shared_ptr callback); + void RegisterClientCallback(hwc2_callback_data_t data, + hwc2_function_pointer_t hook); void VSyncControl(bool enabled); @@ -62,6 +65,9 @@ class VSyncWorker : public Worker { int display_; std::atomic_bool enabled_; int64_t last_timestamp_; + + hwc2_callback_data_t vsync_callback_data_ = NULL; + HWC2_PFN_VSYNC vsync_callback_hook_ = NULL; }; } // namespace android -- 2.11.0