Merge pull request #825 from libretro/fix/811-xenowings-input

fix(dsp): indexed store after a divide writes the old data (JTRM bug 2, #811 input)

docs/jtrm-errata.md

@@ -220,7 +220,7 @@ level 0 with no legend entry. HBW (26 Apr 1995) is a separate, partly overlappin
 | # | Title (short) | v8 pp. | Applies | Lvl | HBW xref | Modeled? |
 |---|---|---|---|---|---|---|
 | 1 | Unscaled 16-bit object fetch every 3 ticks (should be 2) | 133-134 | TOM/OP | 1 | n/a | no: OP cost is an occupancy estimate (`bus_arbiter_op_charge`), not tick-level |
-| 2 | Scoreboard failure on indexed-store data | 134 | TOM+JERRY | 1 | GPU/DSP #1 | no |
+| 2 | Scoreboard failure on indexed-store data | 134 | TOM+JERRY | 1 | GPU/DSP #1 | **DSP, partial**: `div` immediately followed by store 49/50/60/61 of its quotient register stores the old value (`dsp.c` `dsp_div_store_fixups`, #811: Xenowings' joypad reader relies on it). Not modeled: a later store inside the divide latency, the external-load variant, and the GPU |
 | 3 | Transparency with HILO set | 134 | TOM v1 only | 2 | n/a | n/a (not on production Tom v2) |
 | 4 | Horizontal Period register length | 134 | TOM | 0 | n/a | n/a (`TOMReset` writes HP 844/850) |
 | 5 | Clipping inefficiency (720-px line buffer) | 134-135 | TOM/OP | 1 | n/a | n/a, perf only (720 limit in `op.c`) |

src/jerry/dsp.c

@@ -362,6 +362,50 @@ static uint32_t *DSPLoadOverwritePeek(uint32_t *val, uint32_t *pc)
 	return dsp_ldx_slot;
 }
 
+/* Indexed store of a divide's result (issue #811).  JTRM Rev 8 p.134,
+ * TOM/JERRY bug 2 "Scoreboard Failure on Indexed Addressing Mode Stores"
+ * ("applies to both Tom & Jerry"): the data of store instructions 49,
+ * 50, 60 and 61 is not scoreboarded, so when it is the result of a
+ * divide still in flight the store writes the register's OLD contents;
+ * the address registers stay fully protected.  Xenowings' DSP joypad
+ * reader ends with `div r9,r8 / store r8,(r14+1)` and relies on this:
+ * the joypad bits go to $F1C3A8 and the quotient never does.  Writing
+ * the quotient instead fed the game garbage and the ship never moved.
+ * Modeled for the instruction immediately after the divide only (the
+ * documented pattern; an intervening instruction may be the manual's
+ * dependent "protection instruction", which this does not decode).
+ * Diagnostics (test ABI): stale-data stores applied since DSPReset.
+ * Monotonic, not serialized. */
+uint64_t dsp_div_store_fixups = 0;
+
+/* Called right after a divide executed, dsp_pc at the next instruction.
+ * Returns the divide's destination slot if that instruction is an
+ * indexed store whose data register is that slot and whose address
+ * registers are not, else NULL.  Only peeks DSP local RAM. */
+static uint32_t *DSPDivStorePeek(uint32_t *divDst, uint32_t *pc)
+{
+	uint32_t off, n, p1, p2;
+	uint16_t op;
+
+	if (dsp_pc < DSP_WORK_RAM_BASE || dsp_pc >= DSP_WORK_RAM_BASE + 0x2000 - 1)
+		return NULL;
+	off = dsp_pc - DSP_WORK_RAM_BASE;
+	op  = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1];
+	n   = op >> 10;
+	p1  = (op >> 5) & 0x1F;
+	p2  = op & 0x1F;
+	if (n != 49 && n != 50 && n != 60 && n != 61)
+		return NULL;
+	if (&dsp_reg[p2] != divDst)
+		return NULL;
+	if (((n == 49 || n == 60) && p2 == 14)
+	      || ((n == 50 || n == 61) && p2 == 15)
+	      || ((n == 60 || n == 61) && p1 == p2))
+		return NULL;
+	*pc = dsp_pc;
+	return divDst;
+}
+
 static uint32_t dspgo_poll_count;
 
 #define BRANCH_CONDITION(x)		dsp_branch_condition_table[(x) + ((jaguar_flags & 7) << 5)]
@@ -1115,6 +1159,7 @@ void DSPReset(void)
 	dsp_pc				  = 0x00F1B000;
 	dspgo_poll_count	  = 0;
 	dsp_load_wb_fixups	  = 0;
+	dsp_div_store_fixups  = 0;
 	dsp_ldx_slot		  = NULL;
 	dsp_acc				  = 0x00000000;
 	dsp_remain			  = 0x00000000;
@@ -1385,6 +1430,7 @@ static int32_t   idleProbeCyc0, idleProbeCyc1;
 static uint32_t  idleProbeOpc0, idleProbeOpc1;
 static uint32_t  idleProbeS0[64], idleProbeS1[64];
 static uint64_t  idleProbeLdxFix;		/* dsp_load_wb_fixups at S0 */
+static uint64_t  idleProbeDivFix;		/* dsp_div_store_fixups at S0 */
 static uint8_t   idleProbeFz0, idleProbeFn0, idleProbeFc0;
 static uint8_t   idleProbeFz1, idleProbeFn1, idleProbeFc1;
 
@@ -1790,6 +1836,7 @@ static int32_t DSPIdleLoopProbe(int32_t cycles, uint32_t head, uint32_t jrAddr)
 		idleProbeJr    = jrAddr;
 		idleProbeBank  = dsp_reg;
 		idleProbeLdxFix = dsp_load_wb_fixups;
+		idleProbeDivFix = dsp_div_store_fixups;
 		idleProbeStage = 1;
 		return cycles;
 	}
@@ -1824,8 +1871,10 @@ static int32_t DSPIdleLoopProbe(int32_t cycles, uint32_t head, uint32_t jrAddr)
 	/* Issue #811: dsp_idle_check_body models a load followed by an
 	 * overwrite of its target in program order (the overwrite wins), but
 	 * the interpreter now lets the external load data land last.  A loop
-	 * in which that fired is not the loop the dataflow proof describes. */
-	if (dsp_load_wb_fixups != idleProbeLdxFix)
+	 * in which that fired is not the loop the dataflow proof describes.
+	 * Same for a divide whose indexed store wrote the pre-divide value. */
+	if (dsp_load_wb_fixups != idleProbeLdxFix
+	      || dsp_div_store_fixups != idleProbeDivFix)
 	{
 		dsp_idle_memo_reject(head, jrAddr);
 		return cycles;
@@ -1950,6 +1999,10 @@ void DSPExec(int32_t cycles)
 	 * instruction (see dsp_ldx_slot). */
 	uint32_t *ldxArmed = NULL;
 	uint32_t ldxVal = 0, ldxPc = 0;
+	/* Issue #811 stale indexed-store data after a divide (see
+	 * dsp_div_store_fixups), armed for exactly the next instruction. */
+	uint32_t *divArmed = NULL;
+	uint32_t divOld = 0, divPc = 0;
 
 #ifdef DSP_SINGLE_STEPPING
 	if (dsp_control & 0x18)
@@ -2041,8 +2094,9 @@ void DSPExec(int32_t cycles)
 	dsp_ldx_slot   = NULL;
 
 	/* An armed late write-back runs one instruction past the budget so a
-	 * load/overwrite pair never straddles a slice (see dsp_ldx_slot). */
-	while ((cycles > 0 || ldxArmed) && DSP_RUNNING)
+	 * load/overwrite pair never straddles a slice (see dsp_ldx_slot);
+	 * likewise a divide/indexed-store pair (see dsp_div_store_fixups). */
+	while ((cycles > 0 || ldxArmed || divArmed) && DSP_RUNNING)
 	{
       uint16_t opcode;
       uint32_t index;
@@ -2124,7 +2178,34 @@ void DSPExec(int32_t cycles)
 		dsp_opcode_second_parameter = opcode & 0x1F;
 		dsp_pc += 2;
 		dsp_exec_opcode_count++;
-		dsp_executeOpcode(index);
+		if (divArmed && pcThis == divPc
+		      && &dsp_reg[dsp_opcode_second_parameter] == divArmed)
+		{
+			/* Issue #811: indexed store right behind a divide into its
+			 * data register -- the quotient has not been written back
+			 * yet, so the store sees the old contents. */
+			uint32_t quot = *divArmed;
+
+			*divArmed = divOld;
+			dsp_executeOpcode(index);
+			*divArmed = quot;
+			dsp_div_store_fixups++;
+			divArmed = NULL;
+		}
+		else if (index == 21)
+		{
+			uint32_t *dst = &dsp_reg[dsp_opcode_second_parameter];
+			uint32_t old = *dst;
+
+			dsp_executeOpcode(index);
+			divArmed = DSPDivStorePeek(dst, &divPc);
+			divOld = old;
+		}
+		else
+		{
+			dsp_executeOpcode(index);
+			divArmed = NULL;
+		}
 		cycles -= dsp_opcode_cycles[index];
 
 		/* Issue #811: the previous instruction was an external load and

test/tools/xenowings_ingame_video.c

@@ -39,6 +39,25 @@
  *     render.  The model lives in src/m68000/m68kinterface.c;
  *     test/test_m68k_prefetch.c pins it without a ROM.
  *
+ * 5. Input reaches the ship.  The run is made twice from one process (a
+ *    forked child is the reference arm): identical up to the first lit
+ *    gameplay frame, then the input arm holds RIGHT for the rest of the
+ *    window.  Within INPUT_WINDOW frames of that press the two frame-hash
+ *    streams must diverge, in at least MIN_DIFF_FRAMES frames.  Steps 1-4
+ *    are judged on the reference (no-input) arm.
+ *
+ *    The bug this catches: the DSP joypad reader ends with
+ *
+ *        div   r9,r8          ; r8 = joypad bits
+ *        store r8,(r14+1)     ; -> $F1C3A8, read by the game
+ *
+ *    JTRM Rev 8 p.134, TOM/JERRY bug 2: indexed-store data is not
+ *    scoreboarded, so on silicon the store writes r8 BEFORE the divide's
+ *    quotient lands -- the joypad bits.  The core stored the quotient,
+ *    the game saw junk, and the ship never moved: the held-input stream
+ *    was identical to the no-input one.  Requires the DSP bug-13 fix too
+ *    (`load (r0=$F14000),r1 / moveq #0,r1` at F1BBA0), else r8 is 0.
+ *
  * Exit status: 0 pass, 1 fail.
  *
  * Usage:
@@ -50,12 +69,18 @@
  *      test/harness/harness.c -ldl -lm
  */
 
+/* fork/waitpid under -std=c99 on glibc.  Must precede the first system
+ * header include. */
+#define _POSIX_C_SOURCE 200809L
+
 #include "../harness/harness.h"
 #include "libretro.h"
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/wait.h>
+#include <unistd.h>
 
 #define MENU_MIN_WIDTH     600u   /* menu is 652 wide; everything else 326 */
 #define MENU_LIT_PCT       20.0
@@ -67,6 +92,8 @@
 #define GAME_WINDOW        1200u  /* frames after the loading screen */
 #define GAME_LIT_PCT       50.0
 #define MIN_RUN_FRAMES     600u   /* consecutive lit gameplay frames */
+#define INPUT_WINDOW       600u   /* first divergence after the press; ~380 measured */
+#define MIN_DIFF_FRAMES    60u    /* frames that must differ in the window */
 
 enum { PH_MENU, PH_LOADING, PH_GAME, PH_DONE };
 
@@ -75,6 +102,9 @@ typedef struct {
     int      phase;
     unsigned w;
     double   pct;
+    uint64_t hash;          /* FNV-1a of the last real frame */
+    int      held;          /* input arm: hold RIGHT from first_lit */
+    uint64_t hashes[GAME_WINDOW + 1];   /* by frame - loading_frame */
     unsigned menu_run;
     unsigned press_frame;
     unsigned loading_frame;
@@ -95,15 +125,19 @@ static void xw_video(void *ud, const void *data, unsigned width,
 {
     xw_state *s = (xw_state *)ud;
     unsigned x, y, lit = 0;
+    uint64_t h = 1469598103934665603ULL;
 
     if (!data)
         return;               /* duped frame: keep the previous reading */
     for (y = 0; y < height; y++) {
         const uint32_t *row = (const uint32_t *)((const uint8_t *)data + y * pitch);
-        for (x = 0; x < width; x++)
+        for (x = 0; x < width; x++) {
             if (row[x] & 0x00FFFFFFu)
                 lit++;
+            h = (h ^ (row[x] & 0x00FFFFFFu)) * 1099511628211ULL;
+        }
     }
+    s->hash = h;
     s->w   = width;
     s->pct = (width && height) ? 100.0 * (double)lit / ((double)width * height) : 0.0;
 }
@@ -138,12 +172,18 @@ static bool xw_frame(void *ud, unsigned f)
         }
         break;
     case PH_GAME:
+        if (f - s->loading_frame <= GAME_WINDOW)
+            s->hashes[f - s->loading_frame] = s->hash;
         if (s->w < MENU_MIN_WIDTH) {
             if (s->pct > s->best_pct)
                 s->best_pct = s->pct;
             if (s->pct >= GAME_LIT_PCT) {
-                if (!s->lit_frames)
+                if (!s->lit_frames) {
                     s->first_lit = f;
+                    if (s->held)
+                        harness_press(s->cfg, 0, RETRO_DEVICE_ID_JOYPAD_RIGHT,
+                                      f + 1, GAME_WINDOW);
+                }
                 s->lit_frames++;
                 if (!s->run || s->pct < s->run_min_pct)
                     s->run_min_pct = s->pct;
@@ -178,51 +218,119 @@ static bool xw_frame(void *ud, unsigned f)
     return s->phase != PH_DONE;
 }
 
+/* One full run.  `held` selects the input arm. */
+static void xw_run_arm(int argc, char **argv, int held, xw_state *s)
+{
+    harness_config cfg = HARNESS_CONFIG_DEFAULT;
+
+    memset(s, 0, sizeof *s);
+    s->cfg = &cfg;
+    s->phase = PH_MENU;
+    s->held = held;
+    cfg.frames = MENU_TIMEOUT + LOADING_TIMEOUT + GAME_WINDOW + 60u;
+
+    if (!harness_init_from_args(&cfg, argc, argv)) {
+        s->fail = "harness init failed";
+        return;
+    }
+    cfg.video_callback      = xw_video;
+    cfg.video_callback_data = s;
+    cfg.frame_callback      = xw_frame;
+    cfg.frame_callback_data = s;
+    if (!harness_load_rom(&cfg)) {
+        s->fail = "ROM load failed";
+        return;
+    }
+    harness_run(&cfg);
+    if (!s->fail && s->phase != PH_DONE)
+        s->fail = "frame budget ran out before the in-game window closed";
+    harness_shutdown(&cfg);
+    s->cfg = NULL;
+}
+
 int main(int argc, char **argv)
 {
     harness_config cfg = HARNESS_CONFIG_DEFAULT;
-    xw_state s;
-    char detail[256];
+    static xw_state s, in;
+    char detail[512];
     harness_result res;
-    int pass;
+    FILE *xfer;
+    pid_t pid;
+    int status = 0, pass, input_ok = 0, child_failed;
+    unsigned i, ndiff = 0, first_diff = 0, press_rel = 0;
 
-    memset(&s, 0, sizeof s);
-    s.cfg = &cfg;
-    s.phase = PH_MENU;
-    cfg.frames = MENU_TIMEOUT + LOADING_TIMEOUT + GAME_WINDOW + 60u;
-
-    if (!harness_init_from_args(&cfg, argc, argv))
+    /* The reference (no-input) arm runs in a forked child, so each arm
+     * gets fresh core statics, and hands its state back through a temp
+     * file; the parent runs the input arm concurrently.  `fail` points
+     * into the child's address space, so only its non-NULL-ness crosses. */
+    xfer = tmpfile();
+    if (!xfer) {
+        perror("tmpfile");
         return 1;
-    cfg.video_callback      = xw_video;
-    cfg.video_callback_data = &s;
-    cfg.frame_callback      = xw_frame;
-    cfg.frame_callback_data = &s;
-    if (!harness_load_rom(&cfg))
+    }
+    fflush(NULL);
+    pid = fork();
+    if (pid < 0) {
+        perror("fork");
         return 1;
+    }
+    if (pid == 0) {
+        xw_run_arm(argc, argv, 0, &s);
+        fwrite(&s, sizeof s, 1, xfer);
+        fflush(xfer);
+        _exit(0);
+    }
+    xw_run_arm(argc, argv, 1, &in);
+    if (waitpid(pid, &status, 0) != pid || !WIFEXITED(status)
+            || fseek(xfer, 0, SEEK_SET) != 0 || fread(&s, sizeof s, 1, xfer) != 1) {
+        fprintf(stderr, "xenowings_ingame_video: reference arm died\n");
+        return 1;
+    }
+    fclose(xfer);
+    child_failed = s.fail != NULL;
+    s.fail = child_failed ? "reference (no-input) arm failed to reach gameplay" : NULL;
 
-    harness_run(&cfg);
+    if (!s.fail && !in.fail && in.loading_frame == s.loading_frame
+            && in.first_lit == s.first_lit && s.first_lit) {
+        press_rel = s.first_lit + 1 - s.loading_frame;
+        for (i = press_rel; i <= GAME_WINDOW; i++) {
+            if (in.hashes[i] != s.hashes[i]) {
+                if (!ndiff)
+                    first_diff = i;
+                ndiff++;
+            }
+        }
+        input_ok = ndiff >= MIN_DIFF_FRAMES && first_diff - press_rel <= INPUT_WINDOW;
+    }
 
-    if (!s.fail && s.phase != PH_DONE)
-        s.fail = "frame budget ran out before the in-game window closed";
-    pass = !s.fail && s.best_run >= MIN_RUN_FRAMES && s.reloads == 0;
-    if (s.fail)
-        snprintf(detail, sizeof detail, "%s", s.fail);
+    pass = !s.fail && !in.fail && s.best_run >= MIN_RUN_FRAMES && s.reloads == 0
+        && input_ok;
+    if (s.fail || in.fail)
+        snprintf(detail, sizeof detail, "%s", s.fail ? s.fail : in.fail);
     else
         snprintf(detail, sizeof detail,
                  "menu B at frame %u, loading at %u, %u in-game frame(s) >= %.0f%% lit "
                  "(first %u, best %.1f%%), longest streak %u (dimmest %.1f%%, need %u), "
-                 "%u return(s) to the loader%s",
+                 "%u return(s) to the loader; RIGHT held from +%u: %u frame(s) differ "
+                 "from no input, first at +%u (need >= %u, first within +%u)%s",
                  s.press_frame, s.loading_frame, s.lit_frames, GAME_LIT_PCT,
                  s.first_lit, s.best_pct, s.best_run, s.best_run_min_pct,
-                 MIN_RUN_FRAMES, s.reloads,
+                 MIN_RUN_FRAMES, s.reloads, press_rel, ndiff, first_diff,
+                 MIN_DIFF_FRAMES, INPUT_WINDOW,
                  pass ? "" :
                  !s.lit_frames ? " -- gameplay is black (#811: OLP=0 from the load/moveq race?)"
-                               : " -- gameplay did not last (#811: 68000 prefetch / anti-tamper at $350C?)");
+                 : (s.best_run < MIN_RUN_FRAMES || s.reloads)
+                               ? " -- gameplay did not last (#811: 68000 prefetch / anti-tamper at $350C?)"
+                 : (in.loading_frame != s.loading_frame || in.first_lit != s.first_lit)
+                               ? " -- arms diverged before any input (nondeterminism?)"
+                               : " -- input has no effect (#811: DSP joypad reader, JTRM bugs 2/13?)");
 
+    /* harness_report only needs the parsed output mode. */
+    if (!harness_init_from_args(&cfg, argc, argv))
+        return 1;
     res.status = pass ? "PASS" : "FAIL";
     res.name   = "xenowings_ingame_video";
     res.detail = detail;
     harness_report(&cfg, &res, 1);
-    harness_shutdown(&cfg);
     return pass ? 0 : 1;
 }