Initial kmsdrm support [forked + updated] #2795
Conversation
|
|
| kms = ["drm", "input", "xkbcommon", "udev", "parking_lot"] | ||
| kms-ext = ["libseat"] |
There was a problem hiding this comment.
Is it still necessary for split the feature into kms and kms-ext like this? I forget what the original rationale for this was.
There was a problem hiding this comment.
kms-ext uses (libseat)[https://sr.ht/~kennylevinsen/seatd/].
Seat management takes care of mediating access to shared devices (graphics, input), without requiring the applications needing access to be root.
So this lib links/uses existing methods such as logind to create a user "seat" and allow access to various things. The lib is also a good intermediate step between winit and systemd-logind (plus others).
In our case this would allow anyone using this feature to run via TTY without requiring root. Standard kms here does require root access.
Unfortunately this feature is broken in this PR.
❯ cargo run --example window --no-default-features --features=kms-ext
Compiling libseat-sys v0.1.7
Compiling slog v2.7.0
Compiling libseat v0.1.7
Compiling winit v0.28.4 (/home/luke/Projects/winit-drm)
error: The platform you're compiling for is not supported by winit
--> src/platform_impl/mod.rs:68:1
|
68 | compile_error!("The platform you're compiling for is not supported by winit");
| input = { version = "0.7.1", optional = true } | ||
| libseat = { version = "0.1.4", optional = true } | ||
| udev = { version = "0.6.3", optional = true } | ||
| xkbcommon = { version = "0.5.0", optional = true } |
There was a problem hiding this comment.
It might be worth it to just use xkbcommon-dl instead, since that's what #2789 is using.
| pub struct EventLoopWindowTarget<T> { | ||
| /// Drm Connector | ||
| pub connector: connector::Info, | ||
|
|
||
| /// Drm crtc | ||
| pub crtc: crtc::Info, | ||
|
|
||
| /// Drm plane | ||
| pub plane: plane::Handle, | ||
|
|
||
| /// Allows window to edit cursor position | ||
| pub(crate) cursor_arc: Arc<Mutex<PhysicalPosition<f64>>>, | ||
|
|
||
| /// Drm device | ||
| pub device: Card, | ||
|
|
||
| /// Event loop handle. | ||
| pub event_loop_handle: calloop::LoopHandle<'static, EventSink>, | ||
|
|
||
| pub(crate) event_sink: EventSink, | ||
|
|
||
| /// A proxy to wake up event loop. | ||
| pub event_loop_awakener: calloop::ping::Ping, | ||
|
|
||
| _marker: std::marker::PhantomData<T>, | ||
| } |
There was a problem hiding this comment.
It would be nice if all of the fields were either pub or pub(crate), instead of this mix.
| let mut enumerator = Enumerator::new().map_err(|e| { | ||
| os_error!(OsError::KmsError(format!( | ||
| "failed to open udev enumerator: {}", | ||
| e | ||
| ))) | ||
| })?; |
There was a problem hiding this comment.
Might be nice to use a combinator like we have in softbuffer to avoid duplicating this map_err block`.
| Ok(PostAction::Continue) | ||
| } | ||
|
|
||
| fn register(&mut self, poll: &mut Poll, factory: &mut TokenFactory) -> calloop::Result<()> { |
There was a problem hiding this comment.
Maybe instead of using poll directly, we can wrap the libinput handling in the calloop::Generic type and use that to handle things?
| /// Implementing `AsRawFd` is a prerequisite to implementing the traits found | ||
| /// in this crate. Here, we are just calling `as_raw_fd()` on the inner File. | ||
| impl unix::io::AsRawFd for Card { | ||
| fn as_raw_fd(&self) -> unix::io::RawFd { | ||
| *self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Modern versions of the drm crate use AsFd, maybe we should use that instead?
| impl Drop for Card { | ||
| fn drop(&mut self) { | ||
| unsafe { std::fs::File::from_raw_fd(*self.0) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't look sound to me. Since the i32 can still be used outside of the Card (thanks to AsRawFd), couldn't it still be used after a free? It might be better to define Card as a wrapper around an OwnedFd and then wrap that in an Arc.
There was a problem hiding this comment.
Yeah, this isn't right because drop is closing this every time you drop a Card, even if you cloned it. The dropping needs to be within the Arc.
Winit currently requires Rust 1.64.0, so it would be correct and require less code to use struct Card(Arc<OwnedFd>).
| pub fn new<T>( | ||
| event_loop_window_target: &super::event_loop::EventLoopWindowTarget<T>, | ||
| _attributes: WindowAttributes, | ||
| _platform_attributes: platform_impl::PlatformSpecificWindowBuilderAttributes, | ||
| ) -> Result<Self, error::OsError> { |
There was a problem hiding this comment.
Might be worth considering #33 here. Maybe, if a window has already been created, it's better to just return an error instead of returning another window?
| @@ -204,13 +226,15 @@ pub enum MonitorHandle { | |||
| /// } | |||
| /// ``` | |||
| /// The result can be converted to another enum by adding `; as AnotherEnum` | |||
| macro_rules! x11_or_wayland { | |||
| macro_rules! x11_or_wayland_or_drm { | |||
There was a problem hiding this comment.
Especially if we add more backends in the future, it might be better to change the name of this (maybe platform_specific!)?
|
I don't mean to be a bother, but is there a timeframe for merging this? It's exactly what I'm looking for to be used with slint and in embedded environment (STM32MP157). I can test this later on. |
No timeline for changes like that, you might look into smithay project instead if you want to. It's not only about wayland in the end of the day. |
Yeah been looking at a few options. It would be great to get this in winit as it means a few projects would gain it for free. SDL2 also has a similar DRM method and I've used that for my room4doom (as example). |
The reason I wondered about this is because if winit implements KMS backend, then many many other projects gain KMS ability for free. Especially nice for GUI toolkits in embedded applications. |
|
Closing due to inactivity. |
I forked #2272 and updated it to resolve merge conflicts and so it compiles with the current dependencies.
CHANGELOG.mdif knowledge of this change could be valuable to users