winit icon indicating copy to clipboard operation
winit copied to clipboard

Initial kmsdrm support

Open StratusFearMe21 opened this issue 3 years ago • 41 comments
trafficstars

  • [x] Tested on all platforms changed
  • [x] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • [x] Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • [x] Created or updated an example program if it would help users understand this functionality
  • [x] Updated feature matrix, if new features were added or implemented

What's this?

This is the start of a kmsdrm backend for winit! Essentially, if wayland and X.org are not available, this PR makes winit create a gbm device and a libinput listener to send the device window events.

KMS/DRM

KMS/DRM is the system that wayland compositors and X use to draw things to the screen. It's an abstraction over the graphics card essentially. In order to render a framebuffer to the screen, you must

  1. Open the global DRM file descriptor
  2. Select a connector (monitor)
  3. Select a CRTC
  4. Add a framebuffer
  5. Add a plane
  6. Commit all previous changes to the DRM file descriptor

My PR does steps 1-4 and 6. Steps 5 is where the user will come in and initialize a framebuffer for their graphics library of choice.

Event Handling

KMS/DRM mode does not have a traditional input system, in fact it has none at all. It requires that you grab the raw input file descriptors from /dev/input and read those. This is where libinput comes in. This is the same library that smithay and many wayland compositors use to complete this task. It even provides an event based interface which integrates very well with winit. I chose this library because it melds very well with winit, and it handles every type of input that winit supports out of the box, however

  • I heard talk in #1006 about using a Rust crate to handle raw input, libinput is not written in Rust
  • libinput supports multi-touch given that you are not performing a gesture. In this case libinput will produce gesture events and only tell you when the gesture begins, moves, and ends. These gestures do not provide finger ids either.
  • The way that my backend is implemented, it produces both WindowEvents and DeviceEvents even if a window has not been initialized.

Also, I should probably mention, there is an invisible virtual cursor that my KMS/DRM backend simulates which takes deltas from libinput and produces DeviceEvents for the raw mouse deltas and WindowEvents for the virtual location of the cursor on screen.

It it possible to access the gbm device mutex via the gbm_device(), the current connector via gbm_connector() and the current crtc via the gbm_crtc() method in the EventLoopWindowTargetExtUnix trait

Note

  • I'm positively stumped on what to do about these warnings
warning: use of deprecated field `event::KeyboardInput::modifiers`: Deprecat
ed in favor of WindowEvent::ModifiersChanged
   --> src/platform_impl/linux/drm/event_loop.rs:529:88
    |
529 | ...                   }, virtual_keycode: CHAR_MAPPINGS[k as usize], m
odifiers: self.modifiers } , is_synthetic: false }}, &mut ());
    |                                                                      ^
^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: associated function is never used: `dummy`
  --> src/platform_impl/linux/drm/mod.rs:49:25
   |
49 |     pub const unsafe fn dummy() -> Self {
   |                         ^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: associated function is never used: `dummy`
   --> src/platform_impl/linux/drm/mod.rs:138:25
    |
138 |     pub const unsafe fn dummy() -> Self {
    |                         ^^^^^

warning: `winit` (lib) generated 3 warnings

In the future

I could add an actual cursor and render it to the screen using the cursor framebuffer.

Thanks for considering my PR!

StratusFearMe21 avatar Apr 29 '22 02:04 StratusFearMe21

Wow! This is neat. I'll be sure to look closer at this when I can, but for now, I just have to note that I think your editor's autoformatting bungled up our CHANGELOG.md, FEATURES.md (all instances of - for bullet-points have become *), and Cargo.toml (4-space indentation has become tab-based indentation).

maroider avatar Apr 29 '22 02:04 maroider

Wow! This is neat. I'll be sure to look closer at this when I can, but for now, I just have to note that I think your editor's autoformatting bungled up our CHANGELOG.md, FEATURES.md (all instances of - for bullet-points have become *), and Cargo.toml (4-space indentation has become tab-based indentation).

Whoops! Sorry about that. I'm using remark and taplo for my formatting, I'll go back and fix the formatting on the documents you mentioned

StratusFearMe21 avatar Apr 29 '22 03:04 StratusFearMe21

Cargo.toml and FEATURES.md both still have what looks like unintentional formatting changes.

maroider avatar May 03 '22 07:05 maroider

Sorry about that, I fixed the formatting again

StratusFearMe21 avatar May 03 '22 11:05 StratusFearMe21

Forget about the raw-window-handle issue, I've confirmed myself that gbm is indeed thread-safe. You'll see the PR I made to raw-window-handle above

Any system level (syscall) file descriptor access is thread safe in all mainstream UNIX-like OSes. Though depending on the age they are not necessarily signal safe.

If you call read, write, accept or similar on a file descriptor from two different tasks then the kernel's internal locking mechanism will resolve contention.

For reads each byte may be only read once though and writes will go in any undefined order.

The stdio library functions fread, fwrite and co. also have by default internal locking on the control structures, though by using flags it is possible to disable that.

(I don't know what being "signal safe" is, but I don't think it matters)

StratusFearMe21 avatar May 03 '22 12:05 StratusFearMe21

One of the maintainers of drm-rs/gbm.rs/input.rs/smithay here, this looks very well put together and I cannot spot any obvious mistakes. (But obviously I don't know a lot about winit internals.)

One nitpick: The mode selection is likely copied from some outdated example code. drm-rs gained the option to read out mode-types recently. Most monitors have a "Preferred" mode, which is likely what winit should select. (https://docs.rs/drm/latest/drm/control/struct.Mode.html#method.mode_type).

Additionally, I think there was an earlier attempt to do something like this for glutin, I think(?): https://github.com/Smithay/gbm.rs/pull/2 I am not in the loop at the great scheme of things here, but if there is any way, I can help with reviewing or adjusting the code of any of these libs, please let me know. I would be glad to support such an endeavour, even if I don't have much time to write code for it myself. Given our wayland-compositor project smithay might also contain some code, that could be useful in the future, I would like to avoid code duplication where possible and rather build a shared foundation, if that is of any interest. I also see it as a net positive, that drm-rs/gbm.rs/input.rs might get some more usage in the future with this implementation.

Thanks for working on this! :+1:

Drakulix avatar May 03 '22 14:05 Drakulix

One nitpick: The mode selection is likely copied from some outdated example code.

Yeah, guilty. That should be fixed now

StratusFearMe21 avatar May 03 '22 14:05 StratusFearMe21

@Drakulix, Is it possible to capture the keyboard with libinput? Currently all the key presses that go to the window are passed to the tty, and that just doesn't seem ideal.

StratusFearMe21 avatar May 03 '22 17:05 StratusFearMe21

@Drakulix, Is it possible to capture the keyboard with libinput? Currently all the key presses that go to the window are passed to the tty, and that just doesn't seem ideal.

If you are running directly from a tty, you would need to set the tty into graphical mode to be able to receive keyboard events. Usually this is done by using proper session management.

You have two options here:

  • Either you do the manual approach and do the ioctls directly.
    • e.g. see smithay's direct session backend: https://github.com/Smithay/smithay/blob/master/src/backend/session/direct.rs#L279-L340).
    • In that case you need to make sure (e.g. by wrapping everything in a catch_unwind, that you reset the mode on exit. Otherwise it might happen, that the tty remains unusable.
    • It also does not integrate super well into calloop, you need to setup a signal and get a signal fd from that to add it to calloop. (If you want to do support vt-switches at all, that is.)
  • Or you rely on some library to do that for you, I would recommend libseat for the job.
    • This obviously adds another dependency, but I would recommend it, because...
    • ... the kernel vt-api is considered legacy and libseat will only fall back to it if no other session daemon (e.g. systemd-logind, elogind, seatd) is running on the system (but that is all handled at runtime).
    • ... you can properly open the devices using the session, which means winit could run unprivileged.
    • ... you get callbacks, when the user switches sessions (e.g. on a vt-switch) and stop rendering / event handling temporarily, instead of likely erroring out and panicking.
    • (smithay will also likely switch to libseat exclusively at some point in the future.)
    • Bindings: https://github.com/PolyMeilex/libseat-rs
    • smithay backend reference code: https://github.com/Smithay/smithay/blob/master/src/backend/session/libseat.rs

Drakulix avatar May 03 '22 17:05 Drakulix

Thanks for the input, I'll start with libseat

StratusFearMe21 avatar May 03 '22 18:05 StratusFearMe21

libseat works like a champ, and on top of that integrates flawlessly with libinput. Thanks for the tip (reference code was very helpful)

StratusFearMe21 avatar May 03 '22 20:05 StratusFearMe21

@Drakulix, this PR is very close to being feature compete. All it needs is a propper cursor, however, no matter what I try, I can't get a cursor buffer/plane working. Is their some reference I can look at for how to make and use the DRM cursor plane?

StratusFearMe21 avatar May 04 '22 04:05 StratusFearMe21

@Drakulix, this PR is very close to being feature compete. All it needs is a propper cursor, however, no matter what I try, I can't get a cursor buffer/plane working. Is their some reference I can look at for how to make and use the DRM cursor plane?

I don't think we have a single compositor, that makes use of the cursor plane, right now.. (they used to, but not since we switched to the atomic api).

The code in smithay to support this use-case does exist however: https://github.com/Smithay/smithay/blob/master/src/backend/drm/surface/atomic.rs#L870-L917 . It should be enough to just add those to your AtomicModeReq.

A thing you might be missing is, that the kernel expects SRC_W and SRC_H in 16.16 fixed point values: https://github.com/Smithay/smithay/blob/master/src/backend/drm/surface/atomic.rs#L886

Another is that, as far as I know, there is no guarantee, that a dumb-buffer will work for cursor planes (depending on the driver). So you might be better of using gbm for that. (You can request a writable gbm-buffer, so you don't need a GL context to render to it: https://docs.rs/gbm/latest/gbm/struct.BufferObjectFlags.html#associatedconstant.WRITE)

Drakulix avatar May 04 '22 08:05 Drakulix

One nitpick: I've generally heard of this as DRM/KMS since KMS is afaik part of the DRM subsystem and not the other way around: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html.

As such, would it make sense to s/kmsdrm/drmkms or is there some underlying reason? If this has already been documented/discussed, apologies!

MarijnS95 avatar May 04 '22 10:05 MarijnS95

One nitpick: I've generally heard of this as DRM/KMS since KMS is afaik part of the DRM subsystem and not the other way around: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html.

As such, would it make sense to s/kmsdrm/drmkms or is there some underlying reason? If this has already been documented/discussed, apologies!

The reason I do it like this is because thats how SDL2 does it https://wiki.libsdl.org/FAQUsingSDL. But of course, I'd be happy to s/kmsdrm/drmkms if that isnt a good enough reason.

StratusFearMe21 avatar May 04 '22 10:05 StratusFearMe21

Another is that, as far as I know, there is no guarantee, that a dumb-buffer will work for cursor planes (depending on the driver). So you might be better of using gbm for that.

Oof, I just removed gbm as a dependency on this package and made it only required in glutin. I did this because Vulkan uses an entirely different system than GBM for framebuffers and such. I guess its not a big deal that this implementation has no visible cursor

StratusFearMe21 avatar May 04 '22 10:05 StratusFearMe21

@Drakulix, libseat works very well, however, it seems to supress the app from printing anything. Is there a way to fix this?

P.S: thanks for being so helpful on this PR.

StratusFearMe21 avatar May 04 '22 12:05 StratusFearMe21

@Drakulix, libseat works very well, however, it seems to supress the app from printing anything. Is there a way to fix this?

P.S: thanks for being so helpful on this PR.

Not really, sadly. The graphical tty-mode suppresses any log messages getting printed to the tty and the kernel does not keep a buffer of "missed" messages.

Drakulix avatar May 04 '22 12:05 Drakulix

@Drakulix, libseat works very well, however, it seems to supress the app from printing anything. Is there a way to fix this? P.S: thanks for being so helpful on this PR.

Not really, sadly. The graphical tty-mode suppresses any log messages getting printed to the tty and the kernel does not keep a buffer of "missed" messages.

I see, thanks for responding so quickly

StratusFearMe21 avatar May 04 '22 12:05 StratusFearMe21

I'm pretty confident with this PR at this point. As a functionality test, I ported Alacritty to this PR and now it runs straight from the tty https://github.com/StratusFearMe21/alacritty

StratusFearMe21 avatar May 05 '22 19:05 StratusFearMe21

@Drakulix, If I have an xkbcommon context, how do I get the default options for what the keymap should be? If I put empty strings for all the options, it compiles the keymap but it doesn't seem to compile the right one.

StratusFearMe21 avatar May 05 '22 22:05 StratusFearMe21

Nevermind, it must be something on my end

StratusFearMe21 avatar May 05 '22 22:05 StratusFearMe21

Okay, I fixed the issue, logical error

StratusFearMe21 avatar May 05 '22 23:05 StratusFearMe21

I'm pretty confident with this PR at this point. As a functionality test, I ported Alacritty to this PR and now it runs straight from the tty https://github.com/StratusFearMe21/alacritty

I also gave these changes a try with Slint and it works out of the box. Amazing work @StratusFearMe21!

tronical avatar May 06 '22 13:05 tronical

I also gave these changes a try with Slint and it works out of the box. Amazing work @StratusFearMe21!

Yay! I'm glad everything's working!

StratusFearMe21 avatar May 06 '22 13:05 StratusFearMe21

Yeah, at this point, I think this is ready to be merged. Im very happy with this PR in its current state.

StratusFearMe21 avatar May 07 '22 00:05 StratusFearMe21

 Alrighty, thanks for reviewing my PR. I've never made a PR to such a high-profile crate before, so sorry that my formatting is a little inconsistent. It's a little disappointing that DRM can't be a default backend, I was hoping we could do it like how SDL2 does it where it is possible to run apps using the kmsdrm video driver, and whether the app cares or not didn't matter. Anyhoo, I'll begin this refactor tomorrow

StratusFearMe21 avatar May 07 '22 03:05 StratusFearMe21

I was hoping we could do it like how SDL2 does it where it is possible to run apps using the kmsdrm video driver, and whether the app cares or not didn't matter. Anyhoo, I'll begin this refactor tomorrow

My main concert is that it brings lots of dependencies most users not sure that want to have. Also it's impossible to run it on some nvidia systems due to lack of gbm(some has it now though).

Also, certain stuff like multiwindow isn't possible on it, so applications should explicitly handle it and tune their stuff wrt that. Trying to run and straight crashing due to how limited kms backend is doesn't sound ideal to me.

Though, maybe other winit maintainers/downstream users have better opinion on that topic.

kchibisov avatar May 07 '22 03:05 kchibisov

Yeah, I guess the big weakness in this patch is that it brings in a lot of C dependencies. I agree with you on that.

StratusFearMe21 avatar May 07 '22 03:05 StratusFearMe21

As for GBM support on NVidia GPUs, that shouldn't be a problem

StratusFearMe21 avatar May 07 '22 03:05 StratusFearMe21