feat(runtime): deferred INTC pending-raise latch with per-tick drain - #151
feat(runtime): deferred INTC pending-raise latch with per-tick drain#151smmathews wants to merge 9 commits into
Conversation
6adba13 to
f98e577
Compare
submitDmaSend completes DMA transfers synchronously but never raised the completion interrupt, so the standard sce-libdma kick-and-wait protocol (sceDmaSend -> CreateSema -> AddIntcHandler -> EnableIntc -> WaitSema) could never be released: a synchronous dispatch at the raise site would fire before the handler is registered and be lost, hanging the guest. Add a level-triggered pending-cause latch: - raisePendingIntc(cause): atomically sets a per-cause bit in an atomic pending bitmask (safe from any thread; vblank causes 2/3 and out-of-range causes are excluded -- vblank stays the worker's own periodic dispatch). - drainPendingIntc(rdram, runtime): called once per tick by the interrupt worker; dispatches each pending cause, clearing the bit only once at least one registered+enabled handler actually ran, or after it ages out. A per-cause age array tracks undelivered ticks: a pending cause survives 120 drains and drops on the 121st (~2 s @ 60 Hz), with the age reset on re-raise. This covers the raise-vs-registration race in both directions. - dispatchIntcHandlersForCause is renamed dispatchAndCountIntcHandlersForCause and changes return type void -> int, returning the number of handlers dispatched (0 when the cause is disabled or unregistered) so the drain can tell delivery from a miss. submitDmaSend raises INTC cause 5 after a VIF1 (0x10009000) kick; delivery is intentionally deferred to the next drain tick, which is hardware-plausible DMA-completion latency. The raise is an unconditional over-approximation rather than a per-kick edge: the VIFcode i-bit and DMAtag irq bit are decoded, but the resulting INT stat bit is sticky (cleared only by FBRST/CPU write) and has no consumer, so it is inert and cannot gate a clean edge. Over-raising is benign when unused (the cause ages out), whereas gating on the inert bit would drop legitimate interrupts. resetInterruptHandlerState() (called from cleanupRuntime) resets the handler tables, masks, and latch so runtime state does not leak between tests. Regression tests cover delivery on the next drain tick, the age-out boundary (survives exactly 120 undelivered drains, drops on the 121st, age resets on re-raise), pending persistence across the registration race, and vblank/out-of-range exclusion. raisePendingIntc, drainPendingIntc, and resetInterruptHandlerState are exposed in the runtime header for use by the interrupt worker and tests.
f98e577 to
1ade90c
Compare
…er libstdc++ elfio's elf_types.hpp relies on uint16_t/uint32_t/uint64_t being transitively available without including <cstdint> itself. This build environment's libstdc++ no longer pulls those in transitively, causing elf_parser.cpp to fail to compile with 'does not name a type' errors.
The "Semaphore poll/signal remains stable under host-thread contention" test asserted signalOkCount > 0, but SignalSema only returns success when count < max_count. With init_count == max_count == 1 the counter starts full, so the signaler thread only succeeds after the poller has drained the count. Whether that interleaving occurs depends entirely on host thread scheduling, so under load (as on the linux-clang CI runner) the signaler can run all 64 iterations while the counter is full, hitting KE_SEMA_OVF every time and leaving signalOkCount == 0 -> spurious failure. Replace the non-deterministic signalOk > 0 assertion with the conservation invariant finalCount == init_count - successful_polls + successful_signals, which holds for every interleaving and is a stronger stability check: it detects any lost or double-applied acquire/release under contention. The guaranteed pollOk > 0 assertion (the first poll always observes count == 1) and the count-range check are retained.
…ng-latch-drain # Conflicts: # ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp # ps2xTest/src/ps2_runtime_expansion_tests.cpp
| } | ||
| } | ||
|
|
||
| void drainPendingIntc(uint8_t *rdram, PS2Runtime *runtime) |
There was a problem hiding this comment.
This unconditional raise can change guest synchronization semantics, not just interrupt timing. A game may keep a cause-5 handler registered while submitting many ordinary VIF1 packets and request an interrupt only on selected VIFcodes using the I bit. With the current implementation, every packet invokes that handler and may signal semaphores or advance display-list state prematurely.
The VIF1 interpreter already has the exact per-command edge available:
const bool irq = (cmd & 0x80000000u) != 0u;
I agree that reading the sticky STAT.INT bit later from submitDmaSend() would be unreliable, but we do not need to use the sticky register as the event source. Could processVIF1Data() record an interrupt edge when it decodes an I bit, and could PS2Memory expose those events through a consume API similar to consumeCompletedDmacCauses()?
Please add integration coverage for both cases:
A VIF1 transfer containing a VIFcode with I=1 raises cause 5.
A normal VIF1 transfer with no I bit does not raise cause 5.
The current over-approximation is unsafe whenever a persistent cause-5 handler exists.
| } | ||
|
|
||
| void dispatchDmacHandlersForCause(uint8_t *rdram, PS2Runtime *runtime, uint32_t cause); | ||
| void raisePendingIntc(uint32_t cause); |
There was a problem hiding this comment.
raisePendingIntc() only sets the pending bit. The age counter advances exclusively from drainPendingIntc(), and the production drain is called by the interrupt worker.
This means the advertised approximately two-second expiration does not apply before anything has started the worker. A VIF1 kick can set cause 5, no handler or VSync API may be used for an arbitrary amount of time, and a handler registered much later can receive that stale interrupt because its age remained zero the entire time.
This is particularly relevant because the targeted sequence raises before AddIntcHandler(), and AddIntcHandler() is what starts the worker.
Could expiration use a monotonic timestamp associated with the raise, or could the implementation otherwise guarantee that pending causes are drained even when no handler/VSync worker existed at raise time? Please add a regression that covers a raise occurring before worker startup and verifies that it cannot survive beyond the configured expiration window.
| } | ||
|
|
||
| const uint32_t bit = 1u << cause; | ||
| const uint32_t prev = g_pending_intc_causes.fetch_or(bit, std::memory_order_acq_rel); |
There was a problem hiding this comment.
The age is reset only when fetch_or() observes a 0 -> 1 transition. If a cause has been pending for almost the full timeout and the same cause is raised again, the second raise does not refresh the age because the bit is already set. The next drain can therefore drop an interrupt that was asserted only one tick ago.
Collapsing multiple raises into one pending level is reasonable, but the artificial stale-event timeout should not classify a recently reasserted level using the age of an older assertion.
Please add a regression that:
Raises a cause.
Advances it to the age-out boundary.
Raises the same cause again while the bit is still set.
Verifies that the next drain does not discard the newly reasserted event.
Once cause 5 is raised only for real VIF I events, resetting the age on each raise should no longer conflict with expiration.
| cleanupRuntime(env); | ||
| }); | ||
|
|
||
| tc.Run("raisePendingIntc delivers to a registered handler on the next drain tick", [](TestCase &t) |
There was a problem hiding this comment.
All new tests drive raisePendingIntc() directly, so they validate the latch but not the behavior introduced in submitDmaSend().
Please add an integration test that submits an actual VIF1 packet, lets processPendingTransfers() parse it, registers/enables the handler after the kick, and then verifies deferred delivery. A corresponding no-I packet test should verify that no cause-5 interrupt is produced.
Without those tests, the suite cannot detect the unconditional producer bug or prove that the DQ8-style DMA-to-late-handler path is what actually unblocks.
…a PS2Memory consume API Cause 5 is a real hardware event defined by a VIFcode I bit, not by DMA completion. processVIF1Data already decodes that bit; record it as an edge on PS2Memory and consume it from submitDmaSend before raising the deferred cause-5, mirroring the existing consumeCompletedDmacCauses() pattern one level of abstraction up. An ordinary VIF1 packet no longer trips a persistent cause-5 handler. Dropping the channelBase == VIF1 guard on the raise (the edge counter is only ever incremented by processVIF1Data) opens a narrow gap: an edge parsed via processVIF1Data during a non-VIF1 kick's processPendingTransfers, or via the GS/Font/GIF-chain paths, would otherwise wait for the next unrelated submitDmaSend to be consumed. Add a second consume site in drainCompletedDmacHandlers, the runtime's existing "consume queued DMAC events and dispatch" chokepoint, so those edges are delivered within the frame instead of being misattributed to a later kick.
…reshed on every raise Replace the per-cause drain-tick counter with a per-cause steady_clock timestamp captured at raise time. Two problems with the tick counter: its window scales with how often something happens to call drainPendingIntc, not with real time, so a cause raised before the interrupt worker starts could survive far longer (or shorter) than intended; and the timestamp was previously only stamped on the 0->1 edge, so a cause re-raised while already pending kept aging from its first raise instead of its most recent one, letting a live, repeatedly re-raised cause get dropped out from under an about-to-register handler. The timestamp is now stored on every raise, independent of whether the pending bit was already set, and drainPendingIntc compares wall-clock age against a fixed ~2s window instead of counting ticks. Add an injectable clock (setPendingIntcClockForTest) so the expiry window can be exercised deterministically in tests without sleeping; resetInterruptHandlerState restores the real clock so a test that installs a fake one can't leak it into the next test.
Two new tests drive the real producer (ps2_stubs::sceDmaSend, not a direct raisePendingIntc call) with a CNT-then-END VIF1 DMA chain: one with an I-bit VIFcode in the payload, one without. They cover the exact DQ8 boot hazard -- a kick that runs before sce libdma registers its cause-5 handler -- and the negative case the old unconditional raise got wrong: an ordinary VIF1 packet must not trip a persistent cause-5 handler. Rewrite the drain-tick age-out test to drive the injectable clock instead of looping a tick counter (the counter it referenced is gone), and extend it to re-raise mid-window and confirm the bit survives because freshness is measured from the most recent raise, not the first. Add a test for a raise that predates worker startup, confirming it cannot outlive the expiry window once a drain finally runs.
…ternal doc ref; trim level-triggered comment
|
Thanks for the thorough review, all four points were right, and I've reworked the PR accordingly (pushed as follow-up commits so your line comments stay anchored; the PR body is updated to match):
|
feat(runtime): deferred INTC pending-raise latch with per-tick drain
Summary
Adds a deferred INTC pending-raise latch with per-tick drain and late-handler delivery.
submitDmaSendcompletes DMA transfers synchronously (processPendingTransfers()) but never raised the completion interrupt, so the standard sce-libdma kick-and-wait protocol (sceDmaSend→CreateSema→AddIntcHandler→EnableIntc→WaitSema) could never be released — the handler is registered after the kick returns, so a synchronous dispatch at the raise site would be lost and the guest would block onWaitSemaforever (observed on a downstream title's per-frame VIF1 display-list submit path).What changed
Kernel/Syscalls/Interrupt.cpp/Interrupt.hg_pending_intc_causes(atomic bitmask) + a per-cause raise timestamp.raisePendingIntc(cause): atomically sets the pending bit; safe from any thread; excludes vblank causes 2/3 (they remain the worker's own periodic dispatches) and out-of-range causes; stamps the cause's raise timestamp on every raise.drainPendingIntc(rdram, runtime): called once per tick by the interrupt worker (alongside the vblank dispatches). A pending bit is cleared only once ≥1 registered+enabled handler actually ran ([INTC:deliver]), or once its wall-clock age exceeds the expiry window ([INTC:drop]). This covers the raise-vs-registration race in both directions.dispatchIntcHandlersForCause→dispatchAndCountIntcHandlersForCause,void→int(count of handlers dispatched) so the drain can distinguish delivery from a miss.Kernel/Stubs/Helpers/Support.hsubmitDmaSendraises cause 5 (VIF1) viaps2_syscalls::raisePendingIntc(5u). Delivery is intentionally deferred to the next drain tick (≤ one vblank of latency — hardware-plausible DMA-completion timing).Level-triggered raise-mid-drain collapse (by design)
drainPendingIntcclears the single pending bit on delivery. If another raise of the same cause lands mid-drain (between the dispatch and thefetch_andclear), the two raises collapse into one delivery. This is accepted under the level-triggered design — a set bit means "at least one pending", not a count. A code comment at thefetch_andsite marks the tradeoff as deliberate (not a lost-wakeup bug).Design notes / header-exposed symbols
All behavior-identical; testability-driven:
drainPendingIntcis non-static and declared inInterrupt.hso the regression tests can drive drain ticks deterministically;std::countr_zero(C++20) is used instead of__builtin_ctzfor MSVC portability.interrupt_state::g_pending_intc_causes(extern) is exposed inInterrupt.hso tests can assert the latch state directly.resetInterruptHandlerState()is declared inInterrupt.hand defined inInterrupt.cpp(test-support). It restores the INTC/DMAC handler tables, enable masks, id/order counters, and the pending latch to their process-start defaults. It must live inInterrupt.cppbecause the handler tables arestaticper translation unit and the raise-timestamp array is not header-exposed — only that TU can reset the live instances. Tests call it incleanupRuntimeas belt-and-suspenders defensive isolation — the suite's actual isolation ismain.cpp'sBeforeEach(reset_ps2_test_function_table), which wipes the recompiled-function table between tests.Rework
Addresses maintainer review: cause 5 is now gated on the real hardware event instead of over-approximated, and the age-out window is wall-clock instead of drain-tick-counted.
processVIF1Datadecodes a VIFcode with the I bit (cmd & 0x80000000). The decode already happened at that call site; it now also records an edge onPS2Memory(m_vif1InterruptEdges), consumed through a newconsumeVif1InterruptEdges()API that mirrors the existingconsumeCompletedDmacCauses(). An ordinary VIF1 packet with no I bit no longer invokes a persistent cause-5 handler — the previous unconditional per-kick raise was a genuine over-approximation the maintainer flagged correctly.submitDmaSendright after the synchronous transfer completes, so the downstream raise-before-registration path (sceDmaSendkicks VIF1, cause 5 is pending by the timeAddIntcHandler/EnableIntcrun) still works end to end.submitDmaSend(the GS/Font/GIF-chain paths, which also call intoprocessVIF1Data) are consumed indrainCompletedDmacHandlers— the runtime's existing "consume queued DMAC events and dispatch" chokepoint, called at every drain point — so a real I-bit event from those paths is delivered within the frame instead of leaking into a later, unrelatedsubmitDmaSendkick.drainPendingIntcrather than with real time (a cause raised before the interrupt worker starts could survive for an unbounded number of wall-clock seconds), and the age was only reset on the initial raise, so a cause re-raised while already pending kept aging from its first raise — a live, repeatedly re-raised cause could be dropped out from under a handler that was about to register.setPendingIntcClockForTest) lets the expiry window be exercised deterministically in tests without sleeping;resetInterruptHandlerStaterestores the real clock so a test that installs a fake one can't leak it into the next test.drainPendingIntccall to observe and act on a pending cause. A cause raised while nothing has ever driven a drain (no VSync worker running, no manual drain) stays pending indefinitely until one runs — there is no standalone timer thread driving expiry independent of the drain path.processVIF1Data: the edge counter tracked the runtime's existing per-frame cause-5 delivery one-for-one, in the same relative order, for the full length of the observed boot. The instrumentation was temporary, deps-tree-only, and reverted before this branch's changes were made.Tests
ps2xTest(ps2_runtime_interrupt_tests.cpp), driving the latch and the real producer with the interrupt worker thread stopped so drain ticks are deterministic:$a0 == 5, pending bit clears.sceDmaSendwhose payload carries an I-bit VIFcode, before any cause-5 handler is registered; assert the pending bit is set from the kick alone; then register+enable a handler and drain: delivered exactly once. This is the downstream raise-before-registration path exercised through the real producer, not a directraisePendingIntccall.raisePendingIntcon vblank causes 2/3 and cause 32 never sets a bit.Verification
ps2x_tests(Release, GCC/Ninja on Linux,-msse4.1matching this repo's CI configuration) from a clean tree.EVIDENCE.mdfor the reproduction command and one mutation per pinned property (each verified to make exactly the named test fail, then restored and re-verified green).