[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #13 from Martin Flöser  ---
(In reply to Bhushan Shah from comment #12)
> Can I re-open this bug, the commit above fixes the issue of crash, but right
> after starting plasma kwin_wayland is hung, and there is no mouse pointer as
> well, so it's still impossible to interact with Plasma.. or should I report
> separate bug?

Please use a separate bug. It's a different issue now, most likely related to
the incorrect request we get from KScreen.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Bhushan Shah]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #12 from Bhushan Shah  ---
Can I re-open this bug, the commit above fixes the issue of crash, but right
after starting plasma kwin_wayland is hung, and there is no mouse pointer as
well, so it's still impossible to interact with Plasma.. or should I report
separate bug?

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

Martin Flöser  changed:

   What|Removed |Added

  Latest Commit||https://commits.kde.org/kwi
   ||n/c601e875cfa7d673f1567c710
   ||87036631e42d272
 Resolution|--- |FIXED
 Status|CONFIRMED   |RESOLVED

--- Comment #11 from Martin Flöser  ---
Git commit c601e875cfa7d673f1567c71087036631e42d272 by Martin Flöser.
Committed on 13/11/2017 at 20:19.
Pushed by graesslin into branch 'master'.

[platforms/drm] At safety checks for the properties

Summary:
The AMS code accesses elements in a vector which might not be valid. This
change refactors the code to be more robust, especially the DrmPlane,
which started to crash after adding transformation support.

Reviewers: #kwin, #plasma, fvogt, subdiff

Subscribers: plasma-devel, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D8752

M  +14   -5plugins/platforms/drm/drm_object.cpp
M  +17   -14   plugins/platforms/drm/drm_object.h
M  +2-2plugins/platforms/drm/drm_object_connector.cpp
M  +5-2plugins/platforms/drm/drm_object_crtc.cpp
M  +24   -9plugins/platforms/drm/drm_object_plane.cpp
M  +3-0plugins/platforms/drm/egl_gbm_backend.cpp

https://commits.kde.org/kwin/c601e875cfa7d673f1567c71087036631e42d272

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #10 from Martin Flöser  ---
Thanks for testing. Looks like init on DrmOutput fails. I'll have a look.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Fabian Vogt]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #9 from Fabian Vogt  ---
(In reply to Martin Flöser from comment #8)
> Possible patch at: https://phabricator.kde.org/D8752

I tried this patch and kwin still crashes in QEMU, but at a different place now
at least:

#0  KWin::DrmOutput::pixelSize (this=0x0) at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_output.cpp:162
#1  0x7fffdcccfca9 in KWin::DrmCrtc::blank (this=0x55889a10)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_object_crtc.cpp:85
#2  0x7fffdccd1398 in KWin::DrmOutput::~DrmOutput (this=0x55843240,
__in_chrg=)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_output.cpp:68
#3  0x7fffdccd1699 in KWin::DrmOutput::~DrmOutput (this=0x55843240,
__in_chrg=)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_output.cpp:87
#4  0x7fffdccc9912 in KWin::DrmBackend::updateOutputs
(this=this@entry=0x557cb500)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_backend.cpp:460
#5  0x7fffdccca81d in KWin::DrmBackend::openDrm (this=0x557cb500)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/plugins/platforms/drm/drm_backend.cpp:326
#6  0x75a0ffec in QMetaObject::activate(QObject*, int, int, void**) ()
from /usr/lib64/libQt5Core.so.5
#7  0x77b1c9e2 in KWin::LogindIntegration::hasSessionControlChanged
(this=, _t1=, _t1@entry=true)
at
/usr/src/debug/kwin-5.11.90git.20171108T174438~9df174483/build/kwin_autogen/EWIEGA46WW/moc_logind.cpp:188

At this point `export KWIN_DRM_NO_AMS=1` helps though and the session starts
up. Unfortunately kwin_wayland gives up just after these messages:

kscreen.kwayland: Loading Wayland backend.
kscreen.kded: PowerDevil SuspendSession action not available!
kscreen.kwayland: Loading Wayland backend.
kwin_wayland: Couldn't find current GLX or EGL context.

and crashes somewhere in the destructors of Qt's global objects. That's exactly
the same state as Comment 5 though.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

Martin Flöser  changed:

   What|Removed |Added

URL||https://phabricator.kde.org
   ||/D8752
  Flags||ReviewRequest+

--- Comment #8 from Martin Flöser  ---
Possible patch at: https://phabricator.kde.org/D8752

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Bhushan Shah]

https://bugs.kde.org/show_bug.cgi?id=386490

Bhushan Shah  changed:

   What|Removed |Added

 CC||bhus...@gmail.com

--- Comment #7 from Bhushan Shah  ---
A data point,

The KWIN_DRM_NO_AMS property is not totally useful, if I export it, the kwin
"starts" but it is not functional : see the following qemu screenshot :
https://i.imgur.com/Gjy2cy4.jpg

It basically doesn't get screen resolution correctly and I can't interact with
the kwin/plasmashell.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #6 from Martin Flöser  ---
As a workaround you could also 
export KWIN_DRM_NO_AMS=1

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Fabian Vogt]

https://bugs.kde.org/show_bug.cgi?id=386490

Fabian Vogt  changed:

   What|Removed |Added

 CC||fab...@ritter-vogt.de

--- Comment #5 from Fabian Vogt  ---
I got kwin_wayland to work in openQA again, I had to revert the following
commits:

Revert "[drm] Implement changing of modes"
This reverts commit 33a4cf405071f2c74977a68c5b332dde4cea5853.

Revert "[platforms/drm] Fix typo in cleanup of eglSurface"
This reverts commit 7739dea33c2233b1fbbd49f52fc6263bfc2e2f0c

Revert "[platforms/drm] Add support for rotation property on the Plane"
This reverts commit 77b5c3caa9534862a057ea76f26bd094aaaff56e

Revert "[platforms/drm] Rotate screen if requested from KScreen"
This reverts commit b2d8bbec8157f9f8fbe70ace473162437dee5676

Revert "[platforms/drm] Restore previous mode if an atomic test fails"
This reverts commit 26cdfd317fc87b024ebcd9c2adefc86cfd59504e

Revert "[platforms/drm] Properly adjust cursor position on a rotated
output"
This reverts commit c06c234778f8c9ad3c5ba708097240a7a88d8a62

The revert of b2d8bbec8157f9f8fbe70ace473162437dee5676 is what fixes the crash
originally reported here.
However, even with that revert it did not work:

The behaviour I experienced while trying to figure out what's wrong exactly:
- dbus-launch startplasmacompositor (or just using sddm) resulted in ksplashqml
starting, but after a few seconds kwin_wayland crashed
- The crash is not related to the reason that broke it, kwin_wayland exits for
an invisible reason (nothing in the log) and crashes in ~DrmOutput.
- Running kwin_wayland with konsole/kwrite/anything else worked fine

So my guess is that kscreen's requests for rotation somehow cause the DRM
backend to give up.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

Martin Flöser  changed:

   What|Removed |Added

  Component|wayland-generic |platform-drm

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #4 from Martin Flöser  ---
I would say all access needs guarding. My interpretation of
https://01.org/linuxgraphics/gfx-docs/drm/drm-kms-properties.html is that the
properties are not mandatory. For our usage rotation is clearly optional so we
don't need to assert on it or report it as an error. We should consider it
optional.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Roman Gilg]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #3 from Roman Gilg  ---
Yes, that's a good idea. We could then decide to not use this
plane/crtc/connector.

That said, my assumption is that for an atomic mode property this should never
happen. But since we would do this check only once at the beginning of runtime
I'm fine with it. Or should we assert? I mean if my assumption is correct it
would be a critical driver bug. Maybe don't hide it.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #2 from David Edmundson  ---
That still leaves an inconsistency where you're guarding the insert in
initProps but not guarding access.

If you're gonna do that, you should error on initProps if one of the other
props doesn't exist.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[Roman Gilg]

https://bugs.kde.org/show_bug.cgi?id=386490

--- Comment #1 from Roman Gilg  ---
The problem seems to be, that not all drivers export the newly queried
"rotation" property to user space in contrast to the atomic mode setting
propertiers, which are expected to all get exported to my knowledge.

The "rotation" property isn't atomic, so it might make sense to instead of from
now on guarding all atomic properties that we just don't use the Property class
for rotation and instead introduce a local variable in DrmPlane for the
rotation property.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 386490] Crash in Atomic DRM setting

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=386490

David Edmundson  changed:

   What|Removed |Added

 Ever confirmed|0   |1
 Status|UNCONFIRMED |CONFIRMED
 CC||subd...@gmail.com
  Flags||Wayland+, X11-

-- 
You are receiving this mail because:
You are watching all bug changes.