From f4b671f0f6d3130b1e8a6e264d2d15f241164ce0 Mon Sep 17 00:00:00 2001 From: Hiroyuki Ishii Date: Tue, 15 Jun 2021 11:12:34 +0900 Subject: [PATCH] qtwayland: Fix timer leak in QWaylandWindow With long-running qt applications which have much animations (like refgui app in ic-eg's cluster demo), it becomes very sluggish due to a timer leak issue which already have been reported to the qt project: https://bugreports.qt.io/browse/QTBUG-79838 Although this issue is not fixed yet in upstream, we've confirmed that the patch attached to the ticket works fine, so let's apply it. Signed-off-by: Hiroyuki Ishii --- ...x-100ms-freeze-when-applications-do-.patch | 121 ++++++++++++++++++ ...-Fix-timer-leak-and-a-potential-race.patch | 93 ++++++++++++++ recipes-qt/qt5/qtwayland_git.bb | 6 + 3 files changed, 220 insertions(+) create mode 100644 recipes-qt/qt5/qtwayland/0001-Revert-Client-Fix-100ms-freeze-when-applications-do-.patch create mode 100644 recipes-qt/qt5/qtwayland/0002-Fix-timer-leak-and-a-potential-race.patch diff --git a/recipes-qt/qt5/qtwayland/0001-Revert-Client-Fix-100ms-freeze-when-applications-do-.patch b/recipes-qt/qt5/qtwayland/0001-Revert-Client-Fix-100ms-freeze-when-applications-do-.patch new file mode 100644 index 0000000..a269fdb --- /dev/null +++ b/recipes-qt/qt5/qtwayland/0001-Revert-Client-Fix-100ms-freeze-when-applications-do-.patch @@ -0,0 +1,121 @@ +From 8e75b9403c90c15bb2b6466cecd4f77f038a95f5 Mon Sep 17 00:00:00 2001 +From: Hiroyuki Ishii +Date: Tue, 15 Jun 2021 10:19:56 +0900 +Subject: [PATCH] Revert "Client: Fix 100ms freeze when applications do not + swap after deliverUpdateRequest" + +This reverts commit 9f5b96225885f927727a57b6123d8550d6c373bb. +--- + src/client/qwaylandwindow.cpp | 46 ++++++++++++++++++++++++++++------- + src/client/qwaylandwindow_p.h | 1 + + 2 files changed, 38 insertions(+), 9 deletions(-) + +diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp +index 0df99d9f..919ab3ca 100644 +--- a/src/client/qwaylandwindow.cpp ++++ b/src/client/qwaylandwindow.cpp +@@ -1081,6 +1081,25 @@ QVariant QWaylandWindow::property(const QString &name, const QVariant &defaultVa + + void QWaylandWindow::timerEvent(QTimerEvent *event) + { ++ if (event->timerId() == mFallbackUpdateTimerId) { ++ killTimer(mFallbackUpdateTimerId); ++ mFallbackUpdateTimerId = -1; ++ qCDebug(lcWaylandBackingstore) << "mFallbackUpdateTimer timed out"; ++ ++ if (!isExposed()) { ++ qCDebug(lcWaylandBackingstore) << "Fallback update timer: Window not exposed," ++ << "not delivering update request."; ++ return; ++ } ++ ++ if (mWaitingForUpdate && hasPendingUpdateRequest() && !mWaitingForFrameCallback) { ++ qCWarning(lcWaylandBackingstore) << "Delivering update request through fallback timer," ++ << "may not be in sync with display"; ++ deliverUpdateRequest(); ++ } ++ } ++ ++ + if (mFrameCallbackTimerId.testAndSetOrdered(event->timerId(), -1)) { + killTimer(event->timerId()); + qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed"; +@@ -1092,7 +1111,6 @@ void QWaylandWindow::timerEvent(QTimerEvent *event) + + void QWaylandWindow::requestUpdate() + { +- qCDebug(lcWaylandBackingstore) << "requestUpdate"; + Q_ASSERT(hasPendingUpdateRequest()); // should be set by QPA + + // If we have a frame callback all is good and will be taken care of there +@@ -1100,17 +1118,20 @@ void QWaylandWindow::requestUpdate() + return; + + // If we've already called deliverUpdateRequest(), but haven't seen any attach+commit/swap yet +- // This is a somewhat redundant behavior and might indicate a bug in the calling code, so log +- // here so we can get this information when debugging update/frame callback issues. +- // Continue as nothing happened, though. +- if (mWaitingForUpdate) +- qCDebug(lcWaylandBackingstore) << "requestUpdate called twice without committing anything"; ++ if (mWaitingForUpdate) { ++ // Ideally, we should just have returned here, but we're not guaranteed that the client ++ // will actually update, so start this timer to deliver another request update after a while ++ // *IF* the client doesn't update. ++ int fallbackTimeout = 100; ++ mFallbackUpdateTimerId = startTimer(fallbackTimeout); ++ return; ++ } + + // Some applications (such as Qt Quick) depend on updates being delivered asynchronously, + // so use invokeMethod to delay the delivery a bit. + QMetaObject::invokeMethod(this, [this] { + // Things might have changed in the meantime +- if (hasPendingUpdateRequest() && !mWaitingForFrameCallback) ++ if (hasPendingUpdateRequest() && !mWaitingForUpdate && !mWaitingForFrameCallback) + deliverUpdateRequest(); + }, Qt::QueuedConnection); + } +@@ -1120,7 +1141,6 @@ void QWaylandWindow::requestUpdate() + // Can be called from the render thread (without locking anything) so make sure to not make races in this method. + void QWaylandWindow::handleUpdate() + { +- qCDebug(lcWaylandBackingstore) << "handleUpdate" << QThread::currentThread(); + // TODO: Should sync subsurfaces avoid requesting frame callbacks? + QReadLocker lock(&mSurfaceLock); + if (!mSurface) +@@ -1131,6 +1151,15 @@ void QWaylandWindow::handleUpdate() + mFrameCallback = nullptr; + } + ++ if (mFallbackUpdateTimerId != -1) { ++ // Ideally, we would stop the fallback timer here, but since we're on another thread, ++ // it's not allowed. Instead we set mFallbackUpdateTimer to -1 here, so we'll just ++ // ignore it if it times out before it's cleaned up by the invokeMethod call. ++ int id = mFallbackUpdateTimerId; ++ mFallbackUpdateTimerId = -1; ++ QMetaObject::invokeMethod(this, [this, id] { killTimer(id); }, Qt::QueuedConnection); ++ } ++ + mFrameCallback = mSurface->frame(); + wl_callback_add_listener(mFrameCallback, &QWaylandWindow::callbackListener, this); + mWaitingForFrameCallback = true; +@@ -1150,7 +1179,6 @@ void QWaylandWindow::handleUpdate() + + void QWaylandWindow::deliverUpdateRequest() + { +- qCDebug(lcWaylandBackingstore) << "deliverUpdateRequest"; + mWaitingForUpdate = true; + QPlatformWindow::deliverUpdateRequest(); + } +diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h +index 5f15ca30..52e57c72 100644 +--- a/src/client/qwaylandwindow_p.h ++++ b/src/client/qwaylandwindow_p.h +@@ -229,6 +229,7 @@ protected: + + // True when we have called deliverRequestUpdate, but the client has not yet attached a new buffer + bool mWaitingForUpdate = false; ++ int mFallbackUpdateTimerId = -1; // Started when waiting for app to commit + + QMutex mResizeLock; + bool mWaitingToApplyConfigure = false; diff --git a/recipes-qt/qt5/qtwayland/0002-Fix-timer-leak-and-a-potential-race.patch b/recipes-qt/qt5/qtwayland/0002-Fix-timer-leak-and-a-potential-race.patch new file mode 100644 index 0000000..55b1202 --- /dev/null +++ b/recipes-qt/qt5/qtwayland/0002-Fix-timer-leak-and-a-potential-race.patch @@ -0,0 +1,93 @@ +From c41842607c557118f4c3128e3e4c18ef675ebf80 Mon Sep 17 00:00:00 2001 +From: Simon Yuan +Date: Thu, 7 Nov 2019 09:22:37 +1300 +Subject: [PATCH] Fix timer leak and a potential race + +The callback timer is now killed immediately before starting a new timer, this +makes sure there is always a single active callback timer. It's unclear why +killing the timer in a separate lambda doesn't always kill the timer in time, +the hypothesis is that if killing the timer comes after starting a new one, then +the previous timer is now left dangling. Whatever the reason is, it makes even +more sense to kill the timer in the same lamda and immediately before starting a +new timer anyway. + +Another improvement which may also be contributing to fixing the timer leak is +changing the fallback update timer to a QAtomicInt as well. +--- + src/client/qwaylandwindow.cpp | 24 ++++++++++++------------ + src/client/qwaylandwindow_p.h | 2 +- + 2 files changed, 13 insertions(+), 13 deletions(-) + +diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp +index 919ab3ca..dd303b3b 100644 +--- a/src/client/qwaylandwindow.cpp ++++ b/src/client/qwaylandwindow.cpp +@@ -1081,9 +1081,8 @@ QVariant QWaylandWindow::property(const QString &name, const QVariant &defaultVa + + void QWaylandWindow::timerEvent(QTimerEvent *event) + { +- if (event->timerId() == mFallbackUpdateTimerId) { +- killTimer(mFallbackUpdateTimerId); +- mFallbackUpdateTimerId = -1; ++ if (mFallbackUpdateTimerId.testAndSetOrdered(event->timerId(), -1)) { ++ killTimer(event->timerId()); + qCDebug(lcWaylandBackingstore) << "mFallbackUpdateTimer timed out"; + + if (!isExposed()) { +@@ -1123,6 +1122,9 @@ void QWaylandWindow::requestUpdate() + // will actually update, so start this timer to deliver another request update after a while + // *IF* the client doesn't update. + int fallbackTimeout = 100; ++ int fbuId = mFallbackUpdateTimerId.fetchAndStoreOrdered(-1); ++ if (fbuId != -1) ++ killTimer(fbuId); + mFallbackUpdateTimerId = startTimer(fallbackTimeout); + return; + } +@@ -1151,12 +1153,11 @@ void QWaylandWindow::handleUpdate() + mFrameCallback = nullptr; + } + +- if (mFallbackUpdateTimerId != -1) { ++ int id = mFallbackUpdateTimerId.fetchAndStoreOrdered(-1); ++ if (id != -1) { + // Ideally, we would stop the fallback timer here, but since we're on another thread, + // it's not allowed. Instead we set mFallbackUpdateTimer to -1 here, so we'll just + // ignore it if it times out before it's cleaned up by the invokeMethod call. +- int id = mFallbackUpdateTimerId; +- mFallbackUpdateTimerId = -1; + QMetaObject::invokeMethod(this, [this, id] { killTimer(id); }, Qt::QueuedConnection); + } + +@@ -1165,13 +1166,12 @@ void QWaylandWindow::handleUpdate() + mWaitingForFrameCallback = true; + mWaitingForUpdate = false; + +- // Stop current frame timer if any, can't use killTimer directly, see comment above. +- int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); +- if (fcbId != -1) +- QMetaObject::invokeMethod(this, [this, fcbId] { killTimer(fcbId); }, Qt::QueuedConnection); +- + // Start a timer for handling the case when the compositor stops sending frame callbacks. +- QMetaObject::invokeMethod(this, [this] { // Again; can't do it directly ++ // Can't use killTimer directly, see comment above. ++ QMetaObject::invokeMethod(this, [this] { ++ int fcbId = mFrameCallbackTimerId.fetchAndStoreOrdered(-1); ++ if (fcbId != -1) ++ killTimer(fcbId); + if (mWaitingForFrameCallback) + mFrameCallbackTimerId = startTimer(100); + }, Qt::QueuedConnection); +diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h +index 52e57c72..7e40289b 100644 +--- a/src/client/qwaylandwindow_p.h ++++ b/src/client/qwaylandwindow_p.h +@@ -229,7 +229,7 @@ protected: + + // True when we have called deliverRequestUpdate, but the client has not yet attached a new buffer + bool mWaitingForUpdate = false; +- int mFallbackUpdateTimerId = -1; // Started when waiting for app to commit ++ QAtomicInt mFallbackUpdateTimerId = -1; // Started when waiting for app to commit + + QMutex mResizeLock; + bool mWaitingToApplyConfigure = false; diff --git a/recipes-qt/qt5/qtwayland_git.bb b/recipes-qt/qt5/qtwayland_git.bb index fbd1b6f..990b5d6 100644 --- a/recipes-qt/qt5/qtwayland_git.bb +++ b/recipes-qt/qt5/qtwayland_git.bb @@ -17,6 +17,12 @@ LIC_FILES_CHKSUM = " \ # 5.14.meta-qt5.1 SRC_URI += "file://0001-tst_seatv4-Include-array.patch" +# Patch reported in https://bugreports.qt.io/browse/QTBUG-79838, not upstreamed yet +SRC_URI += " \ + file://0001-Revert-Client-Fix-100ms-freeze-when-applications-do-.patch \ + file://0002-Fix-timer-leak-and-a-potential-race.patch \ + " + PACKAGECONFIG ?= " \ wayland-client \ wayland-server \ -- 2.25.1