fix(dsp): take DSP interrupts at the DSP's next instruction, not in the event callback (#744) (#849)

Club Drive was silent under HLE. DSPSetIRQLine vectored the DSP
synchronously inside the I2S event callback; the 68K, which runs its
whole slice before DSPExec, then wrote D_PC=$F1B030 while DSPGO=1 (JTRM v8
p.110: D_PC writes only while the DSP is idle) and landed on the vector
with zero ISR instructions run; the game's init wrote IMASK=1 back and no
interrupt was ever taken again (DSP spins at $F1B120, LTXD 0). Pure I2S
phase luck: 5/97 first-tick phases silent on develop, 0/97 with the fix.

DSPSetIRQLine now latches the interrupt (IMASKCleared, already
serialized) and the DSP takes it at its next instruction boundary in DSP
time; a host D_FLAGS write ORs into the latch instead of dropping it.
The handoff-state hypotheses (R31, bank seeding) were tested and refuted.

Club Drive HLE: RMS 0.0 -> 2948.6, onset frame 6. Corpus A/B vs develop:
160/160 framebuffer streams identical in both modes; cart matrix audio
changed only for Club Drive HLE (better) and Cybermorph HLE (+5 samples,
neutral). Audio presence + clipping unchanged on Wolf3D, Doom, WMCJ,
IS1/IS2, Skyhammer, Atari Karts, Raiden, T2K, Zoop. test_dsp_unit 7/9/9a
updated to the new contract; Club Drive HLE presence check added.

Refs #744

Co-authored-by: Claude Opus 5.5 <[email protected]>

Makefile

@@ -1728,6 +1728,19 @@ test: test/test_dram_timing test/test_cheat test/test_event_queue test/test_jlin
 	else \
 		bash scripts/test-skip.sh record "Iron Soldier 1 (audio presence, risc=2x)" "no ROM matching 'Iron Soldier*' in the private corpus"; \
 	fi
+	@# Club Drive under the HLE boot (#744): the 68K rewrites D_PC while
+	@# the DSP runs; when that landed right after an I2S event the DSP
+	@# restarted with IMASK stuck and the game was silent forever.  The
+	@# fix defers DSP interrupt vectoring to the DSP's next instruction.
+	@# Measured on the fix: RMS ~2949, onset frame 6 (develop: RMS 0,
+	@# no onset).  Same envelope as Iron Soldier 1 above.
+	@rom=$$(bash scripts/find-rom.sh 'Club Drive (1994).jag' 'Club Drive*.jag' 'Club Drive*.j64'); \
+	if [ -n "$$rom" ]; then \
+		./test/test_audio_presence ./$(TARGET) "$$rom" --label "Club Drive (HLE)" --rms-floor 200 --rms-ceiling 25000 --quiet && \
+		./test/test_audio_clipping ./$(TARGET) "$$rom" --label "Club Drive (HLE)" --quiet; \
+	else \
+		bash scripts/test-skip.sh record "Club Drive (HLE audio presence)" "no ROM matching 'Club Drive*' in the private corpus"; \
+	fi
 	@# Save-state determinism: replay the same frames after
 	@# retro_unserialize and require identical video AND audio.  This is
 	@# what backs `savestate_features = 3` in dist/info/ and the zero

src/jerry/dsp.c

@@ -799,7 +799,13 @@ void DSPWriteLong(uint32_t offset, uint32_t data, uint32_t who/*=UNKNOWN*/)
          case 0x00:
             {
                uint32_t * preWriteBank = dsp_reg;
-               IMASKCleared = (dsp_flags & IMASK) && !(data & IMASK);
+               /* OR, not assign: a host write to D_FLAGS must not drop
+                * an interrupt DSPSetIRQLine latched for the next
+                * instruction boundary.  Re-checking is always safe --
+                * DSPHandleIRQsNP takes nothing that is masked, disabled
+                * or not latched. */
+               IMASKCleared = IMASKCleared
+                  || ((dsp_flags & IMASK) && !(data & IMASK));
                dsp_flags = (data & ~IMASK) | ((data & IMASK) ? (dsp_flags & IMASK) : 0);
                dsp_flag_z = dsp_flags & 0x01;
                dsp_flag_c = (dsp_flags >> 1) & 0x01;
@@ -1006,9 +1012,10 @@ void DSPHandleIRQsNP(void)
 
 	/* IMASK reads clear the moment the D_FLAGS store is decoded, but the
 	 * store has not retired yet -- the instruction behind it is already
-	 * past Read Operands and still runs masked (see DSPWriteLong).  This
-	 * entry point is also reached asynchronously from DSPSetIRQLine, which
-	 * cannot see DSPExec's own hold-off, so re-check here.  Leave the
+	 * past Read Operands and still runs masked (see DSPWriteLong).  DSPExec's
+	 * loop-top caller already waits out the hold-off; this used to be
+	 * reached asynchronously from DSPSetIRQLine too (no longer, #744), and
+	 * stays as a guard for any caller outside that loop.  Leave the
 	 * latch standing in dsp_control and IMASKCleared unconsumed: DSPExec
 	 * dispatches it once the delay expires, at most two slots later.
 	 *
@@ -1070,7 +1077,42 @@ void DSPSetIRQLine(int irqline, int state)
 	if (state)
 	{
 		dsp_control |= mask;
-		DSPHandleIRQsNP();
+		/* Latch now, vector later: the DSP takes the interrupt itself at
+		 * its next instruction boundary (DSPExec's loop-top re-check),
+		 * not here.  JTRM Rev 8 p.38: an interrupt "forces a call" into
+		 * the instruction stream and pushes "the address of the last
+		 * instruction to be executed before the interrupt occurred" --
+		 * it is something the running DSP does, so a stopped DSP takes
+		 * nothing until DSPGO and an event callback must not move the
+		 * PC on its behalf.
+		 *
+		 * Vectoring synchronously from the event (JERRY I2S/PIT
+		 * callbacks run between scheduler slices) made the interrupt
+		 * visible to the 68000 for its entire next slice before the DSP
+		 * had executed a single cycle of the handler.  Club Drive
+		 * (#744) rewrites D_PC to $F1B030 from the 68000 at $807CE8
+		 * while DSPGO=1 (JTRM Rev 8 p.110 only promises D_PC writes
+		 * "whenever the DSP is idle"); when that write landed in the slice right
+		 * after an I2S event it found PC parked on the $F1B010 vector
+		 * with IMASK set, restarted the DSP's init with IMASK still
+		 * set (the init's read-modify-write of D_FLAGS writes the 1
+		 * back), and no interrupt was ever serviced again: LTXD stayed
+		 * 0 forever.  Which I2S phase the HLE boot happened to have
+		 * decided it -- 5 of 97 swept phases were silent, and the BIOS
+		 * boot merely had a lucky one.  For a running DSP the loop-top
+		 * re-check normally runs at the same DSP time the synchronous
+		 * call used to, with the same return address (dsp_pc - 2).  Two
+		 * exceptions: a deferred interrupt waits out an in-flight
+		 * DSP-issued D_FLAGS store (at most one instruction in real
+		 * code, see DSP_FLAGS_RETIRE_DELAY), and a stopped DSP is not
+		 * vectored until DSPGO.
+		 *
+		 * IMASKCleared doubles as "re-check pending interrupts at the
+		 * next boundary"; it already respects the D_FLAGS retire
+		 * hold-off, already gates the #569 idle-loop probe, and is
+		 * already in the savestate, so a latch raised in the last event
+		 * of a frame survives serialization. */
+		IMASKCleared = true;
 	}
 }
 
@@ -1278,11 +1320,11 @@ void DSPSyncToM68K(void)
  *     NOTE: unlike GPUExec, DSPExec has no slice-entry DSPHandleIRQs
  *     call at all.  The only in-loop dispatch is the
  *     `IMASKCleared && dspFlagsRetireDelay == 0` re-check at the top of
- *     DSPExec's loop below, and IMASKCleared is set only by a
- *     store to D_FLAGS (dsp.c:698) -- which the admission rule excludes.
- *     The other entry point, DSPSetIRQLine -> DSPHandleIRQsNP
- *     (dsp.c:925-935), is only ever reached from event callbacks, i.e.
- *     between slices, per (1).  The probe additionally requires
+ *     DSPExec's loop below.  IMASKCleared is set by a store to
+ *     D_FLAGS (dsp.c:698) -- which the admission rule excludes -- or by
+ *     DSPSetIRQLine latching an interrupt (since #744 it no longer
+ *     vectors there itself), reached from event callbacks between
+ *     slices, per (1), or from a D_CTRL DSPINT0 store -- again a store.  The probe additionally requires
  *     IMASKCleared == false and dspFlagsRetireDelay == 0, so neither
  *     the pending-IRQ path nor the D_FLAGS retire countdown is live.
  *

test/test_dsp_unit.c

@@ -443,20 +443,34 @@ static void test_int_ena_dispatch(void)
    /* Start the DSP with INT_ENA0 already enabled */
    p_DSPWriteLong(DSP_CTRL_ADDR, DSPGO, 6);
    p_DSPWriteLong(DSP_FLAGS_ADDR, INT_ENA0, 2);
+   /* That D_FLAGS write is DSP-issued (who=2), so it has a two-slot
+    * retire window (DSP_FLAGS_RETIRE_DELAY) during which the DSP does
+    * not take interrupts; run it out first. */
+   p_DSPExec(2);
 
    pc_before = *p_dsp_pc;
+   p_dsp_reg_bank_0[0] = 0;
 
-   /* Now assert the IRQ line — this should dispatch immediately */
+   /* Assert the IRQ line.  This only latches (#744): the DSP takes the
+    * interrupt itself at its next instruction boundary, so an event
+    * callback never moves the PC on the DSP's behalf. */
    p_DSPSetIRQLine(0, 1);
 
+   if (*p_dsp_pc == pc_before)
+      PASS("IRQ assert latched without moving PC (%08X)", pc_before);
+   else
+      FAIL("IRQ assert vectored synchronously: PC %08X -> %08X", pc_before, *p_dsp_pc);
+
+   /* One DSP step: take the interrupt, then run the handler's first
+    * instruction (moveq #7,r0 at vector 0). */
+   p_DSPExec(1);
+
    pc_after = *p_dsp_pc;
 
-   if (pc_after == DSP_RAM_BASE)
-      PASS("INT_ENA0 + IRQ assert dispatched to vector 0 ($F1B000)");
-   else if (pc_after != pc_before)
-      PASS("INT_ENA0 + IRQ assert changed PC (before=%08X after=%08X)", pc_before, pc_after);
+   if (pc_after == DSP_RAM_BASE + 2 && p_dsp_reg_bank_0[0] == 7)
+      PASS("INT_ENA0 + IRQ assert dispatched to vector 0 ($F1B000) at the next instruction");
    else
-      FAIL("INT_ENA0 + IRQ assert did NOT dispatch: PC stayed at %08X", pc_after);
+      FAIL("INT_ENA0 + IRQ assert did NOT dispatch: PC=%08X R0=%08X", pc_after, p_dsp_reg_bank_0[0]);
 
    /* Stop DSP */
    p_DSPWriteLong(DSP_CTRL_ADDR, 0, 6);
@@ -523,18 +537,25 @@ static void test_interrupt_priority(void)
    p_DSPWriteLong(DSP_PC_ADDR, 0xF1B800, 6);
    p_DSPWriteLong(DSP_CTRL_ADDR, DSPGO, 6);
    p_DSPWriteLong(DSP_FLAGS_ADDR, INT_ENA0 | INT_ENA1, 2);
+   /* That D_FLAGS write is DSP-issued (who=2), so it has a two-slot
+    * retire window (DSP_FLAGS_RETIRE_DELAY) during which the DSP does
+    * not take interrupts; run it out first. */
+   p_DSPExec(2);
 
    /* Assert IRQ1 — DSPSetIRQLine calls DSPHandleIRQsNP which now sees
     * both INT_LAT0+INT_ENA0 and INT_LAT1+INT_ENA1 pending.
     * The higher-priority one (IRQ1) should win. */
    p_DSPSetIRQLine(1, 1);
+   /* Taken at the DSP's next instruction boundary (#744); the one
+    * step also runs the handler's first instruction (a NOP). */
+   p_DSPExec(1);
 
    pc_after = *p_dsp_pc;
 
    /* Vector 1 = $F1B010, Vector 0 = $F1B000 */
-   if (pc_after == DSP_RAM_BASE + 0x10)
+   if (pc_after == DSP_RAM_BASE + 0x12)
       PASS("Highest-priority interrupt (IRQ1 -> $F1B010) dispatched");
-   else if (pc_after == DSP_RAM_BASE)
+   else if (pc_after == DSP_RAM_BASE + 0x02)
       FAIL("Lower-priority interrupt (IRQ0 -> $F1B000) dispatched instead of IRQ1, PC=%08X", pc_after);
    else
       FAIL("Unexpected PC after dual-interrupt dispatch: %08X", pc_after);
@@ -564,8 +585,25 @@ static void test_interrupt_return_address(void)
    p_DSPExec(1);
 
    p_DSPWriteLong(DSP_FLAGS_ADDR, INT_ENA0, 2);
+   /* That D_FLAGS write is DSP-issued (who=2), so it has a two-slot
+    * retire window (DSP_FLAGS_RETIRE_DELAY) during which the DSP does
+    * not take interrupts; run it out first. */
+   p_DSPExec(2);
    p_DSPSetIRQLine(0, 1);
 
+   /* Asserting the line only latches (#744); the PC must not move until
+    * the DSP itself takes the interrupt.  Ungated, unlike Test 7, so CI
+    * on every platform guards this contract. */
+   if (*p_dsp_pc == DSP_RAM_BASE + 0x106)
+      PASS("IRQ assert latched without moving PC");
+   else
+      FAIL("IRQ assert moved PC to $%08X before the DSP ran (expected $%08X)",
+            *p_dsp_pc, DSP_RAM_BASE + 0x106);
+
+   /* Taken at the DSP's next instruction boundary; the one step also
+    * runs the handler's first instruction (a NOP). */
+   p_DSPExec(1);
+
    saved_pc = read_dsp_ram32(0x8FC);
 
    if (p_dsp_reg_bank_0[31] == DSP_RAM_BASE + 0x8FC)
@@ -574,16 +612,18 @@ static void test_interrupt_return_address(void)
       FAIL("IRQ stack pointer = $%08X (expected $%08X)",
             p_dsp_reg_bank_0[31], DSP_RAM_BASE + 0x8FC);
 
-   if (saved_pc == DSP_RAM_BASE + 0x100)
+   /* Last instruction executed before the interrupt: the second of the
+    * two retire-window NOPs ($F1B102, $F1B104). */
+   if (saved_pc == DSP_RAM_BASE + 0x104)
       PASS("IRQ saved return PC $%08X", saved_pc);
    else
       FAIL("IRQ saved return PC $%08X (expected $%08X)",
-            saved_pc, DSP_RAM_BASE + 0x100);
+            saved_pc, DSP_RAM_BASE + 0x104);
 
-   if (*p_dsp_pc == DSP_RAM_BASE)
-      PASS("IRQ vectored PC to $%08X", *p_dsp_pc);
+   if (*p_dsp_pc == DSP_RAM_BASE + 2)
+      PASS("IRQ vectored PC to $%08X and ran the handler's first instruction", DSP_RAM_BASE);
    else
-      FAIL("IRQ PC = $%08X (expected $%08X)", *p_dsp_pc, DSP_RAM_BASE);
+      FAIL("IRQ PC = $%08X (expected $%08X)", *p_dsp_pc, DSP_RAM_BASE + 2);
 
    if (*p_dsp_control & INT_LAT0)
       PASS("IRQ latch remains set for handler acknowledgement");