Introduce a display subsystem for GPU output #319

mtjhrc · 2025-04-23T13:31:23Z

This PR adds initial support for the more traditional display output from the VM by basically having virtual displays (until now we only had cross-domain/wayland).

This implementes 2 display backends:

DisplayBackendNoop - used by default, pretty much returns an error for every operation
DisplayBackendGtk - implementation using GTK (works on Linux, to be tested on macOS)

This introduces 2 new public API function:

int32_t krun_set_display_backend_gtk(uint32_t ctx_id)
int32_t krun_set_display(uint32_t ctx_id, uint32_t display_id, uint32_t width, uint32_t height)

Current limitations to be addressed by future PRs:

add a virtio-input device and forward the events from the window to the guest
improve the performance by using dmabufs instead copying it to the CPU
send display resize events to the guest

You can test this using the newly added --display option in chroot_vm and running something graphical in the guest - the simplest option is probably kmscube:
$ ./chroot_vm --display=0:1920:1080 rootfs_fedora kmscube

This is similar concept to the mpsc::channel but uses an EventFd to notify the reader when data is availible. This seems like it could be generally useful, when we need to use Epoll to also wait for other file descriptor events. This is just a simple implementation using EventFd and could maybe be optimized. (maybe even using a unix pipe directly) Signed-off-by: Matej Hrica <[email protected]>

We wrongly expected an extra virtio_gpu_ctrl_hdr for VIRTIO_GPU_CMD_GET_DISPLAY_INFO The command doesn't have any body and we have already read the ctrl_header. Signed-off-by: Matej Hrica <[email protected]>

Introduce a DisplayBackend trait and a DisplayBackendNoop implementation. Hook up the gpu device to use the display backend. For now this only hooks up the simplest and slowest option of display output - the output is copied to the CPU using the transfer_read operation and passed to the display backend. The DisplayBackendNoop doesn't provide any displays, so this doesn't display anything yet. Introduce a new public API function krun_set_display() for configuring VM displays. The virtio-gpu code is based on rust-vmm/vhost-device-gpu. Signed-off-by: Matej Hrica <[email protected]>

Add a DisplayBackend implementation using GTK. This also introduces a new public API function krun_set_display_backend_gtk() for enabling the gtk display backend. Signed-off-by: Matej Hrica <[email protected]>

The --display option can be provided multiple times. Specyfing at least one display automatically enables the GTK display backend. Signed-off-by: Matej Hrica <[email protected]>

slp · 2025-04-30T12:46:05Z

On macOS I'm getting this:

thread 'gtk display' panicked at /Users/slopezpa/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/gtk4-0.9.6/src/rt.rs:106:9:
assertion `left != right` failed: Attempted to initialize GTK on OSX from non-main thread
  left: 0
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I haven't investigated further yet.

jakecorrenti

I can't speak to the logic of the subsystem since I'm not familiar with what's best practices here, but the code itself LGTM.

Could use a rebase, though.

bilelmoussaoui

some comments here and there

bilelmoussaoui · 2025-05-13T13:07:33Z

src/devices/src/display/gtk/worker.rs

+
+fn build_overlay(window: &Window) -> Overlay {
+    let overlay_bar = HeaderBar::builder()
+        .visible(true)


Suggested change

.visible(true)

all widgets are visible by default

bilelmoussaoui · 2025-05-13T13:07:47Z

src/devices/src/display/gtk/worker.rs

+
+    let overlay_unfullscreen_btn = Button::builder()
+        .tooltip_text("Exit fullscreen mode")
+        .icon_name("view-restore")


where this icon comes from?

I looked at this list, and picked icon names: https://specifications.freedesktop.org/icon-naming-spec/latest/#names
(but looking at it again, it seems this is from 2007, so I'm not sure if I should be rely on this 😄 )
Is it a bad assumption, that these icons will always exist?

bilelmoussaoui · 2025-05-13T13:08:50Z

src/devices/src/display/gtk/worker.rs

+
+pub fn gtk_display_main_loop(rx: PollableChannelReciever<DisplayEvent>, displays: DisplayInfoList) {
+    gtk4::init().expect("Failed to initialize GTK");
+    let main_loop = glib::MainLoop::new(None, false);


any reason for running a manual main loop instead of using gtk::Application?

Well, originally I also wanted to also use a custom MainContext and only use gtk from this worker thread. This is technically a library and theoretically other glib/gtk things could be running (but more of a theoretical than a real problem I guess). I feel like the Rust bindings always want you to use the global context anyway (for example the source::unix_fd_add_local), or maybe I just don't understand, how to use it properly. I'm not really familiar with glib/gtk C API anyway.

But it looks like this might be a moot point anyway, because on macOS it seems gtk has to be initialized on the main thread anyway...

bilelmoussaoui · 2025-05-13T13:10:07Z

src/devices/Cargo.toml

@@ -29,6 +30,8 @@ virtio-bindings = "0.2.0"
 vm-memory = { version = ">=0.13", features = ["backend-mmap"] }
 zerocopy = { version = "0.6.3", optional = true }
 zerocopy-derive = { version = "0.6.3", optional = true }
+gtk4 = { version = "0.9.6", features = ["v4_14"], optional = true }
+glib = { version = "0.20.9", optional = true }


it is already pulled by gtk4, no need for a specific dep.

bilelmoussaoui · 2025-05-13T13:10:39Z

src/devices/src/display/gtk/worker.rs

+        height: i32,
+        format: gdk::MemoryFormat,
+    ) -> Self {
+        let header_bar = HeaderBar::builder().build();


Suggested change

let header_bar = HeaderBar::builder().build();

let header_bar = HeaderBar::default();

is enough i guess

bilelmoussaoui · 2025-05-13T13:10:52Z

src/devices/src/display/gtk/worker.rs

+            // Enforce a minimum window size:
+            .height_request(64)
+            .titlebar(&header_bar)
+            .visible(true)


same as other widgets

bilelmoussaoui · 2025-05-13T13:11:43Z

src/devices/src/display/gtk/worker.rs

+            .build();
+
+        let actions = SimpleActionGroup::new();
+        actions.add_action_entries([


why use a separate action group ? i would recommend using a gtk::Window which is an gio::ActionMap already

bilelmoussaoui · 2025-05-13T13:12:33Z

src/devices/src/display/gtk/worker.rs

+        ]);
+        window.insert_action_group("scanout", Some(&actions));
+
+        let fullscreen_btn = gtk4::Button::builder()


weird that some types are imported, some are not ;-)

bilelmoussaoui · 2025-05-13T13:13:11Z

src/devices/src/display/gtk/worker.rs

+
+        // Unset the width/height after the window is created to allow resizing the window to
+        // be smaller
+        picture.set_size_request(-1, -1);


that looks like a hack.

Yes it is. Any idea how to do this properly, though?

I wanted to set the default window width/height, I see Window.set_default_size exits, but what to set the dimensions to? The window height should be the height of the image + height of the header bar, which I don't know how to obtain, and even if I did calculating this manually feels even more hacky and error prone.

I don't see a way to set the default, but not minimal dimensions of the Picture widget (which would automatically create the window with correct size).

You will have to create a custom paintable, see https://github.com/gtk-rs/gtk4-rs/tree/main/examples/custom_paintable or other custom paintables examples.

bilelmoussaoui · 2025-05-15T07:04:43Z

src/devices/src/display/gtk/worker.rs

+use gtk4::{
+    gdk,
+    gio::{ActionEntry, Cancellable, SimpleActionGroup},
+    prelude::{ActionMapExtManual, GtkWindowExt, WidgetExt},


Suggested change

prelude::{ActionMapExtManual, GtkWindowExt, WidgetExt},

prelude::*,

as stuff can move from ExtManual to Ext with time, so avoid potential code fixing with version bumps

bilelmoussaoui · 2025-05-15T07:06:16Z

src/devices/src/display/gtk/worker.rs

+fn build_kill_vm_btn() -> Button {
+    let kill_vm_btn = Button::builder()
+        .label("Kill VM")
+        .action_name("scanout.kill")
+        .build();
+    kill_vm_btn.set_css_classes(&["destructive-action"]);
+    kill_vm_btn
+}


seems too much to do this in a separate function. Also use add_css_class, the function you are using removes all the pre-existing CSS classes in the widget if any.

mtjhrc force-pushed the gtk-display branch 10 times, most recently from 38c4a1a to 7f463ef Compare April 29, 2025 11:11

mtjhrc marked this pull request as ready for review April 29, 2025 11:58

mtjhrc requested review from jakecorrenti, MatiasVara, slp and tylerfanelli as code owners April 29, 2025 11:58

mtjhrc force-pushed the gtk-display branch from 7f463ef to 226e8ea Compare April 29, 2025 13:54

mtjhrc added 5 commits April 30, 2025 10:30

gpu: Fix GetDisplayInfo command decoding

3255a04

We wrongly expected an extra virtio_gpu_ctrl_hdr for VIRTIO_GPU_CMD_GET_DISPLAY_INFO The command doesn't have any body and we have already read the ctrl_header. Signed-off-by: Matej Hrica <[email protected]>

display: Introduce GTK display backend

0f5aecd

Add a DisplayBackend implementation using GTK. This also introduces a new public API function krun_set_display_backend_gtk() for enabling the gtk display backend. Signed-off-by: Matej Hrica <[email protected]>

examples/chroot_vm: Add --display parameter for attaching displays

102c12f

The --display option can be provided multiple times. Specyfing at least one display automatically enables the GTK display backend. Signed-off-by: Matej Hrica <[email protected]>

mtjhrc force-pushed the gtk-display branch from 226e8ea to 102c12f Compare April 30, 2025 08:30

jakecorrenti reviewed May 8, 2025

View reviewed changes

bilelmoussaoui reviewed May 13, 2025

View reviewed changes

bilelmoussaoui reviewed May 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a display subsystem for GPU output #319

Introduce a display subsystem for GPU output #319

mtjhrc commented Apr 23, 2025

slp commented Apr 30, 2025

jakecorrenti left a comment •

edited

Loading

bilelmoussaoui left a comment

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

mtjhrc May 13, 2025

bilelmoussaoui May 13, 2025

mtjhrc May 13, 2025

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

bilelmoussaoui May 13, 2025

mtjhrc May 13, 2025 •

edited

Loading

bilelmoussaoui May 14, 2025

bilelmoussaoui May 15, 2025

bilelmoussaoui May 15, 2025

	let header_bar = HeaderBar::builder().build();
	let header_bar = HeaderBar::default();

	prelude::{ActionMapExtManual, GtkWindowExt, WidgetExt},
	prelude::*,

Introduce a display subsystem for GPU output #319

Are you sure you want to change the base?

Introduce a display subsystem for GPU output #319

Conversation

mtjhrc commented Apr 23, 2025

slp commented Apr 30, 2025

jakecorrenti left a comment • edited Loading

Choose a reason for hiding this comment

bilelmoussaoui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtjhrc May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecorrenti left a comment •

edited

Loading

mtjhrc May 13, 2025 •

edited

Loading