Rotation in the past was not working in the DRM backend reliable. Now on 5.17
it even freezes the KWin session, so for now we need to just disable it trying.
Details
- Reviewers
- None
- Group Reviewers
KWin - Commits
- R108:1bd2f8ba6370: [platforms/drm] Disable transformations
Tried to change Rotation with KScreen. Nothing happened but session was still
usable afterwards.
Diff Detail
- Repository
- R108 KWin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Disable only 90° rotations and do this in the Drm backend. That being said
while 180° degree rotations work the cursor is not visible anymore on my
system which makes it difficult to revert it back and impossible to use.
Rotation in the past was not working in the DRM backend reliable. Now on 5.17 it even freezes the KWin session, so for now we need to just disable it trying.
What video card do you have? Do planes have rotation property?
Please do that first, currently it looks like we're shooting in the dark.
IIRC, ascent12 made an utility to dump information about DRM devices.
This diff is not meant to fix anything. It just ensures users can not render the session unusable accidentally by changing rotation.
I.e. when I have time I want to look into it, but currently I don't have (and won't till release). I also assume there needs to be some larger change for this to make work not suitable for 5.17 release.
I get it, however we should analyze the problem first, e.g. whether that's a kwin bug or something else. Let's not repeat D23591
I've just had a quick look at i915 source code. Apparently, it requires Y/Yf tiling in order for rotate-90 and rotate-270 to work, which means we need to add support for buffer modifiers. However, this also means that the drm backend does bad job at state management (TEST_ONLY commit should have failed).
Anyway, if no one wants to put any work into fixing this issue prior to 5.17 release, feel free to merge this patch. Although I recommend to disable output rotation altogether until we have proper fix.
However, this also means that the drm backend does bad job at state management (TEST_ONLY commit should have failed).
I was testing this on slightly different device then i915, and based on quick debugging, it seems TEST_ONLY commit is failing correctly all the time but when trying to reset the lastState it goes into infinite loop.
I did more investigation and for some reason last state is set to incorrect mode, which starts recursion.. I am yet to figure out why this happens, is it DRM lying to us or something I am not quite sure. If I can't figure this out by this week, let's merge this I guess.
Some more debugging with master branch,
I added following debug in doAtomicCommit
qCDebug(KWIN_DRM) << "BSHAH " << m_mode.hdisplay << m_mode.hsync_start << m_mode.hsync_end << m_mode.htotal << m_mode.hskew << m_mode.vdisplay << m_mode.vsync_start << m_mode.vsync_end << m_mode.vtotal << m_mode.vscan << m_mode.vrefresh << m_mode.flags << m_mode.type << m_mode.name << really << flags; //really is whether commit is real or test
And ran following to capture relevant output in kwin log.
kscreen-doctor output.2.rotation.left && sleep 2 && kscreen-doctor output.2.rotation.normal
Which results in following,
At startup we have set following mode, and it all is correctly, first is test commit and next is final commit
kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 1280 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 1 1025 kwin_wayland_drm: Atomic Modeset successful.
After that it's mostly repaint/pageflip commits
<snip> kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 256 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 1 513 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 256 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 1 513 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 256 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 1 513 </snip>
Now once kscreen-doctor command is executed, it commits the test commit,
kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 1280 kwin_wayland_drm: Atomic request failed to commit: No space left on device kwin_wayland_drm: Atomic test commit failed. Aborting present.
Which fails with no space left on device. (ENOSPC).. Sleeping for 2 seconds continues same spam loop, with same mode set, despite in test commit fallback code we trying to set previous mode and transformation.
kwin_wayland_drm: Atomic test commit failed. Aborting present. kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 1280 kwin_wayland_drm: Atomic request failed to commit: No space left on device kwin_wayland_drm: Atomic test commit failed. Aborting present. kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 0 1280 kwin_wayland_drm: BSHAH 1920 1968 2000 2082 0 1080 1082 1087 1144 0 0 9 0 1 1025 kwin_wayland_drm: Atomic Modeset successful.
After 2 seconds, setting normal rotation make it send same test commit and it passes :/ It is quite weird because despite we setting same transformation back in "if test commit fails" code base, it doesn't actually manage to set that, but later works fine.
Next I'll enable LIBGL_DEBUG=verbose and see if this makes any sense or provides me with reasoning why it does this.
PS: any simple way or function to transform mode to string rather then looong debug I have? :P
Also one of my initial suspicion was that we are not setting transformation wl_output in case commit fails:
So I tried following but it didn't make any difference.
diff --git a/plugins/platforms/drm/drm_output.cpp b/plugins/platforms/drm/drm_output.cpp index bfc9d4026..3e5afb9a6 100644 --- a/plugins/platforms/drm/drm_output.cpp +++ b/plugins/platforms/drm/drm_output.cpp @@ -849,6 +849,8 @@ bool DrmOutput::presentAtomically(DrmBuffer *buffer) m_primaryPlane->setNext(buffer); m_nextPlanesFlipList << m_primaryPlane; + + auto wlOutput = waylandOutput(); if (!doAtomicCommit(AtomicCommitMode::Test)) { //TODO: When we use planes for layered rendering, fallback to renderer instead. Also for direct scanout? @@ -862,6 +864,9 @@ bool DrmOutput::presentAtomically(DrmBuffer *buffer) if (m_primaryPlane) { m_primaryPlane->setTransformation(m_lastWorkingState.planeTransformations); } + if (wlOutput) { + wlOutput->setTransform(m_lastWorkingState.outputTransformation); + } m_modesetRequested = true; // the cursor might need to get rotated updateCursor(); @@ -886,6 +891,9 @@ bool DrmOutput::presentAtomically(DrmBuffer *buffer) if (m_primaryPlane) { m_lastWorkingState.planeTransformations = m_primaryPlane->transformation(); } + if (wlOutput) { + m_lastWorkingState.outputTransformation = wlOutput->transform(); + } m_lastWorkingState.valid = true; } m_pageFlipPending = true; diff --git a/plugins/platforms/drm/drm_output.h b/plugins/platforms/drm/drm_output.h index a8ec22e54..62b6aa58b 100644 --- a/plugins/platforms/drm/drm_output.h +++ b/plugins/platforms/drm/drm_output.h @@ -153,6 +153,7 @@ private: Qt::ScreenOrientation orientation; drmModeModeInfo mode; DrmPlane::Transformations planeTransformations; + KWayland::Server::OutputInterface::Transform outputTransformation; QPoint globalPos; bool valid = false; } m_lastWorkingState;
Thanks for looking into this. But I don't think there is a (good) solution possible without some principle changes. For example wayland output device transformation is set in the Drm backend but should be set on AbstractWaylandOutput level.
Overall I want to have the following structure:
- Platform receives configuration change request.
- Platform implementation tries HW transformation (in Drm case via AMS test commit).
- If it fails optionally tries composited transformation.
- Afterwards the Platform will return configuration change request's succeeded or failed event and sets output device properties accordingly.
That's something I had in the back of my mind already for some time but since it was not possible for 5.17 anymore I worked on other tasks for now. Will be part of T11459.
Planned changes look good to me, but I am trying to understand why current code is failing the way it is failing (even if we don't fix this in end is fine. but as a learning exercise I am trying to fix this)
I've some questions and more debug information, if you want to answer them.
When looking at this further, our atomic commit fails with ENOSPC, and looking at dmesg, I found following,
[ 132.351603] [drm:drm_atomic_check_only [drm]] [PLANE:30:plane 1A] invalid source coordinates 1920.000000x1080.000000+0.000000+0.000000 (fb 1080x1920)
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_atomic.c#L596
So basically we are trying to fit a 1920x1080 source in 1080x1920. Do I understand this correctly one should also adjust the values of src_x, src_y in addition to just transformation property as we are doing here?
void DrmPlane::setTransformation(Transformations t) { if (auto property = m_props.at(int(PropertyIndex::Rotation))) { property->setValue(int(t)); } }
I found some code in the weston, https://gitlab.freedesktop.org/wayland/weston/blob/master/libweston/backend-drm/state-helpers.c#L165 which does something to that effect, are we missing this code in kwin? or I am missing something else? 😛
So what happens when the user clicks the 90 and 270 degree rotation icons in the KScreen KCM with this change applied? Nothing? If so, I would recommend disabling or remove those buttons instead. You don't want to have UI elements that do nothing.
@romangg Did you look into last comment? Commit 26cdfd317fc87b024ebcd9c2adefc86cfd59504e was added to recover from the broken state of rotation/bad atomic commit. It seems that this code has regressed (atomic commit related code), we are not reverting back to correct plane (or output size), explicitly setting the correct mode using kscreen-doctor does recover from broken state/hang.
Which comment? The current fallback code on AMS is incorrect either way because we don't relay this information to KWayland output configuration. That's once again a larger project.
It does nothing. Which should allow the user to conclude that it's not working at the moment.
https://phabricator.kde.org/D24112#538668
The current fallback code on AMS is incorrect either way because we don't relay this information to KWayland output configuration. That's once again a larger project.
I am confused because this did work (even if code is fundamentally wrong maybe) at least during 5.16 release. Where if on my dell laptop I tried to rotate my screen to 90°, it would flick my screen but it would instantly then revert back to 0°. So clearly there's some regression somewhere, unfortunately not one which can be bisected.
I don't know. I try to not lose too much time here but rather spend it in the future to improve it overall.