Bug 448778 - Unplugging an external monitor leads to a crash
Summary: Unplugging an external monitor leads to a crash
Status: REOPENED
Alias: None
Product: kwin
Classification: Plasma
Component: multi-screen (show other bugs)
Version: git-stable-Plasma/5.24
Platform: Debian stable Linux
: NOR crash (vote)
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords: wayland
Depends on:
Blocks:
 
Reported: 2022-01-19 15:37 UTC by Matej Mrenica
Modified: 2022-11-22 13:32 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.24


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matej Mrenica 2022-01-19 15:37:28 UTC
SUMMARY

STEPS TO REPRODUCE
1. Log into Plasma (Wayland)
2. Switch screens to "external monitor only"
3. unplug the monitor

OBSERVED RESULT
Kwin crashes

EXPECTED RESULT
It doesn't crash

SOFTWARE/OS VERSIONS
Kwin from git: Plasma 5.24 + https://invent.kde.org/plasma/kwin/-/merge_requests/1908

Log: https://pastebin.com/n35DGgM5
Comment 1 Nate Graham 2022-01-19 15:52:51 UTC
Does this happen when *not* running with that merge request? If not, can you comment in that merge request instead of here?
Comment 2 Matej Mrenica 2022-01-19 16:11:20 UTC
(In reply to Nate Graham from comment #1)
> Does this happen when *not* running with that merge request? If not, can you
> comment in that merge request instead of here?

Yes: https://pastebin.com/nns7Jn5b I could comment there but I was told to create a new report.
Comment 3 Zamundaaa 2022-01-19 16:28:25 UTC
I added a bunch of debugging prints to the MR. Can you trigger the crash with that and then attach the logs for kwin_wayland_drm here?
You can get them with either
$ grep kwin_wayland_drm ~/.local/share/sddm/wayland-session.log
or
$ journalctl --boot 0 | grep kwin_wayland_drm
(which depends on your distro)
Comment 4 Matej Mrenica 2022-01-19 17:52:53 UTC
I must be doing something wrong because neither of those commands produce any output.
What I did:
I cloned kwin branch Plasma/5.24 and applied your patch.
Then I built and installed kwin with: "cmake -B build  -DCMAKE_INSTALL_LIBEXECDIR=lib -DBUILD_TESTING=OFF".
Then I set QT_LOGGING_RULES="kwin_wayland_drm.debug=true;kwin_core.debug=true;kwin_wayland_backend.debug=true" in /etc/environment
Finally, I re-logged in and triggered the crash.
Comment 5 Zamundaaa 2022-01-19 19:17:43 UTC
Maybe logging in the legacy session is still broken... Try 
$ kwriteconfig5 --file startkderc --group General --key systemdBoot true
and reboot.
Comment 6 Matej Mrenica 2022-01-20 07:59:25 UTC
OK, now it gives an output: https://pastebin.com/Jb6T2m5F
Comment 7 Zamundaaa 2022-01-20 08:24:25 UTC
Did you reproduce the crash before getting the log? It only shows the external output being connected, not removed. Up to that point everything looks correct
Comment 8 Matej Mrenica 2022-01-20 08:42:35 UTC
In the log I provided I unplug the cable before 08:52:41, is that what you are asking? After that only a cursor appears on my laptop screen and after a few seconds Plasma is restarted (only with systemd startup).
Comment 9 Zamundaaa 2022-01-21 00:21:25 UTC
Sorry, didn't scroll up enough. KWin reads the configuration from KScreen, it is correct (eDP-1 is supposed to be enabled again) and then it gets applied... But it silently fails, which it really should not. 
I could now reproduce it myself though; at least on my laptop KScreen somehow uses a different refresh rate number (60001) than KWin (60000) and that makes applying the changes fail.

I didn't find the underlying bug yet but I pushed a commit to the MR that makes KWin not depend on the refresh rate to match in order to not crash.
Comment 10 Matej Mrenica 2022-01-21 08:20:03 UTC
Latest version of the MR fixes this issue. Unplugging the cable doesn't make Kwin crash anymore.
Comment 11 Zamundaaa 2022-01-27 18:57:13 UTC
Git commit 74b1fb0ab5344d3189e77538fc03c60c45b02707 by Xaver Hugl.
Committed on 27/01/2022 at 18:40.
Pushed by zamundaaa into branch 'master'.

backends/drm: round refresh rate values from KScreen configs

The implicit cast effectively rounds the value down, which make the
refresh rate be different from what KScreen actually wrote.
A better fix would be to use integers instead of floating point numbers
but that needs to happen in KScreen.

M  +2    -1    src/backends/drm/drm_backend.cpp

https://invent.kde.org/plasma/kwin/commit/74b1fb0ab5344d3189e77538fc03c60c45b02707
Comment 12 Zamundaaa 2022-01-27 19:13:13 UTC
Git commit 9f9ad7036518f0d0df984a3eda28e2b4c57607d3 by Xaver Hugl.
Committed on 27/01/2022 at 19:13.
Pushed by zamundaaa into branch 'Plasma/5.24'.

backends/drm: round refresh rate values from KScreen configs

The implicit cast effectively rounds the value down, which make the
refresh rate be different from what KScreen actually wrote.
A better fix would be to use integers instead of floating point numbers
but that needs to happen in KScreen.


(cherry picked from commit 74b1fb0ab5344d3189e77538fc03c60c45b02707)

M  +2    -1    src/backends/drm/drm_backend.cpp

https://invent.kde.org/plasma/kwin/commit/9f9ad7036518f0d0df984a3eda28e2b4c57607d3
Comment 13 Random Guy 2022-09-21 23:23:50 UTC
I believe I am having this same issue, in KDE version 5.25 on Wayland.  I have a PC with 2 monitors connected, and I have a projector on which I watch films/TV shows.  When I turn off the monitors and connect the projector the display sometimes freezes without the rest of the system crashing - music remains playing indefinitely in the background.  The same crash also occasionally happens when waking from sleep.  
 
I presume the fix mentioned in this thread is enough for most use cases, however I have one old monitor and one new monitor so my main setup is 1680*1050 @ 60hz on DVI and a gaming monitor using 1920*1050 @ up to 144hz (with AMD freesync) on HDMI so I think it is testing this fix quite hard (along with many other parts of the system - it took me a while to get it working at all).  It is not a dealbreaker as I can restart and retry when it breaks which is about 1 in 3 attempts, however I wanted to log the fact that this is still an issue.
Comment 14 Zamundaaa 2022-09-24 22:51:48 UTC
Please open a new bug report for that issue, it doesn't sound related at a first glance; KWin crashing does not make your monitors hang
Comment 15 Random Guy 2022-09-25 13:53:41 UTC
(In reply to Zamundaaa from comment #14)
> Please open a new bug report for that issue, it doesn't sound related at a
> first glance; KWin crashing does not make your monitors hang

The monitors are not actually hanging, I can use ctrl+alt+fX and login on another tty.  I think it might actually be a bug within wayland itself so I may make a report there.
Comment 16 Zamundaaa 2022-09-25 14:15:10 UTC
Wayland is not involved with what KWin displays. Please create a new bug report here
Comment 17 aiamuzz 2022-10-27 05:16:52 UTC
Hello @Zamundaaa,

SOFTWARE/OS VERSIONS
OS : Linux Deepin(Debian based)
KERNEL:
5.15.45-amd64-desktop
KWIN:
kwin 5.15.5

Seems like even i am facing the same issue, from a pretty long time that too ...

STEPS TO REPRODUCE
1. Log into the laptop with an external monitor attached
2. Works fine when external monitor is attached
3. Unplug monitor/dock(with monitor attached)
4. grey screen with only the cursor showing on the laptop screen cursor responds but crashed screen.

OBSERVED RESULT
Kwin crashes with only a grey screen with only the mouse cursor visible which responds fine when moved but the screen is unresponsive. a perpetual state until the monitor/dock is reconnected. 

My situation is so bad that I have to CONNECT an external monitor or atleast the VGA port to my laptop to get it come back from the freeze.
STRANGELY THOUGH WHEN I TURN 'WINDOWS EFFECT' OFF THEN THE LAPTOP IS USABLE WITH THE UGLY UI OF COURSE, FOR WINDOWS EFFECT IS THE REAL UX WITHOUT WHICH IT LIKE TRAVELLING IN A CARRIAGE IN THE AGE OF SPACE TRAVEL.


EXPECTED RESULT
It doesn't crash ON DISCONNECTING THE EXTERNAL MONITOR OR DOCKING STATION.
Comment 18 aiamuzz 2022-10-27 05:20:08 UTC
@zamundaaa, I will open another bug if you so wish, but the symptoms are the same as reported by @Matej Mrenica.

(as replied by Matej Mrenica from comment #8)
> In the log I provided I unplug the cable before 08:52:41, is that what you
> are asking? After that only a cursor appears on my laptop screen ...
Comment 19 Zamundaaa 2022-10-27 14:52:45 UTC
(In reply to aiamuzz from comment #18)
> @zamundaaa, I will open another bug if you so wish, but the symptoms are the
> same as reported by @Matej Mrenica.

Can you test if it happens on Wayland as well?
Also, can you test if it happens with a newer version of KWin? Or is 5.15.5 a typo? It's been unsupported for more than three years now.
Comment 20 aiamuzz 2022-10-28 14:51:21 UTC
(In reply to Zamundaaa from comment #19)
> (In reply to aiamuzz from comment #18)
> > @zamundaaa, I will open another bug if you so wish, but the symptoms are the
> > same as reported by @Matej Mrenica.
> 
> Can you test if it happens on Wayland as well?
> Also, can you test if it happens with a newer version of KWin? Or is 5.15.5
> a typo? It's been unsupported for more than three years now.

I know, I've heard that its 3 years old, unfortunately its true Linux Deepin are lagging way behind ...

> Can you test if it happens on Wayland as well ? 

I am sorry I am just a Linux power user with limited understanding, so i'll not know the answer to that question.
If you have some commands whose outputs you want, I can run them so you can help diagnose the issue i am facing.

It would sound amusing, But i have basically pulled a VGA Cable from an old CRT monitor along with the PCB its connected to and i kind of attach this dangling cable with the PCB to my laptop to get my laptop working with 'Windows Effect' enabled ... the alternative being turning off 'Windows Effect' and using an ugly interface ... with the choice between a deep sea and the devil, i've chosen a dangling cable at my office as am used to the features 'Windows Effect' has to offer.


FYI ... https://github.com/linuxdeepin/deepin-kwin ... this is the repo of the package that's probably running on my system.

I was just hoping you are able to identify the issue i am facing and raise a MR/PR to this repo with the patch/code correction so i can install it and rectify my problem.

Please let me know the set of commands you want me to run on my affected system.

Thanks for the quick and prompt response.
Comment 21 aiamuzz 2022-10-28 14:58:50 UTC
I notice the following ...

Extended monitor ...
Resolution : 1920 x 1080 (Recommended)
Refresh Rate : 60Hz (Recommended)

Laptop Screen ...
Resolution : 1920 x 1080 (Recommended)
Refresh Rate : 60.01Hz (Recommended)

I have a feeling the fix you offered here to @Matej Mrenica might fix my problem as well ... as i see in your comment 9 you've spoken of refresh rates of 60000 and 60001 ...
Comment 22 Zamundaaa 2022-10-28 15:19:17 UTC
Yes, that looks like the same underlying problem, no further testing is needed. We don't support such old versions and especially not forks of them though.

The things you can do is to ask the Deepin maintainers to update their fork of KWin to include an equivalent of https://invent.kde.org/plasma/kwin/-/commit/9f9ad7036518f0d0df984a3eda28e2b4c57607d3, or switch to a distro that offers an upstream version of KWin 5.24 or newer.
Comment 23 aiamuzz 2022-10-29 05:34:15 UTC
(In reply to Zamundaaa from comment #22)
> Yes, that looks like the same underlying problem, no further testing is
> needed. We don't support such old versions and especially not forks of them
> though.
> 
> The things you can do is to ask the Deepin maintainers to update their fork
> of KWin to include an equivalent of
> https://invent.kde.org/plasma/kwin/-/commit/
> 9f9ad7036518f0d0df984a3eda28e2b4c57607d3, or switch to a distro that offers
> an upstream version of KWin 5.24 or newer.

oh ... thanks for the confirmation.

unfortunately they are proceeding at snails pace ... the only hope is that they may have picked up the latest kwin version in their next years release. 

@Zamundaaa ... i was hoping to ask a favor ... 
the following is the version package whose fork my distro may have relied on.
https://invent.kde.org/plasma/kwin/-/archive/v5.15.5/kwin-v5.15.5.zip

I see that the folder and file structure is way different to the latest version, which is expected given the above is 3 years old.

If its possible i was hoping you could point me too the file that has the equivalent of the refreshRate line implementation ...

props->refreshRate = mode["refresh"].toDouble() * 1000;

I was hoping, as its only one word 'round ()' to be modified ... i though i could initiate the pull request on my distro forked repo.

I have been trying since the last couple of days with regard to 'src' folder and a file named 'drm_backend.cpp' but with no luck, of course its understandable the code would have undergone considerable change in structure and implementation in the last 3 years.

hoping for a helping hand ...

thanks for identifying and confirming the posssible issue.
Comment 24 aiamuzz 2022-10-29 06:01:17 UTC
apologies for trying to be self reliant ... I am the only one facing this issues it seems ... so trying to help myself ...


could these be the possible file that needs the corrected code ?

kwin-v5.15.5/plugins/platforms/drm/drm_output.cpp

relevant parts ???

**************

namespace {
quint64 refreshRateForMode(_drmModeModeInfo *m)
{
    // Calculate higher precision (mHz) refresh rate
    // logic based on Weston, see compositor-drm.c
    quint64 refreshRate = (m->clock * 1000000LL / m->htotal + m->vtotal / 2) / m->vtotal;
    if (m->flags & DRM_MODE_FLAG_INTERLACE) {
        refreshRate *= 2;
    }
    if (m->flags & DRM_MODE_FLAG_DBLSCAN) {
        refreshRate /= 2;
    }
    if (m->vscan > 1) {
        refreshRate /= m->vscan;
    }
    return refreshRate;
}
}


**************


  // read in mode information
    QVector<KWayland::Server::OutputDeviceInterface::Mode> modes;
    for (int i = 0; i < connector->count_modes; ++i) {
        // TODO: in AMS here we could read and store for later every mode's blob_id
        // would simplify isCurrentMode(..) and presentAtomically(..) in case of mode set
        auto *m = &connector->modes[i];
        KWayland::Server::OutputDeviceInterface::ModeFlags deviceflags;
        if (isCurrentMode(m)) {
            deviceflags |= KWayland::Server::OutputDeviceInterface::ModeFlag::Current;
        }
        if (m->type & DRM_MODE_TYPE_PREFERRED) {
            deviceflags |= KWayland::Server::OutputDeviceInterface::ModeFlag::Preferred;
        }

        KWayland::Server::OutputDeviceInterface::Mode mode;
        mode.id = i;
        mode.size = QSize(m->hdisplay, m->vdisplay);
        mode.flags = deviceflags;
        mode.refreshRate = refreshRateForMode(m);
        modes << mode;
    }


**************

void DrmOutput::setWaylandMode()
{
    AbstractOutput::setWaylandMode(QSize(m_mode.hdisplay, m_mode.vdisplay),
                                   refreshRateForMode(&m_mode));
}

**************
Comment 25 aiamuzz 2022-10-29 06:14:11 UTC
oh ok ... seems like the exact same file ... "drm_backend.cpp" ... the implementation however seems quite different to the one you have in the latest version.


***************
 //2.init wayland output device
    QString name = "VGA-1";
    QString model = "VGA-1000-unknown";
    QString manufacturer = "UOS";
    QByteArray uuid = "ffffffffff";
    QVector<KWayland::Server::OutputDeviceInterface::Mode> modes;
    KWayland::Server::OutputDeviceInterface::Mode dmode;
    dmode.id = 0;
    dmode.size = QSize(mode.hdisplay, mode.vdisplay);
    dmode.flags = KWayland::Server::OutputDeviceInterface::ModeFlag::Current;
    dmode.refreshRate = mode.vrefresh * 1000LL;
    modes << dmode;
    output->initWaylandOutputDevice(name, model, manufacturer, uuid, modes);
**************
Comment 26 aiamuzz 2022-10-29 06:23:37 UTC
I know at the risk of sounding foolish ... will this do the trick ?

your MR ... for the latest version

-  props->refreshRate = mode["refresh"].toDouble() * 1000;
+  props->refreshRate = round(mode["refresh"].toDouble() * 1000);

will the following do the trick to the 3 yr old version ?

-  dmode.refreshRate = mode.vrefresh * 1000LL;
+  dmode.refreshRate = round(mode.vrefresh * 1000LL);

"A better fix would be to use integers instead of floating point numbers"

i don't see toDouble(floating point number) implementation in the above line of code as seen in the implementation of the latest versions ... I am guessing it may need some more tinkering with the code in files preceding "drm_backends.cpp"

hmmmm ...
Comment 28 aiamuzz 2022-11-22 05:55:56 UTC
@Zamundaaa ... firstly apologies for what may seem spamming your thread for additional help ... it only goes to show my desperation ... apologies for the same.

Please find the commit from my distro team ...

https://github.com/linuxdeepin/deepin-kwin/pull/82/commits/8530e2cbf5641b6e56cfb04a202ae95237f3dbc8

they have made this changes as per your commit ... https://invent.kde.org/plasma/kwin/-/commit/9f9ad7036518f0d0df984a3eda28e2b4c57607d3

I have a foolish(may be) question ... after your commit is patched by the affected user, will the monitor frequency show 60Hz or will it continue to show 60.01Hz ... ? ... reason i ask is my display properties is still showing 60.01Hz after patching my machine with the distro's patch ...
Comment 29 aiamuzz 2022-11-22 13:31:40 UTC
@Zamundaaa ... any suggestions on the following ?

https://github.com/linuxdeepin/developer-center/issues/3480#issuecomment-1323123155