[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added a comment. I don't know why, but it seems arc diff doesn't work anymore and I have to use arc diff HEAD~7 to include all 7 commits REPOSITORY rKWIN KWin REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
graesslin added a comment. The latest patch version seems to not include all changes. REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added inline comments. INLINE COMMENTS > luebking wrote in x11_platform.cpp:206 > 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? > = 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) REPOSITORY rKWIN KWin REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
graesslin added inline comments.
INLINE COMMENTS
> composite.cpp:748
> +m_framesToTestForSafety--;
> + if (m_framesToTestForSafety == 0) {
> +
> kwinApp()->platform()->createOpenGLSafePoint(Platform::OpenGLSafePoint::PostLastGuardedFrame);
this misses the
if (m_scene->compositingType() & OpenGLCompositing)
> x11_platform.cpp:225-227
> +}
> +else
> +{
nitpick: coding style
} else {
REPOSITORY
rKWIN KWin
REVISION DETAIL
https://phabricator.kde.org/D3132
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
luebking added inline comments.
INLINE COMMENTS
> graesslin wrote in composite.cpp:748
> this misses the
>
> if (m_scene->compositingType() & OpenGLCompositing)
also indention.
> x11_platform.cpp:206
> +// Deliberately continue with PreFrame
> +case OpenGLSafePoint::PreFrame:
> +if (m_openGLFreezeProtectionThread == nullptr) {
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?
=
REPOSITORY
rKWIN KWin
REVISION DETAIL
https://phabricator.kde.org/D3132
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
graesslin added inline comments. INLINE COMMENTS > antlarr wrote in x11_platform.cpp:229-232 > 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.% 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. yep, agree. REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added inline comments. INLINE COMMENTS > graesslin wrote in composite.h:242 > I would just count down the m_maxFramesTestedForSafety till it reaches 0 ok > graesslin wrote in x11_platform.cpp:215 > 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. > graesslin wrote in x11_platform.cpp:229-232 > 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. 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.% 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. REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
graesslin added inline comments. INLINE COMMENTS > composite.cpp:210-214 > + > kwinApp()->platform()->createOpenGLSafePoint(Platform::OpenGLSafePoint::PreFrame); > > m_scene = SceneOpenGL::createScene(this); > > + > kwinApp()->platform()->createOpenGLSafePoint(Platform::OpenGLSafePoint::PostLastFrame); why is the scene creation also guarded with PreFrame and PostLastFrame? Contextual that doesn't make sense as createScene doesn't render any frames. > antlarr wrote in composite.h:242 > ok. @graesslin do you prefer a count down or a count up and max? In any > case, I agree with changing the name. I would just count down the m_maxFramesTestedForSafety till it reaches 0 > antlarr wrote in x11_platform.cpp:215 > yeah, but then it wouldn't be a synchronous call and it would lose all its > meaning, isn't it? :) You can use QMetaObject::invokeMethod with Qt::QueuedConnection. > x11_platform.cpp:229-232 > +case OpenGLSafePoint::PostFrame: > m_openGLFreezeProtection->deleteLater(); > +m_openGLFreezeProtection = nullptr; > +break; 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. REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
luebking added inline comments. INLINE COMMENTS > antlarr wrote in x11_platform.cpp:215 > 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. 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? REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added inline comments. INLINE COMMENTS > luebking wrote in x11_platform.cpp:215 > You should be able to call it as slot (whether QMetaObject::invokeMethod() > works, I've never tried) yeah, but then it wouldn't be a synchronous call and it would lose all its meaning, isn't it? :) > luebking wrote in x11_platform.cpp:224 > Sorry, misread. (Actually didn't really read and just wanted to point out the > other case ;-) no problem :) REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
luebking added inline comments. INLINE COMMENTS > antlarr wrote in x11_platform.cpp:215 > 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) > antlarr wrote in x11_platform.cpp:224 > 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. Sorry, misread. (Actually didn't really read and just wanted to point out the other case ;-) REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added inline comments.
INLINE COMMENTS
> luebking wrote in composite.h:242
> m_testedFrames or similar, "Frame Count" is too generic.
> Alternatively, just use some "int m_freezeDectionFrames = 30;" and count
> them down (so when < 1, you're done)
ok. @graesslin do you prefer a count down or a count up and max? In any case,
I agree with changing the name.
> luebking wrote in platform.h:148
> Have a PreFirstFrame still and make PostLastFrame PostLastGuardedFrame or
> similar. ("Last frame" is right before kwin shuts down ;-)
There's no need for PreFirstFrame, PreFrame is the same for the first or for
the rest of "Pre" cases. But for the PostLastGuardedFrame note I agree. I'll
change that.
> luebking wrote in x11_platform.cpp:215
> Why do you recreate it with every frame?
> Only create it PreFirstFrame and delete it PostLastFrame (and inbeteween
> simply restart the pointer PreFrame)
The timer is in another thread, so it can't be stopped/restarted from this
thread.
> luebking wrote in x11_platform.cpp:224
> 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.
REVISION DETAIL
https://phabricator.kde.org/D3132
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: antlarr, #kwin, #plasma, davidedmundson
Cc: luebking, graesslin, kwin, plasma-devel, davidedmundson, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added a comment. Btw, I tested the last commit on a virtual machine simulating the freeze with a sleep. I'll check tomorrow what's the minimum value for m_maxFramesTestedForSafety that works to detect the real freeze and update its default value. REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
antlarr added a comment. > How many frames do we need to detect the freeze? I think 30 frames is too much by default. It's adding quite some stress on startup (lots of write to config, lots of additional threads started). Assuming we freeze with tripple buffer when trying to get the next buffer 4 frames should be sufficient to trigger the freeze? Yesterday I tried with 20 and 10 frames and it detected the freeze correctly, then tried 5 and got a strange behaviour (the desktop wasn't frozen but the menus were shown displaced and there was no window decoration). I decided I would remove an option I added to the nouveau driver to test if it helped and rebooted the machine to be sure I tested with a clean system. But then I lost the connection to the machine and it seems to have a problem booting up so I can't do more tests until Monday when someone goes to the SUSE offices where the machine is and check what the problem is. In the meantime, I will improve a bit the PreFrame and PostFrame code so it doesn't add so much overhead (reusing the thread and timer, and only write the config option in the lambda function, which wouldn't work to detect a crash, but it should work to detect a freeze, so I'll separate the code of PreFrame/PostFrame from the code in PreInit/PostInit). I'll update this phab later today with those optimizations. REPOSITORY rKWIN KWin REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3132: [platformx/x11] Add a freeze protection against OpenGL
graesslin added a comment. How many frames do we need to detect the freeze? I think 30 frames is too much by default. It's adding quite some stress on startup (lots of write to config, lots of additional threads started). Assuming we freeze with tripple buffer when trying to get the next buffer 4 frames should be sufficient to trigger the freeze? REPOSITORY rKWIN KWin REVISION DETAIL https://phabricator.kde.org/D3132 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: antlarr, #kwin, #plasma, davidedmundson Cc: graesslin, kwin, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
