feat(gpu): serve the virtio-gpu cursor queue through a display-backend cursor plane - #119
Open
ya-luotao wants to merge 5 commits into
Open
feat(gpu): serve the virtio-gpu cursor queue through a display-backend cursor plane#119ya-luotao wants to merge 5 commits into
ya-luotao wants to merge 5 commits into
Conversation
With VIRGL_RENDERER_NO_VIRGL and no VENUS the host has no 3D renderer at all (macOS without a display server, for instance), yet the device always built a VirglRenderer component. virglrenderer then rejects every non-blob resource (RESOURCE_CREATE_2D, ATTACH_BACKING, TRANSFER_TO_HOST_2D, SET_SCANOUT, FLUSH all fail), so a guest with a KMS scanout never gets a frame to the host. In that mode build a Rutabaga2D component, advertise only VERSION_1 | EDID, and report zero capsets so the guest does not probe virgl either. flush_resource also read the whole resource with the resource stride into a frame buffer sized by the SET_SCANOUT rectangle; when the two differ (Hyprland's dumb buffers), transfer_2d fails and the unwrap took the gpu worker thread down. Record the scanout rectangle, copy only the intersection with the destination stride, and answer ErrUnspec instead of panicking.
A guest that draws its pointer into the scanout costs a full-plane flush per pointer move, because Linux's virtio-gpu driver sets `ignore_damage_clips` whenever the plane's framebuffer changes (`virtgpu_plane.c:91-97`). The device can serve the cursor queue instead and let the backend composite the pointer, but the ABI had nowhere to put it. Add `KRUN_DISPLAY_FEATURE_CURSOR` and two methods, `set_cursor` and `move_cursor`, appended at the end of `krun_display_basic_framebuffer_vtable`. Appending keeps the existing fields at their offsets, so a caller built against the older header can pass a shorter struct whose new fields are NULL and whose feature bit is unset: `verify()` accepts it and the methods report `MethodNotSupported`. That is only the bindings' half of the story. `krun_set_display_backend` currently requires `backend_size` to be at least the *current* struct size and then reads the whole struct, so growing the vtable rejects an old caller before it reaches `verify()`. It needs to accept any `backend_size` down to the original struct size and copy only that many bytes into a zeroed struct; the header and `verify()` document that requirement, and the `an_old_size_struct_is_accepted...` test pins the behaviour this side of it. On the Rust side, `DisplayBackendCursor` is an optional trait next to `DisplayBackendBasicFramebuffer`; a backend implementing it is turned into a backend struct with `into_display_backend_with_cursor`, while `into_display_backend` is unchanged for backends without cursor support. The framebuffer shims moved out of the `IntoDisplayBackend` impl into `basic_framebuffer_vtable` so both paths share them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfZtim8axDRi4BAVLXMLgG
The device dropped the cursor queue at activation and panicked on `UpdateCursor`/`MoveCursor`, so a guest could only draw its pointer into the scanout — which costs a full-plane flush per pointer move. Keep the queue and its event, and have the gpu worker poll both queues instead of blocking on the control event alone. The cursor queue needs no fence machinery and never leaves the worker thread, so it stays a plain queue; Linux's `virtio_gpu_queue_cursor` sends a single out-descriptor with no response buffer, so a response is encoded only when the driver did provide one, and nothing on that path blocks. It also must not be left unserviced: the driver waits for free descriptors and would wedge the guest after a queue's worth of cursor updates. The two control-queue arms now route to the same handlers rather than panicking: a stray cursor command must not take the device down. `update_cursor` reads the cursor resource's pixels back through rutabaga the way a scanout flush does — the guest uploads them with TRANSFER_TO_HOST_2D first — and hands them to the backend's cursor plane, followed by the position; `resource_id == 0` hides the cursor. The image size comes from the resource (the kernel's virtio-gpu cursor plane is 64x64 `DRM_FORMAT_HOST_ARGB8888`, so BGRA on little-endian) and is capped so a bogus resource cannot make the worker allocate something huge. Every failure logs and answers `ErrUnspec`; a backend that never negotiated the cursor feature is not worth an error line per pointer move, since the guest never reads these responses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfZtim8axDRi4BAVLXMLgG
Hyprland's cursor reached the viewer fully opaque, so its transparent surround drew as a black box: of 4096 pixels, 4096 read alpha 255. The guest allocates its cursor as a dumb buffer, and Linux creates the host resource with a hardcoded `DRM_FORMAT_HOST_XRGB8888` whatever the framebuffer's format is (v6.12 `virtgpu_gem.c:78`), so an `AR24` cursor arrives as a `B8G8R8X8` resource even though its pixels do carry alpha. The backend was told `X`, and the runtime then forced every pixel opaque. Take the same way out QEMU does — it ignores the resource format for cursors entirely and copies the raw pixels as ARGB (`hw/display/virtio-gpu.c:44`, `virtio_gpu_update_cursor_data` memcpys `width * height * 4` bytes and never reads `res->format`) — but keep the channel order the resource declared instead of assuming one: map each `X` format to its `A` twin for the cursor path only. Scanouts are untouched; they really are `X`. Measured on Hyprland's arrow afterwards: 3824 pixels fully transparent, 182 antialiased edge pixels, 90 opaque. Also log the cursor's format, size and hotspot per image at debug level, so the next mismatch of this kind is visible without a bisect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfZtim8axDRi4BAVLXMLgG
`krun_set_display_backend` rejected any `backend_size` below the current `struct krun_display_backend` and then read the whole struct. Appending a method to the display vtable therefore breaks every existing caller: they pass their own, now smaller, `sizeof()` and get -EINVAL before `verify()` ever sees the struct. The cursor methods are the first append, so this is the point where it starts to matter. Methods are only ever appended, so a shorter struct has all of its fields where this version expects them. Accept any `backend_size` down to the size the struct had when it was first published — exported from krun_display as `MIN_BACKEND_SIZE`, computed from a frozen `#[repr(C)]` snapshot of the original vtable that must never gain a field — and copy `min(backend_size, size_of::<DisplayBackend>())` bytes into a zeroed struct. The methods the caller did not carry stay NULL with their feature bits unset, which `verify()` already reads as "not supported"; an old-size struct thus loads and simply lacks the cursor feature, which is what the `an_old_size_struct_is_accepted_and_simply_lacks_the_feature` test pins on the bindings side. Clamping the copy also makes the reverse case safe: a caller built against a *newer* header passes a longer struct, and the trailing methods this libkrun does not know about are ignored while `verify()` warns about the feature bits that came with them. All-zeroes is a valid `DisplayBackend` — no features, a null userdata pointer, `None` function pointers — so the zeroed `MaybeUninit` needs no further initialisation, and the byte-wise copy keeps the alignment freedom the previous `read_unaligned` had. Also updates the three places that documented the old behaviour: the vtable note in `libkrun_display.h`, the `verify()` doc comment, and `backend_size` in `include/libkrun.h`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bi4W2CcPvCDU2ReHeWKMt9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(gpu): serve the virtio-gpu cursor queue through a display-backend cursor plane
What
A guest that draws its pointer into the scanout pays a full-plane flush per
pointer move: Linux's virtio-gpu driver sets
ignore_damage_clipswhenever theplane's framebuffer changes (
virtgpu_plane.c:91-97), so every one-pixel movere-uploads and re-flushes the whole framebuffer. virtio-gpu has a cursor queue
exactly to avoid that, but the device dropped the queue at activation and
panicked on
UpdateCursor/MoveCursor, and the display backend ABI had nowhereto put a cursor anyway.
This adds the cursor plane end to end:
krun_display—KRUN_DISPLAY_FEATURE_CURSORand two vtable methods,set_cursorandmove_cursor, plus the Rust side: aCursorImagestruct, anoptional
DisplayBackendCursortrait next toDisplayBackendBasicFramebuffer,and
into_display_backend_with_cursorfor backends that implement both.devices— the gpu worker keeps the cursor queue and serves it.devices— cursor images keep their alpha channel.libkrun—krun_set_display_backendaccepts a backend struct builtagainst an older
libkrun_display.h.ABI compatibility
The two methods are appended to
krun_display_basic_framebuffer_vtable, soevery existing field keeps its offset. A caller built against the older header
passes a shorter struct with its own smaller
sizeof(); the fields it does notcarry read as NULL and
KRUN_DISPLAY_FEATURE_CURSORis not in its feature word.verify()already treats that as "the backend does not support this feature",and
DisplayBackendInstancegates both methods on the feature bit, returningKRUN_DISPLAY_ERR_METHOD_UNSUPPORTEDrather than calling through a NULL pointer.That only works if the loader lets the short struct in, and it did not:
krun_set_display_backendrejected anybackend_sizebelow the currentstruct size and then
read_unaligned'd the whole thing — so appending anythingto the vtable would have broken every existing caller at the door. The fourth
commit relaxes it: accept any
backend_sizedown to the size the struct hadwhen it was first published (
krun_display::MIN_BACKEND_SIZE, computed from afrozen
#[repr(C)]snapshot of the original vtable that must never gain afield) and copy
min(backend_size, size_of::<DisplayBackend>())bytes into azeroed struct before verifying it. All-zeroes is a valid
DisplayBackend— nofeatures, null userdata,
Nonefunction pointers — so the omitted methods landas NULL, which is precisely the "unsupported" case.
Clamping the copy buys the reverse direction for free: a caller built against a
newer header passes a longer struct, the trailing methods this libkrun does
not know are ignored, and
verify()already warns about the feature bits thatcame with them.
Four unit tests in
msb_krun_displaypin this: a round trip ofset_cursor/move_cursorthrough the C vtable, a backend without cursor support stillverifying and reporting
MethodNotSupported, an old-size struct rebuilt the waythe loader rebuilds it being accepted and simply lacking the feature, and the
cursor feature bit without its methods being rejected.
Device behaviour
The gpu worker used to block on the control queue's eventfd. It now
polls thecontrol and cursor eventfds together and drains whichever is ready. The cursor
queue must not be left unserviced — the driver waits for free descriptors and
the guest wedges after a queue's worth of cursor updates — but it needs none of
the control queue's machinery: it carries no fences and never leaves the worker
thread, so it stays a plain
VirtQueue. Linux'svirtio_gpu_queue_cursorsendsa single out-descriptor with no response buffer, so a response is written back
only when the driver actually provided one, and nothing on that path blocks.
The two cursor arms of the control queue now route to the same handlers instead
of panicking: a stray cursor command on the control queue must not take the
device down.
update_cursorreads the cursor resource's pixels back through rutabaga the waya scanout flush does — the guest uploads them with
TRANSFER_TO_HOST_2Dfirst —and hands them to the backend's cursor plane, followed by the position;
resource_id == 0hides the cursor. The image size comes from the resource(Linux's virtio-gpu cursor plane is 64x64
DRM_FORMAT_HOST_ARGB8888) and iscapped at 512x512 so a bogus resource cannot make the worker allocate something
huge. Every failure logs and answers
ErrUnspec; a backend that nevernegotiated the cursor feature does not get an error line per pointer move, since
the guest never reads these responses.
Alpha. The guest allocates its cursor as a dumb buffer, and Linux creates
the host resource with a hardcoded
DRM_FORMAT_HOST_XRGB8888whatever theframebuffer's format is (v6.12
virtgpu_gem.c:78), so anAR24cursor arrivesas a
B8G8R8X8resource even though its pixels do carry alpha. Reported asX,a backend has every reason to force the pixels opaque — and a compositor cursor
then draws as a black box. QEMU sidesteps this by ignoring the resource format
for cursors entirely:
virtio_gpu_update_cursor_data(
hw/display/virtio-gpu.c) memcpyswidth * height * 4bytes and never readsres->format. This takes the same way out but keeps the channel order theresource declared instead of assuming one — each
Xformat is mapped to itsAtwin on the cursor path only. Scanouts are untouched; they really are
X.The cursor's format, size and hotspot are logged per image at debug level.
Verification
modetest-driven, on macOS/HVF with an Arch Linux ARM guest and libkrunfw 6.12,through the
krun_displaybackend used by microsandbox'smsb display: theguest drove the cursor plane at 113 cursor images/s and 206 moves/s with zero
scanout flushes over the same interval — the whole point of the change, since
each of those moves previously cost a 1920x1080 flush.
Hyprland 0.56 on llvmpipe then exercised the real path with a patched aquamarine
that uses the cursor plane: its 64x64 arrow arrives with hotspot (3, 1) and its
alpha intact — 3824 fully transparent pixels, 182 antialiased edge pixels, 90
opaque, against 4096-of-4096 opaque before the alpha commit. The edge pixels all
had their colour channels at or below their alpha, i.e. the cursor plane carries
premultiplied alpha, which is what a compositor's GL renderer produces; a
consumer that wants straight alpha has to divide it back out.
Build:
all clean on aarch64-apple-darwin.
Not included
There is no host compositor here: libkrun hands the cursor image, its hotspot
and its position to the display backend and stops. A consumer opts in by
implementing
set_cursor/move_cursorand constructing its backend withinto_display_backend_with_cursor; one that does not is unaffected and keepsgetting the pointer composited into the scanout by the guest.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Bi4W2CcPvCDU2ReHeWKMt9