fix(server/render): robust vblank clock + display-mode handling on AMD RENOIR - #60
Merged
Merged
Conversation
Adds an overlay_scale rig-config field and --overlay-scale CLI flag (overrides rig-config) that scales the egui overlay via egui::Context::set_zoom_factor, useful on high-DPI displays where the default overlay text/controls are unreadably small. Also fixes a pre-existing bug in the custom Vulkan egui renderer: the vertex shader computes NDC as pos / screen_size, but mesh vertex positions from egui::Context::tessellate() are in logical points, not physical pixels. The renderer was pushing the physical swapchain size as the divisor, which only happened to be correct when pixels_per_point == 1.0. This made the overlay render too small on any display with a non-1.0 DPI scale (and defeated overlay_scale itself, since raising pixels_per_point shrinks the logical layout canvas without correspondingly enlarging the on-screen result). Co-Authored-By: Claude <noreply@anthropic.com>
rig-config's [display] width/height/refresh_hz were parsed and logged but never applied — DRM mode always auto-selected regardless. Worse, the auto-select fallback itself was broken: it picked the mode with the highest refresh rate via Iterator::max_by_key, which returns the *last* of equally-maximal elements. Since drivers commonly report same-refresh modes in descending-resolution order, this silently picked the smallest available mode (reproduced in the field: a 3840x2160-capable display auto-selected 640x480). pick_mode() now: - filters reported modes against rig-config's width/height/refresh_hz (any subset, refresh_hz matched within 1 Hz for EDID rates like 60.007), logging what it picked or a warning if nothing matches - ranks by (refresh_rate, resolution) instead of refresh_rate alone, fixing the tie-break bug for the auto-select fallback - keeps VSTIMD_DISPLAY_MODE=<index> as the highest-priority manual override Also tightens rig-config load() logging so it's unambiguous whether a config file was actually found and parsed vs. defaulted (previously main.rs logged "loaded from <path>" unconditionally, even when the file didn't exist). Added unit tests in drm_init.rs covering the exact regression (12 modes, tied 60.007 Hz, descending resolution), rig-config matching, refresh_hz tolerance, and the no-match fallback — DRM mode needs bare console access so this can't be exercised end-to-end in CI/sandboxes. Co-Authored-By: Claude <noreply@anthropic.com>
DRM_IOCTL_WAIT_VBLANK was found to intermittently fail with EINVAL on AMD RENOIR (amdgpu DC) — traced to a DMCUB firmware fault that also crashes GDM's greeter on this hardware, so it's an external kernel/ firmware issue vstimd can't fix. Previously a failure silently downgraded the clock (DRM -> VK -> present_wait -> raw CPU timestamp) with no signal that stimulus timing had become untrustworthy. - DrmVblank::open()/validate(): confirm the legacy ioctl actually works, after VT switch + Vulkan init (both can invalidate the CRTC state open() selected), before committing to it as the clock source. - DrmVblankState::wait()/register() now return Result: if the already-selected clock fails at runtime, that's treated as unrecoverable rather than silently falling back further. - shutdown::request_fatal()/fatal_reason(): render loops still exit through the normal return path so Drop guards restore the VT/CRTC, then main() exits with code 1 and a clear logged error. Co-Authored-By: Claude <noreply@anthropic.com>
…source
Auto-detection can't reliably predict every GPU/driver combination —
on AMD RENOIR (amdgpu DC), a clock source that passed vstimd's own
startup validation still failed once the render loop was running,
across every probing strategy tried (see fix/drm-vblank-clock-fallback
history). Rather than continuing to chase auto-detection heuristics,
let the clock source be pinned explicitly instead.
- rig_config::DisplayRigConfig::clock: Option<ClockSource>, parsed from
`[display] clock` ("auto", "drm_vblank", "vk_display_control",
"present_wait", "gpu_completion"). Reuses the existing ClockSource
enum (system_info.rs) rather than duplicating its variants; a custom
deserializer maps the literal "auto" string to None so the TOML file
can say `clock = "auto"` explicitly, not just omit the key.
- --preferred-clock-source CLI flag overrides rig-config, matching the
existing --overlay-scale pattern.
- DrmRenderLoopData::new(): when a specific source is forced, only that
source is attempted — no falling through to alternatives. If it's
unavailable, a startup_clock_error is set and surfaced via the
existing fatal_clock_loss path on the first run_loop iteration (so
Self's Drop guards still restore the VT/CRTC), naming the requested
source and the rig-config file to edit.
- "auto" (default) is unchanged: DRM validated late (right before the
loop starts), falling back to VK_EXT_display_control, then
present_wait, then GPU-completion.
Co-Authored-By: Claude <noreply@anthropic.com>
A rig-config with a commented-out `[display]` section header but uncommented keys underneath (e.g. `width = 1920` sitting at document root instead of under `[display]`) parsed successfully with those keys silently dropped — vstimd would run with full defaults for the whole section and give no indication anything was wrong. This is exactly the kind of silent misconfiguration this branch has been eliminating from the vblank clock path; rig-config deserves the same treatment. Add #[serde(deny_unknown_fields)] to RigConfig and all of its nested structs. A stray or misplaced key now fails to parse with a precise line/column and the field name, instead of being silently ignored. Co-Authored-By: Claude <noreply@anthropic.com>
Reported symptom: with rig-config forcing 1920x1080, the display looked correct at first, then stimuli visually shrank into the top-left corner while the rest of the screen reverted to the panel's native 4K. Debug logs confirmed the swapchain and scene tessellation stayed correctly sized at 1920x1080 the whole time — nothing in vstimd's own state changed. The CRTC itself silently reverted to 3840x2160 outside of vstimd's control (matching the AMD RENOIR DMCUB firmware fault already seen in this branch's history), and neither vkQueuePresentKHR nor vkAcquireNextImageKHR surfaced this via ERROR_OUT_OF_DATE_KHR — Vulkan kept scanning out the unchanged 1920x1080 swapchain into a now-larger physical raster with no error at any layer. - DrmDisplayGuard::check_mode(): read-only ioctl (get_crtc, no DRM master needed) comparing each CRTC's actual current mode against the size vstimd requested. - Wired into the render loop, checked roughly once a second (cheap read, but kept off the per-frame hot path) rather than every frame. - Renamed DrmRenderLoopData::fatal_clock_loss to fatal_display_error and reused it for this new failure class — same guarantee as the vblank-clock work earlier in this branch: fail loudly and exit through the normal path (Drop guards still restore VT/CRTC) instead of silently rendering into the wrong resolution. Co-Authored-By: Claude <noreply@anthropic.com>
…ents The new CRTC-mode-drift check (previous commit) ran on frame 0, before any frame had actually been presented. vkCreateDisplayPlaneSurfaceKHR's requested modeset doesn't necessarily commit at surface-creation time on this hardware — it was observed to only take effect once the first frame is presented — so checking that early caught a normal transient startup state as a false positive and shut vstimd down immediately on every launch. Skip frame 0; the periodic check still starts a moment later, well after the first present has had time to land. Verified over a 2-minute run at 1920x1080 with clock forced to VK_EXT_display_control: no false positives, clean shutdown. Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Summary
Investigating issue #59 (DRM vblank ioctl failing on an AMD Radeon RENOIR iGPU) led to a broader finding: the root cause is an external, intermittent AMD Renoir DMCUB firmware fault — confirmed independently via a kernel
dc_dmub_srv_log_diagnostic_data: DMCUB errorthat also crashes GDM's greeter on the same machine, unrelated to vstimd. That fault can't be fixed from userspace. What changed here instead is vstimd's behavior around it: several places where a display/timing assumption could silently break were made to fail loudly and safely instead, plus a way to route around the broken parts entirely.This branch also carries three earlier, already-reviewed commits from
fix/wayland-startup(Wayland startup fix,--overlay-scale, rig-config display-mode wiring) that this work was built on top of and hasn't been merged separately yet.What changed
Vblank clock (was: silent precision downgrade mid-session)
DrmVblank::open()/validate(): confirm the legacyDRM_IOCTL_WAIT_VBLANKioctl actually works, checked as late as possible (right before the render loop starts), before committing to it as the session's clock.DrmVblankState::wait()/register()now returnResult: if the already-selected clock fails at runtime, that's treated as unrecoverable instead of silently falling back to a coarser clock (DRM → VK → present_wait → raw CPU timestamp) with no signal to the experimenter.shutdown::request_fatal(): render loops still exit through the normal return path (Drop guards restore the VT/CRTC), then the process exits with code 1 and a clear logged error — instead of continuing, or exiting 0 like a normal quit.Clock source override (rig-config + CLI)
[display] clockin rig-config ("auto","drm_vblank","vk_display_control","present_wait","gpu_completion") and--preferred-clock-sourcepin a specific clock, bypassing auto-detection entirely. Reuses the existingClockSourceenum rather than duplicating it.rig-config validation
#[serde(deny_unknown_fields)]on every rig-config struct. A rig-config with a commented-out[display]header but uncommented keys underneath (an easy mistake — caught in the field) used to parse successfully with those keys silently dropped, running with full defaults for the whole section. Now it's a parse error naming the exact unknown field and line.Out-of-band CRTC modeset detection
vkQueuePresentKHRnorvkAcquireNextImageKHRsurfaced this viaERROR_OUT_OF_DATE_KHR.DrmDisplayGuard::check_mode(): a read-only ioctl comparing the CRTC's actual mode against what vstimd requested, polled roughly once a second from the render loop (skipping frame 0, since the modeset doesn't necessarily commit before the first present). A mismatch is fatal, through the same safe-exit path as the clock work above.Verified on hardware
All of the above was iterated against a real AMD Radeon RENOIR (RADV) machine over multiple sessions:
clock = "vk_display_control"confirmed to run stably for 2+ minutes at a forced 1920×1080, with the CRTC-drift check active and no false positives.deny_unknown_fieldsconfirmed to catch the exact misconfiguration hit in the field (commented-out[display]header).Test plan
cargo test(all crates)cargo clippy --all-targets(no new warnings)client/python: make test-e2e-null🤖 Generated with Claude Code