:(

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?

In D24112#536872, @zzag wrote:

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?

Haven't investigated it much further yet. I tried it on i915.

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.

https://github.com/ascent12/drm_info

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.

In D24112#536899, @romangg wrote:

This diff is not meant to fix anything. It just ensures users can not render the session unusable accidentally by changing rotation.

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.

In D24112#538658, @romangg wrote:

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:

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? 😛

@jriddell this needs to go into 5.17 still.

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.

In D24112#546346, @bshah wrote:

@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.

In D24112#546321, @ngraham wrote:

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.

It does nothing. Which should allow the user to conclude that it's not working at the moment.

In D24112#546605, @romangg wrote:

In D24112#546346, @bshah wrote:

@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?

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.

In D24112#546630, @bshah wrote:

In D24112#546605, @romangg wrote:

In D24112#546346, @bshah wrote:

@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?

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.

In D24112#546668, @romangg wrote:

I don't know. I try to not lose too much time here but rather spend it in the future to improve it overall.

Agreed.