Commit 8a89d204 authored by Christoph Lutz's avatar Christoph Lutz Committed by Sebastian Kügler

let's continue in debug code instead of returning from XRandRConfig::applyKScreenConfig

Summary:
In XRandRConfig::applyKScreenConfig there is some debug code for a situation that should not really happen (but that was seen in the past) in which currentMode is NULL. After printing the debug info, the method directly returned from applyKScreenConfig, omitting all the other potentially useful initializations code that comes afterwards.

From my POV this is wrong and we had situations in which desktop initialization works better if we don't return here. That's why I changed "return" into "continue".

Analysis of the underlying inconsistency:

The debugging led me from libkscreen to xrandr to libxrandr (XRRGetOutputInfo) to the kernel. Here I noticed the following things:

        1) outputInfo = XRRGetOutputInfo(outputId) returns some outputInfo that dosn't contain
           the mode it should contain (concrete the ID 77). It was simply missing.

        2) crtcInfo = XRRGetCrtcInfo(outputInfo->crtc) But this crtcInfo-Objekt does reference
           the mode 77: crtcInfo->mode is 77

        3) XRRGetScreenResourcesCurrent() also thinks that mode 77 should be present

        4) Because mode 77 was not in outputInfo, XRandROutput::updateModes could not re-add
           this mode to the map m_modes and XRandROutput::currentMode() had to return NULL.
           Nothing wrong in updateModes!

Because libxrandr in 1) also delegates the above mentioned requests directly to the CRTC-driver of the kernel, I came to the conclusion that the inconsistency behind this situation comes from the kernel. It was also hard to reproduce on side of the kernel since multiple requests returned different results (kind of sporadic).

As mentioned above, I could sporadically reproduce the inconsistency with kernel 3.13.0 and as I tried a newer kernel the issue seemed to be vanished. This is the point where I decided to not dig deeper into such an old kernel version.

So in conclusion for me the underlying bug seems to be a kernel bug and not a libkscreen-bug. Maybe in the meanwhile nobody will ever cross the debug code again. BUT in my concrete situation libkscreen seems to be able to recover things from this inconsistency and "continuing" instead of "returning" simply improves the behaviour of the complete system.

Test Plan:
The concrete situation in which I regularily got this currentMode == NULL situation leads to the following bug:

      - kernel 3.13.0
      - physical pc with two displays, one 4:3 and the other 16:9 format
      - forced the system to always start in clone-mode if there is not
        yet a user profile --> we patched kscreen therefore
      - doing the following workflow:
          - log in --> sessions comes up in clone mode (as wished)
          - autorandr-gui --> "extended desktop - left"
          - press OK and DON't store the profile (so no change is stored)
          - log off
          - log in again
      - The following things happen:
          - Now we run into this "currentMode == NULL" situation (message in log file)
          - the screens come up in clone mode (as the previous settings were not saved)
          - BUG: the resolutions are wrong (parts from one of the two screens are cut).
                 applyKScreenConfig returned before it was finished.

With this chage, this bug no longer occures and the resolution is as expected set to the best commonly available resolution.

Reviewers: davidedmundson

Reviewed By: davidedmundson

Subscribers: davidedmundson, plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D6011
parent 114d654b
......@@ -200,7 +200,7 @@ void XRandRConfig::applyKScreenConfig(const KScreen::ConfigPtr &config)
// qDebug() << kRealBacktrace(256);
printConfig(config);
printInternalCond();
return;
continue;
}
// If the output would not fit into new screen size, we need to disable
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment