[platforms/drm] Disable transformations
ClosedPublic

Authored by romangg on Fri, Sep 20, 12:25 PM.

Details

Summary

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.

Test Plan

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.
romangg created this revision.Fri, Sep 20, 12:25 PM
Restricted Application added a project: KWin. · View Herald TranscriptFri, Sep 20, 12:25 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Fri, Sep 20, 12:25 PM
romangg updated this revision to Diff 66562.Fri, Sep 20, 9:04 PM

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.

romangg retitled this revision from Disable setting output transformation to [platforms/drm] Disable setting output transformation.Fri, Sep 20, 9:05 PM
romangg retitled this revision from [platforms/drm] Disable setting output transformation to [platforms/drm] Disable setting 90° transformations.
zzag added a subscriber: zzag.Tue, Sep 24, 10:58 AM

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.

zzag added a comment.Tue, Sep 24, 11:37 AM

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.

zzag added a comment.Tue, Sep 24, 1:48 PM

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

zzag added a comment.Wed, Sep 25, 11:40 AM

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.

bshah added a subscriber: bshah.Thu, Sep 26, 11:47 AM

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.

bshah added a comment.EditedFri, Sep 27, 1:11 PM

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

romangg updated this revision to Diff 67800.Sat, Oct 12, 8:36 PM

Rebase on 5.17 branch.

romangg updated this revision to Diff 67802.Sat, Oct 12, 8:51 PM
  • Disable all rotation again

@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 retitled this revision from [platforms/drm] Disable setting 90° transformations to [platforms/drm] Disable transformations.Sun, Oct 13, 3:59 AM
bshah added a comment.Sun, Oct 13, 6:23 AM

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

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

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.

bshah added a comment.Sun, Oct 13, 6:02 PM

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Oct 13, 7:52 PM
This revision was automatically updated to reflect the committed changes.
zzag added a comment.Mon, Oct 14, 8:26 AM

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.