Changeset View
Standalone View
plugins/platforms/x11/standalone/x11_platform.cpp
| Show All 31 Lines | |||||
| 32 | #include "keyboard_input.h" | 32 | #include "keyboard_input.h" | ||
| 33 | #include "logging.h" | 33 | #include "logging.h" | ||
| 34 | #include "screens_xrandr.h" | 34 | #include "screens_xrandr.h" | ||
| 35 | #include "options.h" | 35 | #include "options.h" | ||
| 36 | 36 | | |||
| 37 | #include <KConfigGroup> | 37 | #include <KConfigGroup> | ||
| 38 | #include <KLocalizedString> | 38 | #include <KLocalizedString> | ||
| 39 | 39 | | |||
| 40 | #include <QThread> | ||||
| 40 | #include <QOpenGLContext> | 41 | #include <QOpenGLContext> | ||
| 41 | #include <QX11Info> | 42 | #include <QX11Info> | ||
| 42 | 43 | | |||
| 43 | namespace KWin | 44 | namespace KWin | ||
| 44 | { | 45 | { | ||
| 45 | 46 | | |||
| 46 | X11StandalonePlatform::X11StandalonePlatform(QObject *parent) | 47 | X11StandalonePlatform::X11StandalonePlatform(QObject *parent) | ||
| 47 | : Platform(parent) | 48 | : Platform(parent) | ||
| ▲ Show 20 Lines • Show All 145 Lines • ▼ Show 20 Line(s) | 193 | { | |||
| 193 | return Xcb::Extensions::self()->hasGlx(); | 194 | return Xcb::Extensions::self()->hasGlx(); | ||
| 194 | } | 195 | } | ||
| 195 | 196 | | |||
| 196 | void X11StandalonePlatform::createOpenGLSafePoint(OpenGLSafePoint safePoint) | 197 | void X11StandalonePlatform::createOpenGLSafePoint(OpenGLSafePoint safePoint) | ||
| 197 | { | 198 | { | ||
| 198 | const QString unsafeKey(QLatin1String("OpenGLIsUnsafe") + (kwinApp()->isX11MultiHead() ? QString::number(kwinApp()->x11ScreenNumber()) : QString())); | 199 | const QString unsafeKey(QLatin1String("OpenGLIsUnsafe") + (kwinApp()->isX11MultiHead() ? QString::number(kwinApp()->x11ScreenNumber()) : QString())); | ||
| 199 | auto group = KConfigGroup(kwinApp()->config(), "Compositing"); | 200 | auto group = KConfigGroup(kwinApp()->config(), "Compositing"); | ||
| 200 | switch (safePoint) { | 201 | switch (safePoint) { | ||
| 201 | case OpenGLSafePoint::PreInit: | 202 | case OpenGLSafePoint::PreInit: | ||
| 203 | case OpenGLSafePoint::PreFrame: | ||||
| 204 | Q_ASSERT(m_openGLFreezeProtectionThread == nullptr); | ||||
| 205 | m_openGLFreezeProtectionThread = new QThread(this); | ||||
luebking: same goes for config writing | |||||
The config is not written in every frame. It's only saved when the timer is triggered. That is, when a freeze is detected. That was the point of the "remove overhead" commit. antlarr: The config is not written in every frame. It's only saved when the timer is triggered. That is… | |||||
Sorry, misread. (Actually didn't really read and just wanted to point out the other case ;-) luebking: Sorry, misread. (Actually didn't really read and just wanted to point out the other case ;-) | |||||
antlarr: no problem :) | |||||
| 206 | m_openGLFreezeProtectionThread->start(); | ||||
| 207 | m_openGLFreezeProtection = new QTimer; | ||||
| 208 | m_openGLFreezeProtection->setInterval(15000); | ||||
| 209 | m_openGLFreezeProtection->setSingleShot(true); | ||||
| 210 | m_openGLFreezeProtection->start(); | ||||
| 211 | m_openGLFreezeProtection->moveToThread(m_openGLFreezeProtectionThread); | ||||
| 212 | connect(m_openGLFreezeProtection, &QTimer::timeout, m_openGLFreezeProtection, | ||||
| 213 | [] { | ||||
| 214 | qFatal("Freeze in OpenGL initialization detected"); | ||||
| 215 | }, Qt::DirectConnection); | ||||
| 216 | | ||||
| 202 | group.writeEntry(unsafeKey, true); | 217 | group.writeEntry(unsafeKey, true); | ||
Why do you recreate it with every frame? luebking: Why do you recreate it with every frame?
Only create it PreFirstFrame and delete it… | |||||
The timer is in another thread, so it can't be stopped/restarted from this thread. antlarr: The timer is in another thread, so it can't be stopped/restarted from this thread. | |||||
You should be able to call it as slot (whether QMetaObject::invokeMethod() works, I've never tried) luebking: You should be able to call it as slot (whether QMetaObject::invokeMethod() works, I've never… | |||||
yeah, but then it wouldn't be a synchronous call and it would lose all its meaning, isn't it? :) antlarr: yeah, but then it wouldn't be a synchronous call and it would lose all its meaning, isn't it? :) | |||||
No, why? The purpose is to push the freeze timer forward as long as the GUI thread isn't blocked. Do I miss something? luebking: No, why? The purpose is to push the freeze timer forward as long as the GUI thread isn't… | |||||
graesslin: You can use QMetaObject::invokeMethod with Qt::QueuedConnection. | |||||
Oh, I thought QueuedConnection needed to get to the event loop to execute the slot method, but I just noticed it's the event loop of the receiver object, so it would be fine. I'll change that. antlarr: Oh, I thought QueuedConnection needed to get to the event loop to execute the slot method, but… | |||||
| 203 | break; | 218 | break; | ||
| 204 | case OpenGLSafePoint::PostInit: | 219 | case OpenGLSafePoint::PostInit: | ||
| 220 | case OpenGLSafePoint::PostFrame: | ||||
PreFrame is (now) effectively "PreFirstGuardedFrame", is it? > rename to avoid bad invocation?luebking: PreFrame is (now) effectively "PreFirstGuardedFrame", is it?
And if invoked at some later point… | |||||
Not really, it's expected to be called many times and it checks if it's the first time it was called or not (also, when the counter gets to 0 it's never called anymore) antlarr: Not really, it's expected to be called many times and it checks if it's the first time it was… | |||||
| 221 | m_openGLFreezeProtection->deleteLater(); | ||||
| 222 | m_openGLFreezeProtectionThread->quit(); | ||||
| 223 | m_openGLFreezeProtectionThread->wait(); | ||||
| 224 | delete m_openGLFreezeProtectionThread; | ||||
| 225 | m_openGLFreezeProtectionThread = nullptr; | ||||
| 226 | m_openGLFreezeProtection = nullptr; | ||||
| 205 | group.writeEntry(unsafeKey, false); | 227 | group.writeEntry(unsafeKey, false); | ||
I think you can merge the PostFrame with PostInit. So that the init test also does the freeze testing. That would also solve the conceptual problem I pointed out above. graesslin: I think you can merge the PostFrame with PostInit. So that the init test also does the freeze… | |||||
Do you prefer to merge PostInit with PostFrame or with PostLastGuardedFrame? The difference would be that if it's merged with PostFrame and a user sets KWIN_MAX_FRAMES_TESTED to 0 (so no freeze detection is done when painting frames) then the detection thread and timer wouldn't be deleted and if it's merged with PostLastGuardedFrame, then a new thread and timer will be created for the frame drawing. I would merge it with PostFrame, since it requires a user who probably knows what he's doing to interact and is more optimized for 99.9999% of cases, and still an idle thread and stopped timer shouldn't consume any resources in the rare case a user sets that, but I ask just in case. antlarr: Do you prefer to merge PostInit with PostFrame or with PostLastGuardedFrame? The difference… | |||||
graesslin: yep, agree. | |||||
| 206 | break; | 228 | break; | ||
| 207 | } | 229 | } | ||
| 208 | group.sync(); | 230 | group.sync(); | ||
| 209 | } | 231 | } | ||
| 210 | 232 | | |||
| 211 | } | 233 | } | ||
graesslin: nitpick: coding style
} else { | |||||
same goes for config writing