Differential D3132 Diff 7638 plugins/platforms/x11/standalone/x11_platform.cpp

Changeset View

Standalone View

View Options

plugins/platforms/x11/standalone/x11_platform.cpp

Show First 20 Lines • Show All 197 Lines • ▼ Show 20 Line(s)
198{ 198​{
199 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()));
200 auto group = KConfigGroup(kwinApp()->config(), "Compositing"); 200​ auto group = KConfigGroup(kwinApp()->config(), "Compositing");
201 switch (safePoint) { 201​ switch (safePoint) {
202 case OpenGLSafePoint::PreInit: 202​ case OpenGLSafePoint::PreInit:
203 group.writeEntry(unsafeKey, true); 203​ group.writeEntry(unsafeKey, true);
204 group.sync(); 204​ group.sync();
205 // Deliberately continue with PreFrame 205​ // Deliberately continue with PreFrame
206 case OpenGLSafePoint::PreFrame: 206​ case OpenGLSafePoint::PreFrame:
luebkingUnsubmitted
Done

PreFrame is (now) effectively "PreFirstGuardedFrame", is it?
And if invoked at some later point would create the timer and hit the config rewrite every single frame (for the counter is stuck at 0)?

> rename to avoid bad invocation?

luebking: PreFrame is (now) effectively "PreFirstGuardedFrame", is it? And if invoked at some later point…
antlarrAuthorUnsubmitted
Done

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…
207 if (m_openGLFreezeProtectionThread == nullptr) { 207​ if (m_openGLFreezeProtectionThread == nullptr) {
208 Q_ASSERT(m_openGLFreezeProtection == nullptr); 208​ Q_ASSERT(m_openGLFreezeProtection == nullptr);
209 m_openGLFreezeProtectionThread = new QThread(this); 209​ m_openGLFreezeProtectionThread = new QThread(this);
210 m_openGLFreezeProtectionThread->setObjectName("FreezeDetector"); 210​ m_openGLFreezeProtectionThread->setObjectName("FreezeDetector");
211 m_openGLFreezeProtectionThread->start(); 211​ m_openGLFreezeProtectionThread->start();
212 m_openGLFreezeProtection = new QTimer; 212​ m_openGLFreezeProtection = new QTimer;
luebkingUnsubmitted
Done

Why do you recreate it with every frame?
Only create it PreFirstFrame and delete it PostLastFrame (and inbeteween simply restart the pointer PreFrame)

luebking: Why do you recreate it with every frame? Only create it PreFirstFrame and delete it…
antlarrAuthorUnsubmitted
Done

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.
luebkingUnsubmitted
Done

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…
antlarrAuthorUnsubmitted
Done

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? :)
luebkingUnsubmitted
Done

No, why? The purpose is to push the freeze timer forward as long as the GUI thread isn't blocked.
If the GUI thread is blocked, the forward pushing won't happen, the timer times out and set GL unsafe etc.

Do I miss something?

luebking: No, why? The purpose is to push the freeze timer forward as long as the GUI thread isn't…
graesslinUnsubmitted
Done

You can use QMetaObject::invokeMethod with Qt::QueuedConnection.

graesslin: You can use QMetaObject::invokeMethod with Qt::QueuedConnection.
antlarrAuthorUnsubmitted
Done

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…
213 m_openGLFreezeProtection->setInterval(15000); 213​ m_openGLFreezeProtection->setInterval(15000);
214 m_openGLFreezeProtection->setSingleShot(true); 214​ m_openGLFreezeProtection->setSingleShot(true);
215 m_openGLFreezeProtection->start(); 215​ m_openGLFreezeProtection->start();
216 m_openGLFreezeProtection->moveToThread(m_openGLFreezeProtectionThread); 216​ m_openGLFreezeProtection->moveToThread(m_openGLFreezeProtectionThread);
217 connect(m_openGLFreezeProtection, &QTimer::timeout, m_openGLFreezeProtection, 217​ connect(m_openGLFreezeProtection, &QTimer::timeout, m_openGLFreezeProtection,
218 [] { 218​ [] {
219 const QString unsafeKey(QLatin1String("OpenGLIsUnsafe") + (kwinApp()->isX11MultiHead() ? QString::number(kwinApp()->x11ScreenNumber()) : QString())); 219​ const QString unsafeKey(QLatin1String("OpenGLIsUnsafe") + (kwinApp()->isX11MultiHead() ? QString::number(kwinApp()->x11ScreenNumber()) : QString()));
220 auto group = KConfigGroup(kwinApp()->config(), "Compositing"); 220​ auto group = KConfigGroup(kwinApp()->config(), "Compositing");
221 group.writeEntry(unsafeKey, true); 221​ group.writeEntry(unsafeKey, true);
luebkingUnsubmitted
Done

same goes for config writing

luebking: same goes for config writing
antlarrAuthorUnsubmitted
Done

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…
luebkingUnsubmitted
Done

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 ;-)
antlarrAuthorUnsubmitted
Done

no problem :)

antlarr: no problem :)
222 group.sync(); 222​ group.sync();
223 qFatal("Freeze in OpenGL initialization detected"); 223​ qFatal("Freeze in OpenGL initialization detected");
224 }, Qt::DirectConnection); 224​ }, Qt::DirectConnection);
225 } 225​ } else {
226 else ​
227 { ​
228 Q_ASSERT(m_openGLFreezeProtection); 226​ Q_ASSERT(m_openGLFreezeProtection);
229 QMetaObject::invokeMethod(m_openGLFreezeProtection, "start", Qt::QueuedConnection); 227​ QMetaObject::invokeMethod(m_openGLFreezeProtection, "start", Qt::QueuedConnection);
graesslinUnsubmitted
Done

nitpick: coding style

} else {
graesslin: nitpick: coding style } else {
230 } 228​ }
231 break; 229​ break;
232 case OpenGLSafePoint::PostInit: 230​ case OpenGLSafePoint::PostInit:
233 group.writeEntry(unsafeKey, false); 231​ group.writeEntry(unsafeKey, false);
234 group.sync(); 232​ group.sync();
235 // Deliberately continue with PostFrame 233​ // Deliberately continue with PostFrame
236 case OpenGLSafePoint::PostFrame: 234​ case OpenGLSafePoint::PostFrame:
237 QMetaObject::invokeMethod(m_openGLFreezeProtection, "stop", Qt::QueuedConnection); 235​ QMetaObject::invokeMethod(m_openGLFreezeProtection, "stop", Qt::QueuedConnection);
238 break; 236​ break;
239 case OpenGLSafePoint::PostLastGuardedFrame: 237​ case OpenGLSafePoint::PostLastGuardedFrame:
240 m_openGLFreezeProtection->deleteLater(); 238​ m_openGLFreezeProtection->deleteLater();
241 m_openGLFreezeProtection = nullptr; 239​ m_openGLFreezeProtection = nullptr;
242 m_openGLFreezeProtectionThread->quit(); 240​ m_openGLFreezeProtectionThread->quit();
graesslinUnsubmitted
Done

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…
antlarrAuthorUnsubmitted
Done

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…
graesslinUnsubmitted
Done

yep, agree.

graesslin: yep, agree.
243 m_openGLFreezeProtectionThread->wait(); 241​ m_openGLFreezeProtectionThread->wait();
244 delete m_openGLFreezeProtectionThread; 242​ delete m_openGLFreezeProtectionThread;
245 m_openGLFreezeProtectionThread = nullptr; 243​ m_openGLFreezeProtectionThread = nullptr;
246 break; 244​ break;
247 } 245​ }
248} 246​}
249 247​
250} 248​}