Fix software cursors with drm backend
ClosedPublic

Authored by ekurzinger on Jan 18 2019, 10:48 PM.

Details

Summary

If hardware cursor support is not available when using the drm backend for Wayland compositing, the software cursor texture will not be updated when the cursor image changes, and it will still be drawn when no cursor image is set (such as when running a full-screen game). Furthermore, the drmModeSetCursor and drmModeMoveCursor functions will still be unnecessarily called when the cursor is moved or hidden.

To correct this, SceneOpenGL should connect Platform::cursorChanged as opposed to Cursor::cursorChanged to its texture update function, as only the former will be emitted when the cursor is updated and the compositor should check if the cursor is hidden and the software cursor image is not null before rendering it. DrmBackend::moveCursor and DrmBackend::hideCursor should also return immediately if using a software cursor.

Test Plan

Run kwin_wayland using the drm backend with the environment variable KWIN_FORCE_SW_CURSOR set.

  • The cursor image should change appropriately when moving / resizing windows, entering text, ect.
  • The cursor should not be rendered if running a full screen game or other application that hides it

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.
ekurzinger created this revision.Jan 18 2019, 10:48 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 18 2019, 10:48 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
ekurzinger requested review of this revision.Jan 18 2019, 10:48 PM

Can you apply for a dev account, there should be an option on identity.kde.org put my name davidedmundson@kde.org as an approver.

plugins/platforms/drm/drm_backend.cpp
681

I don't think that's the layer as the software cursor should work on all platforms.

We should be able to update the software cursor in
Platform::showCursor Platform::hideCursor

davidedmundson added inline comments.Jan 19 2019, 1:15 PM
plugins/platforms/drm/drm_backend.cpp
681

*that's the right layer

Thanks for taking a look, David. Regarding your suggestion, do you think it would make more sense then to connect Platform::cursorChanged to the texture update function in scene_opengl.cpp instead of Cursor::cursorChanged? I'm not entirely clear on the distinction between those two signals, but the former does get emitted when the cursor changes while it seems the latter does not at the moment.

ekurzinger updated this revision to Diff 50575.Jan 30 2019, 6:36 PM
ekurzinger edited the summary of this revision. (Show Details)

Instead of emitting cursorChanged from DrmBackend, modify SceneOpenGL to connect Platform::cursorChanged as opposed to Cursor::cursorChanged to the cursor texture update function.

Instead of emitting cursorChanged from DrmBackend, modify SceneOpenGL to connect Platform::cursorChanged as opposed to Cursor::cursorChanged to the cursor texture update function.

Where does the platform emit cursorChanged?
On wayland the only place I can see that emits Platform::cursorChanged is connected to CursorImage::changed

The rest of the changes look sensible.

Instead of emitting cursorChanged from DrmBackend, modify SceneOpenGL to connect Platform::cursorChanged as opposed to Cursor::cursorChanged to the cursor texture update function.

Where does the platform emit cursorChanged?
On wayland the only place I can see that emits Platform::cursorChanged is connected to CursorImage::changed

Yes, the signal gets emitted by CursorImage::changed(), which itself is triggered by PointerInterface::cursorChanged() from KWayland. It looks like Cursor::cursorChanged() is basically just a wrapper around Platform::cursorChanged() that can be enabled with Cursor::startCursorTracking() which appears to only be used by the zoom effect at the moment. I thought that since the rest of this function interfaces with the software cursor directly through Platform's methods, connecting to Platform::cursorChanged() directly made the most sense.

davidedmundson accepted this revision.Mar 19 2019, 9:35 AM

Ah, so the connect line is just a code tidy up not trying to be an important behavioural change. I misunderstood.

Ship it.

This revision is now accepted and ready to land.Mar 19 2019, 9:35 AM
This revision was automatically updated to reflect the committed changes.