-
Notifications
You must be signed in to change notification settings - Fork 109
feat(runtime): deferred INTC pending-raise latch with per-tick drain #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1ade90c
0b2f961
32ed26d
c993b02
2db7c99
5420222
f766a22
ec26ccd
8d324ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ | |
| #include "ps2_log.h" | ||
| #include "Stubs/GS.h" | ||
|
|
||
| #include <bit> | ||
|
|
||
| namespace ps2_syscalls | ||
| { | ||
| namespace interrupt_state | ||
|
|
@@ -24,10 +26,35 @@ namespace ps2_syscalls | |
| uint32_t g_enabled_dmac_mask = 0xFFFFFFFFu; | ||
| uint64_t g_vsync_tick_counter = 0u; | ||
| VSyncFlagRegistration g_vsync_registration{}; | ||
|
|
||
| std::atomic<uint32_t> g_pending_intc_causes{0u}; // bitmask, one pending bit per cause | ||
| std::atomic<int64_t> g_pending_intc_raise_ns[32] = {}; // steady_clock ns at last raise, per cause | ||
| // The raise-timestamp entries are atomic because raisePendingIntc (any | ||
| // thread) stores a fresh timestamp while the interrupt worker thread | ||
| // reads it in drainPendingIntc. | ||
| } | ||
|
|
||
| using namespace interrupt_state; | ||
|
|
||
| namespace | ||
| { | ||
| SteadyNowFn g_pendingIntcNowFn{}; // empty = real clock | ||
|
|
||
| int64_t now_ns() | ||
| { | ||
| const auto tp = g_pendingIntcNowFn ? g_pendingIntcNowFn() | ||
| : std::chrono::steady_clock::now(); | ||
| return std::chrono::duration_cast<std::chrono::nanoseconds>( | ||
| tp.time_since_epoch()) | ||
| .count(); | ||
| } | ||
| } | ||
|
|
||
| void setPendingIntcClockForTest(SteadyNowFn fn) | ||
| { | ||
| g_pendingIntcNowFn = std::move(fn); | ||
| } | ||
|
|
||
| static void writeGuestU32NoThrow(uint8_t *rdram, uint32_t addr, uint32_t value) | ||
| { | ||
| if (addr == 0u) | ||
|
|
@@ -96,19 +123,19 @@ namespace ps2_syscalls | |
| return (s_cachedStackTop != 0u) ? s_cachedStackTop : (PS2_RAM_SIZE - 0x10u); | ||
| } | ||
|
|
||
| static void dispatchIntcHandlersForCause(uint8_t *rdram, PS2Runtime *runtime, uint32_t cause) | ||
| static int dispatchAndCountIntcHandlersForCause(uint8_t *rdram, PS2Runtime *runtime, uint32_t cause) | ||
| { | ||
| if (!rdram || !runtime) | ||
| { | ||
| return; | ||
| return 0; | ||
| } | ||
|
|
||
| std::vector<IrqHandlerInfo> handlers; | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_irq_handler_mutex); | ||
| if (cause < 32u && (g_enabled_intc_mask & (1u << cause)) == 0u) | ||
| { | ||
| return; | ||
| return 0; | ||
| } | ||
|
|
||
| handlers.reserve(g_intcHandlers.size()); | ||
|
|
@@ -133,6 +160,7 @@ namespace ps2_syscalls | |
| { return a.order < b.order; }); | ||
| } | ||
|
|
||
| int dispatchedCount = 0; | ||
| for (const IrqHandlerInfo &info : handlers) | ||
| { | ||
| if (!runtime->hasFunction(info.handler)) | ||
|
|
@@ -157,6 +185,7 @@ namespace ps2_syscalls | |
| continue; | ||
| } | ||
|
|
||
| ++dispatchedCount; | ||
| try | ||
| { | ||
| R5900Context irqCtx{}; | ||
|
|
@@ -216,6 +245,96 @@ namespace ps2_syscalls | |
| } | ||
| } | ||
| } | ||
|
|
||
| return dispatchedCount; | ||
| } | ||
|
|
||
| void raisePendingIntc(uint32_t cause) | ||
| { | ||
| if (cause >= 32u || cause == kIntcVblankStart || cause == kIntcVblankEnd) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const uint32_t bit = 1u << cause; | ||
| g_pending_intc_raise_ns[cause].store(now_ns(), std::memory_order_relaxed); // every raise | ||
| const uint32_t prev = g_pending_intc_causes.fetch_or(bit, std::memory_order_acq_rel); | ||
| if ((prev & bit) == 0u) | ||
| { | ||
| PS2_IF_AGRESSIVE_LOGS({ | ||
| static std::atomic<uint32_t> s_raiseLogCount{0u}; | ||
| const uint32_t logIndex = s_raiseLogCount.fetch_add(1u, std::memory_order_relaxed); | ||
| if (logIndex < 16u || (logIndex % 256u) == 0u) | ||
| { | ||
| RUNTIME_LOG("[INTC:raise] cause=" << cause); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| void drainPendingIntc(uint8_t *rdram, PS2Runtime *runtime) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. The current over-approximation is unsafe whenever a persistent cause-5 handler exists. |
||
| { | ||
| uint32_t pending = g_pending_intc_causes.load(std::memory_order_acquire); | ||
| while (pending != 0u) | ||
| { | ||
| const uint32_t cause = static_cast<uint32_t>(std::countr_zero(pending)); | ||
| const uint32_t bit = 1u << cause; | ||
| pending &= ~bit; | ||
|
|
||
| const int ran = dispatchAndCountIntcHandlersForCause(rdram, runtime, cause); | ||
| if (ran > 0) | ||
| { | ||
| // Level-triggered: concurrent same-cause raises collapse into one delivery (intentional). | ||
| g_pending_intc_causes.fetch_and(~bit, std::memory_order_acq_rel); | ||
| PS2_IF_AGRESSIVE_LOGS({ | ||
| static std::atomic<uint32_t> s_deliverLogCount{0u}; | ||
| const uint32_t logIndex = s_deliverLogCount.fetch_add(1u, std::memory_order_relaxed); | ||
| if (logIndex < 16u || (logIndex % 256u) == 0u) | ||
| { | ||
| RUNTIME_LOG("[INTC:deliver] cause=" << cause << " handlers=" << ran); | ||
| } | ||
| }); | ||
| } | ||
| else | ||
| { | ||
| const int64_t age = now_ns() - g_pending_intc_raise_ns[cause].load(std::memory_order_relaxed); | ||
| if (age > kPendingIntcMaxAgeNs) | ||
| { | ||
| g_pending_intc_causes.fetch_and(~bit, std::memory_order_acq_rel); | ||
| PS2_IF_AGRESSIVE_LOGS({ | ||
| static std::atomic<uint32_t> s_dropLogCount{0u}; | ||
| const uint32_t logIndex = s_dropLogCount.fetch_add(1u, std::memory_order_relaxed); | ||
| if (logIndex < 16u || (logIndex % 256u) == 0u) | ||
| { | ||
| RUNTIME_LOG("[INTC:drop] cause=" << cause << " aged out with no registered/enabled handler"); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void resetInterruptHandlerState() | ||
| { | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_irq_handler_mutex); | ||
| g_intcHandlers.clear(); | ||
| g_dmacHandlers.clear(); | ||
| g_nextIntcHandlerId = 1; | ||
| g_nextDmacHandlerId = 1; | ||
| g_intc_head_order = 0; | ||
| g_intc_tail_order = 1000; | ||
| g_dmac_head_order = 0; | ||
| g_dmac_tail_order = 1000; | ||
| g_enabled_intc_mask = 0xFFFFFFFFu; | ||
| g_enabled_dmac_mask = 0xFFFFFFFFu; | ||
| } | ||
| g_pending_intc_causes.store(0u, std::memory_order_release); | ||
| for (auto &ts : g_pending_intc_raise_ns) | ||
| { | ||
| ts.store(0, std::memory_order_relaxed); | ||
| } | ||
| setPendingIntcClockForTest(nullptr); // a leaked fake clock would leak into the next test | ||
| } | ||
|
|
||
| void dispatchDmacHandlersForCause(uint8_t *rdram, PS2Runtime *runtime, uint32_t cause) | ||
|
|
@@ -408,15 +527,16 @@ namespace ps2_syscalls | |
| PS2Runtime::DeferredGuestYieldScope deferYield(reschedulePending); | ||
| const uint64_t tickValue = signalVSyncFlag(rdram, runtime); | ||
| ps2_stubs::dispatchGsSyncVCallback(rdram, runtime, tickValue); | ||
| dispatchIntcHandlersForCause(rdram, runtime, kIntcVblankStart); | ||
| dispatchAndCountIntcHandlersForCause(rdram, runtime, kIntcVblankStart); | ||
| handoffBaseline = runtime->guestExecutionHandoffEpochSnapshot(); | ||
| } | ||
| if (reschedulePending && !runtime->isStopRequested()) | ||
| { | ||
| runtime->waitForGuestExecutionHandoff(handoffBaseline); | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::microseconds(500)); | ||
| dispatchIntcHandlersForCause(rdram, runtime, kIntcVblankEnd); | ||
| dispatchAndCountIntcHandlersForCause(rdram, runtime, kIntcVblankEnd); | ||
| drainPendingIntc(rdram, runtime); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| #pragma once | ||
|
|
||
| #include <atomic> | ||
| #include <chrono> | ||
| #include <condition_variable> | ||
| #include <cstdint> | ||
| #include <functional> | ||
| #include "ps2_syscalls.h" | ||
|
|
||
| namespace ps2_syscalls | ||
|
|
@@ -24,9 +28,18 @@ namespace ps2_syscalls | |
| extern uint32_t g_enabled_dmac_mask; | ||
| extern uint64_t g_vsync_tick_counter; | ||
| extern VSyncFlagRegistration g_vsync_registration; | ||
| extern std::atomic<uint32_t> g_pending_intc_causes; | ||
| constexpr int64_t kPendingIntcMaxAgeNs = 2'000'000'000; // ~2 s wall-clock | ||
| } | ||
|
|
||
| void dispatchDmacHandlersForCause(uint8_t *rdram, PS2Runtime *runtime, uint32_t cause); | ||
| void raisePendingIntc(uint32_t cause); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| void drainPendingIntc(uint8_t *rdram, PS2Runtime *runtime); | ||
| // Test-support: reset all INTC/DMAC handler bookkeeping to process-start | ||
| // defaults so regression tests are order-independent. | ||
| void resetInterruptHandlerState(); | ||
| using SteadyNowFn = std::function<std::chrono::steady_clock::time_point()>; | ||
| void setPendingIntcClockForTest(SteadyNowFn fn); // nullptr restores steady_clock::now | ||
| void EnsureVSyncWorkerRunning(uint8_t *rdram, PS2Runtime *runtime); | ||
| uint64_t GetCurrentVSyncTick(); | ||
| void stopInterruptWorker(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.