Differential D23591

[platforms/drm] Disable orientation sensor for now
AbandonedPublic
Actions

Authored by romangg on Aug 30 2019, 11:43 PM.

Details

Reviewers
zzag
Group Reviewers
KWin
Maniphest Tasks
T11475: Redesign orientation sensor
Summary

The orientation sensor causes a crash when disabling and afterwards enabling
again the first output in a multi-monitor environment. This is difficult to
debug, since gdb does not provide good information and with valgrind it does
not crash indicating that it is a race. But output of gdb and valgrind together
shows that the orientation sensor is responsible.

For now disable the orientation sensor. Reasons are:

  • Output enablement is a basic feature and must work first flawlessly before thinking about advanced features.
  • The orientation sensor did never really work flawlessly according to user feedback.
  • The orientation sensor hooks into Screens class which will go away. A rewrite of the sensor is necessary.
Test Plan

When orientation sensor is disable dthe first output can be disabled and enabled
again

Diff Detail

Repository
R108 KWin
Branch
disableOrientationSensorOnDrm
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15875
Build 15893: arc lint + arc unit
romangg created this revision.Aug 30 2019, 11:43 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 30 2019, 11:43 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Aug 30 2019, 11:43 PM
romangg added a comment.Aug 30 2019, 11:45 PM

gdb output:

Thread 1 "kwin_wayland" received signal SIGSEGV, Segmentation fault.
0x00007fccfdb20258 in QMetaObject::disconnect(QObject const*, int, QObject const*, int) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
(gdb) bt
#0  0x00007fccfdb20258 in QMetaObject::disconnect(QObject const*, int, QObject const*, int) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#1  0x00007fccfed865bd in  () at /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#2  0x00007fccfed86be3 in  () at /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#3  0x00007fccfed8700f in  () at /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#4  0x00007fccfdb1b3e2 in QObject::event(QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007fccfe0fc65c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#6  0x00007fccfe103b90 in QApplication::notify(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x00007fccfdaead18 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x00007fccfdaed8d7 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007fccfdb44fd4 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x00007fcce7e6eaad in QUnixEventDispatcherQPA::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/KWinQpaPlugin.so
#11 0x00007fccfdae903a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#12 0x00007fccfdaf2170 in QCoreApplication::exec() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#13 0x000055dc49b91658 in main(int, char**) (argc=4, argv=0x7fff9709ff28) at /home/roman/dev/kde/src/kde/workspace/kwin/main_wayland.cpp:677

Relevant valgrind output:

==31253== Invalid read of size 8
==31253==    at 0x7CD3108: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x7CD4287: QMetaObject::disconnect(QObject const*, int, QObject const*, int) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x6A375BC: ??? (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.12.3)
==31253==    by 0x6A37BE2: ??? (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.12.3)
==31253==    by 0x6A3800E: ??? (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.12.3)
==31253==    by 0x7CCF3E1: QObject::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x731A65B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.12.3)
==31253==    by 0x7321B8F: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.12.3)
==31253==    by 0x7C9ED17: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x7CA18D6: QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x7CF8FD3: QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x1DD9EAAC: QUnixEventDispatcherQPA::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (in /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/KWinQpaPlugin.so)
==31253==  Address 0x24aeac70 is 64 bytes inside a block of size 128 free'd
==31253==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31253==    by 0x7CD6042: QObject::~QObject() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x51E9CDF: OrientationSensorAdaptor::~OrientationSensorAdaptor() (orientationsensoradaptor.cpp:31)
==31253==    by 0x51E9CFB: OrientationSensorAdaptor::~OrientationSensorAdaptor() (orientationsensoradaptor.cpp:34)
==31253==    by 0x51A97E3: KWin::OrientationSensor::setEnabled(bool) (orientation_sensor.cpp:140)
==31253==    by 0x5045CD7: KWin::Screens::Screens(QObject*)::{lambda()#1}::operator()() const (screens.cpp:70)
==31253==    by 0x5046CAD: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KWin::Screens::Screens(QObject*)::{lambda()#1}>::call({lambda()#1}&, void**) (qobjectdefs_impl.h:146)
==31253==    by 0x5046C7F: void QtPrivate::Functor<KWin::Screens::Screens(QObject*)::{lambda()#1}, 0>::call<QtPrivate::List<>, void>({lambda()#1}&, void*, {lambda()#1}&*) (qobjectdefs_impl.h:256)
==31253==    by 0x5046C4D: QtPrivate::QFunctorSlotObject<KWin::Screens::Screens(QObject*)::{lambda()#1}, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:439)
==31253==    by 0x7CCE98E: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x5205CFC: KWin::Screens::changed() (moc_screens.cpp:276)
==31253==    by 0x21639BD2: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (KWin::Screens::*)()>::call(void (KWin::Screens::*)(), KWin::Screens*, void**) (qobjectdefs_impl.h:152)
==31253==  Block was alloc'd at
==31253==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31253==    by 0x6A37E83: QDBusAbstractAdaptor::QDBusAbstractAdaptor(QObject*) (in /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5.12.3)
==31253==    by 0x51E9C96: OrientationSensorAdaptor::OrientationSensorAdaptor(KWin::OrientationSensor*) (orientationsensoradaptor.cpp:25)
==31253==    by 0x51A9775: KWin::OrientationSensor::setEnabled(bool) (orientation_sensor.cpp:136)
==31253==    by 0x5045CD7: KWin::Screens::Screens(QObject*)::{lambda()#1}::operator()() const (screens.cpp:70)
==31253==    by 0x5046CAD: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KWin::Screens::Screens(QObject*)::{lambda()#1}>::call({lambda()#1}&, void**) (qobjectdefs_impl.h:146)
==31253==    by 0x5046C7F: void QtPrivate::Functor<KWin::Screens::Screens(QObject*)::{lambda()#1}, 0>::call<QtPrivate::List<>, void>({lambda()#1}&, void*, {lambda()#1}&*) (qobjectdefs_impl.h:256)
==31253==    by 0x5046C4D: QtPrivate::QFunctorSlotObject<KWin::Screens::Screens(QObject*)::{lambda()#1}, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:439)
==31253==    by 0x7CCE98E: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.3)
==31253==    by 0x5205CFC: KWin::Screens::changed() (moc_screens.cpp:276)
==31253==    by 0x50472C0: KWin::OutputScreens::init() (outputscreens.cpp:39)
==31253==    by 0x5045C18: KWin::Screens::create(QObject*) (screens.cpp:48)
zzag added a subscriber: zzag.Aug 31 2019, 12:06 AM

Is it caused by us or Qt?

plugins/platforms/drm/drm_output.cpp
1017–1023

In general if you want to temporarily disable something, you have to use #if 0 #endif so git history is not altered.

romangg added inline comments.Aug 31 2019, 12:10 AM
plugins/platforms/drm/drm_output.cpp
1017–1023

ok, good point.

romangg updated this revision to Diff 65024.Aug 31 2019, 12:16 AM
  • Use if 0
romangg marked 2 inline comments as done.Aug 31 2019, 12:16 AM
In D23591#522692, @zzag wrote:

Is it caused by us or Qt?

I haven't looked closer at it yet because I'm working on other project. But I very much assume it's caused by us. In my opinion the whole orientation feature must be redone so we can integrate it nicely in our overall AbstractOutput structure. Also I would like to have a fallback path to software rotation and this path shouldn't be just patched on top. This will be a bigger project all together.

anthonyfieroni added a subscriber: anthonyfieroni.EditedAug 31 2019, 9:09 AM

It's added in D8738. Looks like a use after free issue https://phabricator.kde.org/source/kwin/browse/master/orientation_sensor.cpp$140 can be deleteLater, i think. Also DBus adaptors shouldn't be deleted, they should persist in app lifetime.

zzag accepted this revision.Aug 31 2019, 10:45 AM
This revision is now accepted and ready to land.Aug 31 2019, 10:45 AM
anthonyfieroni added a comment.Aug 31 2019, 10:47 AM

-1 to disable something without knowing the reason to not work.

zzag added a comment.Aug 31 2019, 10:48 AM
In D23591#522870, @anthonyfieroni wrote:

-1 to disable something without knowing the reason to not work.

Currently all kwin developers are busy with other stuff. I doubt that we'll be able to fix this before 5.17 release. If you want to help us with this problem, please do that.

romangg added a comment.Aug 31 2019, 11:00 AM
In D23591#522870, @anthonyfieroni wrote:

-1 to disable something without knowing the reason to not work.

Sure, but my assumption is that the orientation needs to be fully redone and I don't want me or other devs spend development time on fixing bugs in something going away anyway. See T11475 for my plan how I imagine a redesigned orientation sensor (should add the task link to the code comment in this diff). And because on the other side the orientation sensor is barely functional at all at the moment from the feedback I got I prefer to just disable it for now and remove it later.

davidedmundson added a subscriber: davidedmundson.Aug 31 2019, 5:49 PM

Whilst I support your future direction and would agree with not putting too much time into it, if the orientation sensor has issues and we were to do a cheap disabling hack, we should to it in the orientation sensor...not somewhere unrelated in DRM code which will impact any future work to port to anything else.

Also, I've probably fixed it. See D23610.

zzag added a comment.Aug 31 2019, 6:11 PM

Okay, it looks like this change is no longer actual. :-)

romangg abandoned this revision.Aug 31 2019, 9:11 PM

Then let's go with the fix attempt first.

Revision Contents
Changeset List

PathPackages
Mplugins/platforms/drm/drm_output.cpp (20 lines)

Diff 65023

View Options

plugins/platforms/drm/drm_output.cpp

Loading...