Differential D2330

[xrandr backend] fall back to preferred mode, fix crash
ClosedPublic
Actions

Authored by sebas on Aug 2 2016, 12:11 AM.

Details

Reviewers
graesslin
dvratil
Commits
R110:f53805676dc0: [xrandr backend] fall back to preferred mode, fix crash
Summary

When enabling outputs, it's possible that a KScreen::Output's current
mode is empty. In this case, it's logical and necessary to fall back to
the output's preferred mode, instead of passing essentially
QString().toInt() as mode id into xcb_randr_set_crtc_config().

In the current code, kscreenOuput->currentMode()->size() can crash, this
is fixed with this patch as well.

This change makes enabling a VGA monitor to my docking station more reliable.

Test Plan

Connected, disconnted, reconfigured a tv connected via HDMI.

Diff Detail

Repository
R110 KScreen Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sebas updated this revision to Diff 5620.Aug 2 2016, 12:11 AM
sebas retitled this revision from to [xrandr backend] fall back to preferred mode, fix crash.
sebas updated this object.
sebas edited the test plan for this revision. (Show Details)
sebas added a reviewer: dvratil.
sebas added a subscriber: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptAug 2 2016, 12:11 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sebas added a comment.Aug 2 2016, 1:33 AM

Here's the backtrace I get without this patch. The crash actually happens inside the debug message, but when it happens, it also means that the mode id we try to pass into xcb_randr_set_crtc_config is invalid (most likely 0)

with disabled output:

kscreen-doctor output.280.enable output.280.position.2560,0 output.mode.

/home/sebas/kf5/install/lib/x86_64-linux-gnu/libexec/kf5/kscreen_backend_launcher crashes

[01::04:27.959] XRandRConfig::enableOutput: Trying to find a CRTC for 280 , crtcs: QMap((63, XRandRCrtc(0x479fc0))(64, XRandRCrtc(0x47a010))(65, XRandRCrtc(0x4808c0)))
[01::04:27.959] XRandRConfig::enableOutput: Testing CRTC 63
[01::04:27.959] XRandRConfig::enableOutput: Free: false
[01::04:27.960] XRandRConfig::enableOutput: Mode: 72
[01::04:27.960] XRandRConfig::enableOutput: Possible outputs: QVector(66, 67, 68, 69, 70, 278, 279, 280)
[01::04:27.960] XRandRConfig::enableOutput: Connected outputs: QVector(66)
[01::04:27.960] XRandRConfig::enableOutput: Geometry: QRect(0,0 2560x1440)
[01::04:27.960] XRandRConfig::enableOutput: Testing CRTC 64
[01::04:27.960] XRandRConfig::enableOutput: Free: true
[01::04:27.960] XRandRConfig::enableOutput: Mode: 0
[01::04:27.960] XRandRConfig::enableOutput: Possible outputs: QVector(66, 67, 68, 69, 70, 278, 279, 280)
[01::04:27.960] XRandRConfig::enableOutput: Connected outputs: QVector()
[01::04:27.960] XRandRConfig::enableOutput: Geometry: QRect(2560,0 0x0)
[01::04:27.961] XRandRConfig::enableOutput: RRSetCrtcConfig (enable output)
[01::04:27.961] XRandRConfig::enableOutput: Output: 280 ( "DP-2-3" )
[01::04:27.961] XRandRConfig::enableOutput: New CRTC: 64
[01::04:27.961] XRandRConfig::enableOutput: Pos: QPoint(2560,0)

Thread 1 "kscreen_backend" received signal SIGSEGV, Segmentation fault.
0x00007ffff7fd71ee in KScreen::Mode::size (this=0x0) at /home/sebas/kf5/src/kde/workspace/libkscreen/src/mode.cpp:102
102 return d->size;
(gdb) bt
#0 0x00007ffff7fd71ee in KScreen::Mode::size (this=0x0) at /home/sebas/kf5/src/kde/workspace/libkscreen/src/mode.cpp:102
#1 0x00007fffe908667b in XRandRConfig::enableOutput (this=0x479100, kscreenOutput=...) at /home/sebas/kf5/src/kde/workspace/libkscreen/backends/xrandr/xrandrconfig.cpp:521
#2 0x00007fffe9081798 in XRandRConfig::applyKScreenConfig (this=0x479100, config=...) at /home/sebas/kf5/src/kde/workspace/libkscreen/backends/xrandr/xrandrconfig.cpp:290
#3 0x00007fffe907cb20 in XRandR::setConfig (this=0x47a0c0, config=...) at /home/sebas/kf5/src/kde/workspace/libkscreen/backends/xrandr/xrandr.cpp:215
#4 0x00000000004065e4 in BackendDBusWrapper::setConfig (this=0x480210, configMap=...) at /home/sebas/kf5/src/kde/workspace/libkscreen/src/backendlauncher/backenddbuswrapper.cpp:85
#5 0x0000000000408496 in BackendAdaptor::setConfig (this=0x479dd0, in0=...) at /home/sebas/kf5/build/kde/workspace/libkscreen/src/backendlauncher/backendadaptor.cpp:51
#6 0x00000000004085f8 in BackendAdaptor::qt_static_metacall (_o=0x479dd0, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffce80)

at /home/sebas/kf5/build/kde/workspace/libkscreen/src/backendlauncher/backendadaptor.moc:113

#7 0x0000000000408774 in BackendAdaptor::qt_metacall (this=0x479dd0, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffce80)

at /home/sebas/kf5/build/kde/workspace/libkscreen/src/backendlauncher/backendadaptor.moc:156

#8 0x00007ffff7f0c280 in QDBusConnectionPrivate::deliverCall (this=0x7fffdc0030f0, object=0x479dd0, msg=..., metaTypes=..., slotIdx=8) at qdbusintegrator.cpp:978
#9 0x00007ffff7f0ba70 in QDBusConnectionPrivate::activateCall (this=0x7fffdc0030f0, object=0x479dd0, flags=273, msg=...) at qdbusintegrator.cpp:881
#10 0x00007ffff7f0f71e in QDBusConnectionPrivate::activateObject (this=0x7fffdc0030f0, node=..., msg=..., pathStartPos=8) at qdbusintegrator.cpp:1470
#11 0x00007ffff7f0ff76 in QDBusActivateObjectEvent::placeMetaCall (this=0x7fffdc00f6c0) at qdbusintegrator.cpp:1590
#12 0x00007ffff739cd7e in QObject::event (this=0x480210, e=0x7fffdc00f6c0) at kernel/qobject.cpp:1256
#13 0x00007ffff7363052 in QCoreApplicationPrivate::notify_helper (receiver=0x480210, event=0x7fffdc00f6c0) at kernel/qcoreapplication.cpp:1149
#14 0x00007ffff7362cd5 in doNotify (receiver=0x480210, event=0x7fffdc00f6c0) at kernel/qcoreapplication.cpp:1090
#15 0x00007ffff7362c48 in QCoreApplication::notify (this=0x7fffffffdab0, receiver=0x480210, event=0x7fffdc00f6c0) at kernel/qcoreapplication.cpp:1076
#16 0x00007ffff778e5b0 in QGuiApplication::notify (this=0x7fffffffdab0, object=0x480210, event=0x7fffdc00f6c0) at kernel/qguiapplication.cpp:1616
#17 0x00007ffff7362bd8 in QCoreApplication::notifyInternal2 (receiver=0x480210, event=0x7fffdc00f6c0) at kernel/qcoreapplication.cpp:1015
#18 0x00007ffff7366c0e in QCoreApplication::sendEvent (receiver=0x480210, event=0x7fffdc00f6c0) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:225
#19 0x00007ffff7363fc0 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x426c90) at kernel/qcoreapplication.cpp:1650
#20 0x00007ffff7363940 in QCoreApplication::sendPostedEvents (receiver=0x0, event_type=0) at kernel/qcoreapplication.cpp:1508
#21 0x00007ffff73dcb1e in postEventSourceDispatch (s=0x468920) at kernel/qeventdispatcher_glib.cpp:270
#22 0x00007ffff2d261a7 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#23 0x00007ffff2d26400 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#24 0x00007ffff2d264ac in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#25 0x00007ffff73dd2f7 in QEventDispatcherGlib::processEvents (this=0x467030, flags=...) at kernel/qeventdispatcher_glib.cpp:417
#26 0x00007fffeee481ac in QPAEventDispatcherGlib::processEvents (this=0x467030, flags=...) at eventdispatchers/qeventdispatcher_glib.cpp:115
#27 0x00007ffff735f54e in QEventLoop::processEvents (this=0x7fffffffda00, flags=...) at kernel/qeventloop.cpp:128
#28 0x00007ffff735f84a in QEventLoop::exec (this=0x7fffffffda00, flags=...) at kernel/qeventloop.cpp:204
#29 0x00007ffff7363322 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:1285
#30 0x00007ffff778e560 in QGuiApplication::exec () at kernel/qguiapplication.cpp:1607
#31 0x0000000000404c40 in main (argc=1, argv=0x7fffffffdc28) at /home/sebas/kf5/src/kde/workspace/libkscreen/src/backendlauncher/main.cpp:43

sebas updated this revision to Diff 5621.Aug 2 2016, 2:48 AM
  • make mode id fallback faster
graesslin added a subscriber: graesslin.Aug 2 2016, 5:56 AM
graesslin added inline comments.
backends/xrandr/xrandrconfig.cpp
518–521

it looks strange to me that you first access the currentModeId and afterwards check whether there is a currentMode.

What about a one liner?

const int modeId = kscreenOutput->currentMode() ? kscreenOutput->currentModeId().toInt() : kscreenOutput->preferredModeId().toInt();

If you don't like the one-liner (which I can understand) I would inverse the logic and init the modeId with preferredModeId and only set to currentModeId if there is a currentMode.

557–560

As you have twice the same code:

static inline int getBestModeId(auto *kscreenOutput) {
    return ....
}

That could also address my other comment nicely as then you can do nice if-else-return switches.

dvratil edited edge metadata.Aug 2 2016, 7:29 AM

I wonder how it happens that we try to enable an output without having the mode set. The KDED module should always pick the best resolution (or falling back to preferred mode) when it's enabling an output.

While this fix makes sense in as a crash-guard, I'd also look into why KDED is failing to set the mode in the first place.

backends/xrandr/xrandrconfig.cpp
520

I wonder if it might be safer to query the preferred mode from corresponding XRandROutput rather than trusting user-provided KScreen::Output here?

559

In changeOutput() we only change position, mode or rotation of an already-enabled output. I would argue that if the mode is missing in the kscreenOutput here (for any reason) it would make more sense to interpret it as "no change" and use xOutput->currentMode()->id() and only if xOutput->currentMode() is null then fall back to preferred mode (although that already hints that something weird is going on).

sebas planned changes to this revision.Aug 2 2016, 11:47 AM
sebas added inline comments.
backends/xrandr/xrandrconfig.cpp
518–521

I had it the other way round first.

preferredModeId() does a lot of magic to find a mode, so it's much slower and more invasive than currentModeId(), therefore, it makes sense to only execute preferredModeId() if we have to, hence the slightly reversed logic.

Your one-liner proposal addresses this as well, and looks more logical. I'll change.

520

Well, the API doesn't forbid to not set a mode but enable an output. This is how I triggered the crash:

kscreen-doctor output.280.enable output.280.position.2560,0

No mode set, without patch it crashes, with patch, it succeeds and does the logical thing.

I *think* (haven't checked) that falling back to preferred here is really a good idea, since

  • otherwise, the next thing is that we pass an invalid mode into xcb -- who knows what happens
  • the config can be loaded from a file which doesn't have mode id set (always the case for disabled displays), so at some point we need to look at preferred mode and pick that if we want to enable (I think the kded module does a pretty good job here, but I wouldn't guarantee that it *always* gets this right (as I said, not enforced), so better safe than sorry here.

The mode may not be set on the XRandROutput on disabled outputs, that's the problem I'm addressing here.

559

Makes sense. I'll change.

sebas updated this revision to Diff 5638.Aug 2 2016, 12:47 PM
sebas edited edge metadata.
  • address comments from mgraesslin and dvratil
graesslin requested changes to this revision.Aug 2 2016, 2:19 PM
graesslin added a reviewer: graesslin.
graesslin added inline comments.
backends/xrandr/xrandrconfig.cpp
557–559

I think you want to remove the if clause now

This revision now requires changes to proceed.Aug 2 2016, 2:19 PM
sebas requested a review of this revision.Aug 2 2016, 8:05 PM
sebas edited edge metadata.
sebas added inline comments.Aug 9 2016, 9:34 PM
backends/xrandr/xrandrconfig.cpp
557–559

No, this part is a bit different from the above hunk, it uses the xOutput by default, and if that is empty, falls back to kscreen's currentModeId or the preferredModeId.

graesslin added inline comments.Aug 10 2016, 11:23 AM
backends/xrandr/xrandrconfig.cpp
557–559

but then I would reorder it. Why call toInt() and then check afterwards whether that one can work at all? I would suggest to make it if-else then

sebas updated this revision to Diff 5796.Aug 10 2016, 11:30 AM
sebas edited edge metadata.
  • Merge branch 'master' into sebas/modefallback
  • Improve conditional
sebas updated this revision to Diff 5797.Aug 10 2016, 11:34 AM
  • Improve conditional
graesslin accepted this revision.Aug 10 2016, 11:35 AM
graesslin edited edge metadata.
This revision is now accepted and ready to land.Aug 10 2016, 11:35 AM
sebas updated this revision to Diff 5798.Aug 10 2016, 11:39 AM
sebas edited edge metadata.

BUG:366575

Closed by commit R110:f53805676dc0: [xrandr backend] fall back to preferred mode, fix crash (authored by sebas). · Explain WhyAug 10 2016, 11:39 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mbackends/xrandr/xrandrconfig.cpp (21 lines)

Diff 5799

View Options

backends/xrandr/xrandrconfig.cpp

Loading...