From d1de19e212dc49fb0781aa6fd345b2ceb34d2f43 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Mon, 5 Sep 2022 10:31:57 +0300 Subject: [PATCH] Make Workspace process batched output updates Currently the Workspace processes output updates as they occur, e.g. when the drm backend scans connectors, the Workspace will handle hotplugged outputs one by one or if an output configuration changes the mode of several outputs, the workspace will process output layout updates one by one instead of handling it in one pass. The main reason for the current behavior is simplicity. However, that can create issues because it's possible that the output layout will be temporarily in degenerate state and features such as sticking windows to their outputs will be broken. In order to fix that, this change makes the Workspace process batched output updates. There are several challenges - disconnected outputs have to be alive when the outputsQueried signal is emitted, the workspace needs to determine what outputs have been added or removed on its own. --- autotests/integration/outputchanges_test.cpp | 6 +- .../integration/xdgshellwindow_rules_test.cpp | 4 +- src/backends/drm/drm_gpu.cpp | 4 +- src/backends/virtual/virtual_backend.cpp | 15 +- src/backends/virtual/virtual_backend.h | 1 - src/backends/wayland/wayland_backend.cpp | 2 +- .../standalone/x11_standalone_platform.cpp | 2 +- .../x11/windowed/x11_windowed_backend.cpp | 2 +- src/core/output.cpp | 14 ++ src/core/output.h | 4 + src/screens.cpp | 2 +- src/wayland/outputmanagement_v2_interface.cpp | 4 +- src/workspace.cpp | 145 ++++++++---------- src/workspace.h | 17 +- 14 files changed, 107 insertions(+), 115 deletions(-) diff --git a/autotests/integration/outputchanges_test.cpp b/autotests/integration/outputchanges_test.cpp index dd558822c3..7f2d95e626 100644 --- a/autotests/integration/outputchanges_test.cpp +++ b/autotests/integration/outputchanges_test.cpp @@ -87,7 +87,7 @@ void OutputChangesTest::testWindowSticksToOutputAfterOutputIsDisabled() auto changeSet = config.changeSet(outputs[0]); changeSet->enabled = false; } - kwinApp()->platform()->applyOutputChanges(config); + workspace()->applyOutputConfiguration(config); // The window will be sent to the second output, which is at (1280, 0). QCOMPARE(window->frameGeometry(), QRect(1280 + 42, 0 + 67, 100, 50)); @@ -117,7 +117,7 @@ void OutputChangesTest::testWindowSticksToOutputAfterAnotherOutputIsDisabled() auto changeSet = config.changeSet(outputs[1]); changeSet->pos = QPoint(0, 0); } - kwinApp()->platform()->applyOutputChanges(config); + workspace()->applyOutputConfiguration(config); // The position of the window relative to its output should remain the same. QCOMPARE(window->frameGeometry(), QRect(42, 67, 100, 50)); @@ -143,7 +143,7 @@ void OutputChangesTest::testWindowSticksToOutputAfterOutputIsMoved() auto changeSet = config.changeSet(outputs[0]); changeSet->pos = QPoint(-10, 20); } - kwinApp()->platform()->applyOutputChanges(config); + workspace()->applyOutputConfiguration(config); // The position of the window relative to its output should remain the same. QCOMPARE(window->frameGeometry(), QRect(-10 + 42, 20 + 67, 100, 50)); diff --git a/autotests/integration/xdgshellwindow_rules_test.cpp b/autotests/integration/xdgshellwindow_rules_test.cpp index ba679f39d2..c3fbc96418 100644 --- a/autotests/integration/xdgshellwindow_rules_test.cpp +++ b/autotests/integration/xdgshellwindow_rules_test.cpp @@ -2910,14 +2910,14 @@ void TestXdgShellWindowRules::testScreenForce() OutputConfiguration config; auto changeSet = config.changeSet(outputs.at(1)); changeSet->enabled = false; - kwinApp()->platform()->applyOutputChanges(config); + workspace()->applyOutputConfiguration(config); QVERIFY(!outputs.at(1)->isEnabled()); QCOMPARE(m_window->output()->name(), outputs.at(0)->name()); // Enable the output and check that the window is moved there again changeSet->enabled = true; - kwinApp()->platform()->applyOutputChanges(config); + workspace()->applyOutputConfiguration(config); QVERIFY(outputs.at(1)->isEnabled()); QCOMPARE(m_window->output()->name(), outputs.at(1)->name()); diff --git a/src/backends/drm/drm_gpu.cpp b/src/backends/drm/drm_gpu.cpp index 1608e35b8e..22b7b1291f 100644 --- a/src/backends/drm/drm_gpu.cpp +++ b/src/backends/drm/drm_gpu.cpp @@ -578,7 +578,7 @@ void DrmGpu::removeOutput(DrmOutput *output) output->pipeline()->setLayers(nullptr, nullptr); m_drmOutputs.removeOne(output); Q_EMIT outputRemoved(output); - delete output; + output->unref(); } DrmBackend *DrmGpu::platform() const @@ -603,7 +603,7 @@ void DrmGpu::removeVirtualOutput(DrmVirtualOutput *output) { if (m_virtualOutputs.removeOne(output)) { Q_EMIT outputRemoved(output); - delete output; + output->unref(); } } diff --git a/src/backends/virtual/virtual_backend.cpp b/src/backends/virtual/virtual_backend.cpp index 3c91a0eb28..8bbab6353c 100644 --- a/src/backends/virtual/virtual_backend.cpp +++ b/src/backends/virtual/virtual_backend.cpp @@ -115,25 +115,12 @@ void VirtualBackend::setVirtualOutputs(int count, QVector geometries, QVe output->updateEnabled(false); m_outputs.removeOne(output); Q_EMIT outputRemoved(output); - delete output; + output->unref(); } Q_EMIT outputsQueried(); } -void VirtualBackend::removeOutput(Output *output) -{ - VirtualOutput *virtualOutput = static_cast(output); - virtualOutput->updateEnabled(false); - - m_outputs.removeOne(virtualOutput); - Q_EMIT outputRemoved(virtualOutput); - - delete virtualOutput; - - Q_EMIT screensQueried(); -} - QImage VirtualBackend::captureOutput(Output *output) const { if (auto backend = qobject_cast(Compositor::self()->backend())) { diff --git a/src/backends/virtual/virtual_backend.h b/src/backends/virtual/virtual_backend.h index 8458acf63c..9ee762ac2e 100644 --- a/src/backends/virtual/virtual_backend.h +++ b/src/backends/virtual/virtual_backend.h @@ -53,7 +53,6 @@ public: return QVector{OpenGLCompositing, QPainterCompositing}; } - Q_INVOKABLE void removeOutput(Output *output); Q_INVOKABLE QImage captureOutput(Output *output) const; Q_SIGNALS: diff --git a/src/backends/wayland/wayland_backend.cpp b/src/backends/wayland/wayland_backend.cpp index 14931dbac9..9bb738049b 100644 --- a/src/backends/wayland/wayland_backend.cpp +++ b/src/backends/wayland/wayland_backend.cpp @@ -978,7 +978,7 @@ void WaylandBackend::removeVirtualOutput(Output *output) if (waylandOutput && m_outputs.removeAll(waylandOutput)) { waylandOutput->updateEnabled(false); Q_EMIT outputRemoved(waylandOutput); - delete waylandOutput; + waylandOutput->unref(); } } diff --git a/src/backends/x11/standalone/x11_standalone_platform.cpp b/src/backends/x11/standalone/x11_standalone_platform.cpp index 538c403d0b..1e1c5bbb9c 100644 --- a/src/backends/x11/standalone/x11_standalone_platform.cpp +++ b/src/backends/x11/standalone/x11_standalone_platform.cpp @@ -606,7 +606,7 @@ void X11StandalonePlatform::doUpdateOutputs() nativeOutput->updateEnabled(false); } Q_EMIT outputRemoved(output); - delete output; + output->unref(); } // Make sure that the position of an output in m_outputs matches its xinerama index, there diff --git a/src/backends/x11/windowed/x11_windowed_backend.cpp b/src/backends/x11/windowed/x11_windowed_backend.cpp index 6fdbe48498..48da756e4f 100644 --- a/src/backends/x11/windowed/x11_windowed_backend.cpp +++ b/src/backends/x11/windowed/x11_windowed_backend.cpp @@ -499,7 +499,7 @@ void X11WindowedBackend::handleClientMessage(xcb_client_message_event_t *event) removedOutput->updateEnabled(false); Q_EMIT outputRemoved(removedOutput); - delete removedOutput; + removedOutput->unref(); Q_EMIT outputsQueried(); } } diff --git a/src/core/output.cpp b/src/core/output.cpp index 0840606b5b..bd875d0eba 100644 --- a/src/core/output.cpp +++ b/src/core/output.cpp @@ -69,6 +69,20 @@ Output::~Output() { } +void Output::ref() +{ + m_refCount++; +} + +void Output::unref() +{ + Q_ASSERT(m_refCount > 0); + m_refCount--; + if (m_refCount == 0) { + delete this; + } +} + QString Output::name() const { return m_information.name; diff --git a/src/core/output.h b/src/core/output.h index 7c4820a1c9..7e9af5833f 100644 --- a/src/core/output.h +++ b/src/core/output.h @@ -94,6 +94,9 @@ public: explicit Output(QObject *parent = nullptr); ~Output() override; + void ref(); + void unref(); + /** * Maps the specified @a rect from the global coordinate system to the output-local coords. */ @@ -328,6 +331,7 @@ protected: Information m_information; QUuid m_uuid; int m_directScanoutCount = 0; + int m_refCount = 1; friend class EffectScreenImpl; // to access m_effectScreen }; diff --git a/src/screens.cpp b/src/screens.cpp index 454c75287d..881023660d 100644 --- a/src/screens.cpp +++ b/src/screens.cpp @@ -24,7 +24,7 @@ namespace KWin Screens::Screens() : m_maxScale(1.0) { - connect(kwinApp()->platform(), &Platform::outputsQueried, this, &Screens::changed); + connect(workspace(), &Workspace::outputsChanged, this, &Screens::changed); } void Screens::init() diff --git a/src/wayland/outputmanagement_v2_interface.cpp b/src/wayland/outputmanagement_v2_interface.cpp index 8d6e96d15a..0c3dc5bb63 100644 --- a/src/wayland/outputmanagement_v2_interface.cpp +++ b/src/wayland/outputmanagement_v2_interface.cpp @@ -13,7 +13,6 @@ #include "core/outputconfiguration.h" #include "core/platform.h" #include "main.h" -#include "screens.h" #include "workspace.h" #include "qwayland-server-kde-output-management-v2.h" @@ -227,14 +226,13 @@ void OutputConfigurationV2Interface::kde_output_configuration_v2_apply(Resource return; } - if (kwinApp()->platform()->applyOutputChanges(config)) { + if (workspace()->applyOutputConfiguration(config)) { if (primaryOutput.has_value()) { auto requestedPrimaryOutput = (*primaryOutput)->handle(); if (requestedPrimaryOutput && requestedPrimaryOutput->isEnabled()) { workspace()->setPrimaryOutput(requestedPrimaryOutput); } } - Q_EMIT workspace()->screens()->changed(); send_applied(); } else { qCDebug(KWIN_CORE) << "Applying config failed"; diff --git a/src/workspace.cpp b/src/workspace.cpp index 0e809c5b6a..86a823b331 100644 --- a/src/workspace.cpp +++ b/src/workspace.cpp @@ -211,19 +211,8 @@ void Workspace::init() connect(options, &Options::separateScreenFocusChanged, m_focusChain.get(), &FocusChain::setSeparateScreenFocus); m_focusChain->setSeparateScreenFocus(options->isSeparateScreenFocus()); - if (waylandServer()) { - updateOutputConfiguration(); - connect(kwinApp()->platform(), &Platform::outputsQueried, this, &Workspace::updateOutputConfiguration); - } - - Platform *platform = kwinApp()->platform(); - connect(platform, &Platform::outputAdded, this, &Workspace::slotPlatformOutputAdded); - connect(platform, &Platform::outputRemoved, this, &Workspace::slotPlatformOutputRemoved); - - const QVector outputs = platform->outputs(); - for (Output *output : outputs) { - slotPlatformOutputAdded(output); - } + slotPlatformOutputsQueried(); + connect(kwinApp()->platform(), &Platform::outputsQueried, this, &Workspace::slotPlatformOutputsQueried); m_screens->init(); @@ -520,6 +509,13 @@ Workspace::~Workspace() m_placement.reset(); delete m_windowKeysDialog; + if (m_placeholderOutput) { + m_placeholderOutput->unref(); + } + for (Output *output : std::as_const(m_outputs)) { + output->unref(); + } + _self = nullptr; } @@ -625,6 +621,15 @@ std::shared_ptr parseMode(Output *output, const QJsonObject &modeInf } } +bool Workspace::applyOutputConfiguration(const OutputConfiguration &config) +{ + if (!kwinApp()->platform()->applyOutputChanges(config)) { + return false; + } + updateOutputs(); + return true; +} + void Workspace::updateOutputConfiguration() { // There's conflict between this code and setVirtualOutputs(), need to adjust the tests. @@ -1409,87 +1414,67 @@ Output *Workspace::outputAt(const QPointF &pos) const return bestOutput; } -void Workspace::slotPlatformOutputAdded(Output *output) +void Workspace::slotPlatformOutputsQueried() { - if (output->isNonDesktop()) { - return; + if (waylandServer()) { + updateOutputConfiguration(); } + updateOutputs(); +} - if (output->isEnabled()) { - addOutput(output); - } +void Workspace::updateOutputs() +{ + const auto availableOutputs = kwinApp()->platform()->outputs(); + const auto oldOutputs = m_outputs; - connect(output, &Output::enabledChanged, this, [this, output]() { - if (output->isEnabled()) { - addOutput(output); - } else { - removeOutput(output); + m_outputs.clear(); + for (Output *output : availableOutputs) { + if (!output->isNonDesktop() && output->isEnabled()) { + m_outputs.append(output); } - }); -} - -void Workspace::slotPlatformOutputRemoved(Output *output) -{ - if (!output->isNonDesktop()) { - removeOutput(output); - } -} - -void Workspace::addOutput(Output *output) -{ - if (!m_activeOutput) { - m_activeOutput = output; - } - if (!m_primaryOutput) { - setPrimaryOutput(output); } - m_outputs.append(output); + // The workspace requires at least one output connected. + if (m_outputs.isEmpty()) { + if (!m_placeholderOutput) { + m_placeholderOutput = new PlaceholderOutput(QSize(16535, 16535), 1); + m_placeholderFilter = std::make_unique(); + input()->prependInputEventFilter(m_placeholderFilter.get()); + } + m_outputs.append(m_placeholderOutput); + } else { + if (m_placeholderOutput) { + m_placeholderOutput->unref(); + m_placeholderOutput = nullptr; + m_placeholderFilter.reset(); + } + } + + if (!m_activeOutput || !m_outputs.contains(m_activeOutput)) { + setActiveOutput(m_outputs[0]); + } + if (!m_primaryOutput || !m_outputs.contains(m_primaryOutput)) { + setPrimaryOutput(m_outputs[0]); + } - connect(output, &Output::geometryChanged, this, &Workspace::desktopResized); desktopResized(); - // Trigger a re-check of output-related rules on all windows - for (Window *window : qAsConst(m_allClients)) { - sendWindowToOutput(window, window->output()); + const QSet oldOutputsSet(oldOutputs.constBegin(), oldOutputs.constEnd()); + const QSet outputsSet(m_outputs.constBegin(), m_outputs.constEnd()); + + const auto added = outputsSet - oldOutputsSet; + for (Output *output : added) { + output->ref(); + Q_EMIT outputAdded(output); } - Q_EMIT outputAdded(output); - - if (m_placeholderOutput) { - m_outputs.removeOne(m_placeholderOutput.get()); - Q_EMIT outputRemoved(m_placeholderOutput.get()); - m_placeholderOutput.reset(); - m_placeholderFilter.reset(); - } -} - -void Workspace::removeOutput(Output *output) -{ - if (!m_outputs.removeOne(output)) { - return; - } - if (m_outputs.empty()) { - // not all parts of KWin handle having no output yet. To prevent crashes, create a placeholder output - m_placeholderOutput = std::make_unique(output->pixelSize(), output->scale()); - m_outputs.append(m_placeholderOutput.get()); - Q_EMIT outputAdded(m_placeholderOutput.get()); - // also prevent accidental inputs while the user has no screen connected - m_placeholderFilter = std::make_unique(); - input()->prependInputEventFilter(m_placeholderFilter.get()); + const auto removed = oldOutputsSet - outputsSet; + for (Output *output : removed) { + Q_EMIT outputRemoved(output); + output->unref(); } - if (m_activeOutput == output) { - m_activeOutput = outputAt(output->geometry().center()); - } - if (m_primaryOutput == output) { - setPrimaryOutput(m_outputs.constFirst()); - } - - disconnect(output, &Output::geometryChanged, this, &Workspace::desktopResized); - desktopResized(); - - Q_EMIT outputRemoved(output); + Q_EMIT outputsChanged(); } void Workspace::slotDesktopAdded(VirtualDesktop *desktop) diff --git a/src/workspace.h b/src/workspace.h index 4c78835719..e0df82bbdd 100644 --- a/src/workspace.h +++ b/src/workspace.h @@ -78,6 +78,7 @@ class Activities; class PlaceholderInputEventFilter; class PlaceholderOutput; class Placement; +class OutputConfiguration; class KWIN_EXPORT Workspace : public QObject { @@ -464,6 +465,12 @@ public: Activities *activities() const; #endif + /** + * Apply the requested output configuration. Note that you must use this function + * instead of Platform::applyOutputChanges(). + */ + bool applyOutputConfiguration(const OutputConfiguration &config); + public Q_SLOTS: void performWindowOperation(KWin::Window *window, Options::WindowOperation op); // Keybindings @@ -543,8 +550,7 @@ private Q_SLOTS: void slotCurrentDesktopChangingCancelled(); void slotDesktopAdded(VirtualDesktop *desktop); void slotDesktopRemoved(VirtualDesktop *desktop); - void slotPlatformOutputAdded(Output *output); - void slotPlatformOutputRemoved(Output *output); + void slotPlatformOutputsQueried(); Q_SIGNALS: /** @@ -574,6 +580,7 @@ Q_SIGNALS: void primaryOutputChanged(); void outputAdded(KWin::Output *); void outputRemoved(KWin::Output *); + void outputsChanged(); /** * This signal is emitted when the stacking order changed, i.e. a window is risen * or lowered @@ -623,9 +630,6 @@ private: Unmanaged *createUnmanaged(xcb_window_t windowId); void addUnmanaged(Unmanaged *c); - void addOutput(Output *output); - void removeOutput(Output *output); - void addWaylandWindow(Window *window); void removeWaylandWindow(Window *window); @@ -639,6 +643,7 @@ private: QString getPlacementTrackerHash(); void updateOutputConfiguration(); + void updateOutputs(); struct Constraint { @@ -757,7 +762,7 @@ private: #endif std::unique_ptr m_placementTracker; - std::unique_ptr m_placeholderOutput; + PlaceholderOutput *m_placeholderOutput = nullptr; std::unique_ptr m_placeholderFilter; private: