Merge pull request #282 from Alexbeav/pr/cdrom-seek-semantics

runtime: correct CD-ROM seek and read handoff

docs/disc-speed.md

@@ -49,6 +49,11 @@ Change is localized to `runtime/src/cdrom.c`. The timing delay paths (seek durat
 read interval) check `disc_speed` and either apply the emulated delay or skip it. No other
 subsystem is affected.
 
+The CD-ROM state sequence does not change at higher speeds. `Setloc` followed by `ReadN` or
+`ReadS` keeps the drive in `SEEK` until the logical seek is complete. The drive then changes
+to `READ` and delivers the requested sector first. An explicit `SeekL` or `SeekP` command
+stops any active read stream before it starts the new seek.
+
 ---
 
 ## Relation to overlay-discovery

recompiler/CMakeLists.txt

@@ -356,6 +356,24 @@ if(BUILD_TESTING)
         ../runtime/include)
     add_test(NAME debug_trace_ranges_test COMMAND debug_trace_ranges_test)
 
+    add_executable(cdrom_seek_semantics_test
+        ../runtime/tests/test_cdrom_seek_semantics.c
+        ../runtime/src/cdrom.c)
+    target_include_directories(cdrom_seek_semantics_test PRIVATE
+        ../runtime/include
+        ../runtime/src)
+    target_compile_definitions(cdrom_seek_semantics_test PRIVATE
+        PSX_CDROM_SEEK_SEMANTICS_TEST=1)
+    if(MSVC)
+        target_compile_options(cdrom_seek_semantics_test PRIVATE /Gy)
+        target_link_options(cdrom_seek_semantics_test PRIVATE /OPT:REF)
+    else()
+        target_compile_options(cdrom_seek_semantics_test PRIVATE
+            -ffunction-sections -fdata-sections)
+        target_link_options(cdrom_seek_semantics_test PRIVATE -Wl,--gc-sections)
+    endif()
+    add_test(NAME cdrom_seek_semantics_test COMMAND cdrom_seek_semantics_test)
+
     add_executable(cli_boot_path_test
         tests/cli_boot_path_test.cpp
         src/cli_boot_path.cpp

runtime/CMakeLists.txt

@@ -341,6 +341,22 @@ if(BUILD_TESTING)
     target_include_directories(cdrom_lid_state_test PRIVATE src)
     add_test(NAME cdrom_lid_state_test COMMAND cdrom_lid_state_test)
 
+    add_executable(cdrom_seek_semantics_test
+        tests/test_cdrom_seek_semantics.c
+        src/cdrom.c)
+    target_include_directories(cdrom_seek_semantics_test PRIVATE include src)
+    target_compile_definitions(cdrom_seek_semantics_test PRIVATE
+        PSX_CDROM_SEEK_SEMANTICS_TEST=1)
+    if(MSVC)
+        target_compile_options(cdrom_seek_semantics_test PRIVATE /Gy)
+        target_link_options(cdrom_seek_semantics_test PRIVATE /OPT:REF)
+    else()
+        target_compile_options(cdrom_seek_semantics_test PRIVATE
+            -ffunction-sections -fdata-sections)
+        target_link_options(cdrom_seek_semantics_test PRIVATE -Wl,--gc-sections)
+    endif()
+    add_test(NAME cdrom_seek_semantics_test COMMAND cdrom_seek_semantics_test)
+
     # Cross-device I_STAT ownership: a memory-card ACK IRQ (SIO0) must not
     # clear another device's pending source — pinned on SPU (I_STAT bit 9)
     # staying set across card ACK delivery until the guest acknowledges it.

runtime/src/cdrom.c

@@ -1594,7 +1594,31 @@ static int read_continues_current_stream(void) {
     return 1;
 }
 
+static int read_start_delay_cycles(int implicit_seek) {
+    return implicit_seek
+        ? seek_complete_delay_cycles() + sector_delay_cycles()
+        : initial_read_delay_cycles();
+}
+
+static void apply_read_start_status(int implicit_seek) {
+    if (implicit_seek) {
+        stat_reg &= (uint8_t)~(CDSTAT_READ | CDSTAT_PLAY);
+        stat_reg |= CDSTAT_SEEK;
+    } else {
+        stat_reg &= (uint8_t)~CDSTAT_SEEK;
+        stat_reg |= CDSTAT_READ;
+    }
+}
+
+static void finish_implicit_seek_status(void) {
+    if (!(stat_reg & CDSTAT_SEEK)) return;
+    stat_reg &= (uint8_t)~CDSTAT_SEEK;
+    stat_reg |= CDSTAT_READ;
+    setloc_seek_far = 0;
+}
+
 static void start_read_stream(uint8_t cmd) {
+    int implicit_seek = setloc_pending;
     cdda_playing = 0;
     cdda_track = 0;
     cdda_delay = 0;
@@ -1613,13 +1637,16 @@ static void start_read_stream(uint8_t cmd) {
     read_sec = seek_sec;
     read_sect = seek_sect;
     read_cmd = cmd;
-    read_delay = initial_read_delay_cycles();
+    /* An unprocessed Setloc makes ReadN/ReadS perform a logical seek before
+     * the first sector. Keep the current bounded seek model, but do not let
+     * sector delivery begin while that seek is still active. */
+    read_delay = read_start_delay_cycles(implicit_seek);
     s_cd_probe_read_start_count++;
     s_cd_probe_read_start_cycles += (uint64_t)read_delay;
     s_cd_timing_next_due = psx_cycle_count + (uint64_t)read_delay;
     s_cd_timing_stream_starts++;
     reading = 1;
-    stat_reg |= CDSTAT_READ;
+    apply_read_start_status(implicit_seek);
     /* ENQUEUE: sector-read stream scheduled (due in read_delay cycles). A
      * content load that happens in OFF but not ON shows up as a missing
      * SRC_CD_READ enqueue here. */
@@ -1654,6 +1681,13 @@ static void stop_read_stream(void) {
     cdrom_clear_pending_dataready();
 }
 
+static void take_explicit_seek_ownership(void) {
+    stop_read_stream();
+    clear_sector_buffer();
+    stat_reg &= (uint8_t)~(CDSTAT_READ | CDSTAT_PLAY);
+    stat_reg |= CDSTAT_SEEK;
+}
+
 static void stop_cdda_playback(void) {
     cdda_playing = 0;
     cdda_track = 0;
@@ -2386,10 +2420,13 @@ static void exec_command(uint8_t cmd) {
             set_irq(CDIRQ_ERROR);
             break;
         }
+        /* A seek owns the drive and cancels any prior ReadN/ReadS generation.
+         * Otherwise the old stream continues during the seek and later reads
+         * never consume the new Setloc target. */
+        take_explicit_seek_ownership();
         xa_reset_decode();
         spu_cd_audio_reset();
         stop_cdda_playback();
-        stat_reg |= CDSTAT_SEEK;
         response_push(stat_reg);
         set_irq(CDIRQ_ACK);
         {
@@ -2533,7 +2570,7 @@ static void process_pending(uint32_t cycles) {
         stat_reg &= ~CDSTAT_SEEK;
         stat_reg |= CDSTAT_READ;   /* PSX-CD-003: GT1 waits for READ after seek */
         setloc_seek_far = 0;
-    setloc_pending = 0;
+        setloc_pending = 0;
         response_push(stat_reg);
         set_irq(CDIRQ_COMPLETE);
         fire_cdrom_irq();
@@ -2674,6 +2711,9 @@ static void process_read_stream(uint32_t cycles) {
     }
 
     if (read_delay <= 0) {
+        /* The implicit Setloc seek completes when the first requested sector
+         * becomes eligible. Status changes from SEEK to READ at this point. */
+        finish_implicit_seek_status();
         uint64_t timing_seq = cd_timing_begin_sector(
             msf_to_lba(read_min, read_sec, read_sect));
         if (irq_flag == 0) {
@@ -2800,6 +2840,7 @@ void cdrom_init(const char* cue_path) {
     seek_min = seek_sec = seek_sect = 0;
     s_setloc_lba = -1;
     setloc_seek_far = 0;
+    setloc_pending = 0;
     s_warm_routes_count = 0;
     s_warm_route_configured = 0;
     s_warm_route_enabled = 0;
@@ -3276,6 +3317,17 @@ int cdrom_savestate_boost_vblanks_remaining(void) {
 /* LE field wire — PendingCmd/QueuedCmd have host padding. */
 #include "pst_wire.h"
 
+/* Keep the existing four-byte wire slot. Old snapshots stored only 0 or 1
+ * here, so bit 1 is free for the pending Setloc state. */
+static int32_t pack_setloc_snapshot_state(void) {
+    return (setloc_seek_far ? 1 : 0) | (setloc_pending ? 2 : 0);
+}
+
+static void unpack_setloc_snapshot_state(int32_t state) {
+    setloc_seek_far = (state & 1) != 0;
+    setloc_pending = (state & 2) != 0;
+}
+
 static int cdrom_snap_emit(PstW *w) {
 #define W8(f)  do { if (!pst_w_u8(w, (uint8_t)(f))) return 0; } while (0)
 #define WI(f)  do { if (!pst_w_i32(w, (int32_t)(f))) return 0; } while (0)
@@ -3311,7 +3363,8 @@ static int cdrom_snap_emit(PstW *w) {
     WU(0u); W8(last_sector_mode); W8(last_sector_have_raw);
     W8(last_sector_raw_mode); W8(last_sector_xa_file); W8(last_sector_xa_channel);
     W8(last_sector_xa_submode); W8(last_sector_xa_coding);
-    W8(seek_min); W8(seek_sec); W8(seek_sect); WI(s_setloc_lba); WI(setloc_seek_far);
+    W8(seek_min); W8(seek_sec); W8(seek_sect); WI(s_setloc_lba);
+    WI(pack_setloc_snapshot_state());
     WI(reading); WI(read_min); WI(read_sec); WI(read_sect); W8(mode_reg);
     W8(read_cmd); WI(read_delay); W8(filter_file); W8(filter_channel); W8(cd_muted);
     WI(cdda_playing); WI(cdda_track); WU(cdda_lba); WI(cdda_delay);
@@ -3377,7 +3430,8 @@ static int cdrom_snap_parse(PstR *r) {
     RU(last_sector_frame); R8(last_sector_mode); R8(last_sector_have_raw);
     R8(last_sector_raw_mode); R8(last_sector_xa_file); R8(last_sector_xa_channel);
     R8(last_sector_xa_submode); R8(last_sector_xa_coding);
-    R8(seek_min); R8(seek_sec); R8(seek_sect); RI(s_setloc_lba); RI(setloc_seek_far); setloc_pending = 0;
+    R8(seek_min); R8(seek_sec); R8(seek_sect); RI(s_setloc_lba);
+    RI(i); unpack_setloc_snapshot_state(i);
     RI(reading); RI(read_min); RI(read_sec); RI(read_sect); R8(mode_reg);
     R8(read_cmd); RI(read_delay); R8(filter_file); R8(filter_channel); R8(cd_muted);
     RI(cdda_playing); RI(cdda_track); RU(cdda_lba); RI(cdda_delay);
@@ -3436,6 +3490,90 @@ int cdrom_snapshot_read(const uint8_t *p, uint32_t len) {
     return 1;
 }
 
+#ifdef PSX_CDROM_SEEK_SEMANTICS_TEST
+int cdrom_test_init_clears_setloc(void) {
+    setloc_seek_far = 1;
+    setloc_pending = 1;
+    cdrom_init(NULL);
+    return setloc_seek_far || setloc_pending;
+}
+
+int cdrom_test_explicit_seek_ownership(void) {
+    int failures = 0;
+    reading = 1;
+    read_cmd = 0x06;
+    read_delay = 1234;
+    pending_dataready = 1;
+    pending_dataready_stat = CDSTAT_READ;
+    pending_present_due = 5678;
+    request_reg |= CDROM_REQUEST_BFRD;
+    stat_reg = CDSTAT_MOTOR | CDSTAT_READ | CDSTAT_PLAY;
+    iso_handle = (void *)(uintptr_t)1;
+    irq_flag = 0;
+    for (int slot = 0; slot < CDROM_NUM_SECTOR_BUFFERS; slot++) {
+        s_sector_ring[slot].size = 32 + slot;
+        s_sector_ring[slot].pos = slot;
+    }
+
+    exec_command(0x15);
+
+    if (reading || read_cmd || read_delay || s_cd_timing_next_due) failures |= 1;
+    if (pending_dataready || pending_dataready_stat || pending_present_due) failures |= 2;
+    if (request_reg & CDROM_REQUEST_BFRD) failures |= 4;
+    for (int slot = 0; slot < CDROM_NUM_SECTOR_BUFFERS; slot++) {
+        if (s_sector_ring[slot].size || s_sector_ring[slot].pos) failures |= 8;
+    }
+    if ((stat_reg & (CDSTAT_READ | CDSTAT_PLAY | CDSTAT_SEEK)) != CDSTAT_SEEK)
+        failures |= 16;
+    return failures;
+}
+
+int cdrom_test_implicit_seek_sequence(void) {
+    const int expected = CDROM_SINGLE_SPEED_SECTOR_CYCLES * 4
+                       + CDROM_SINGLE_SPEED_SECTOR_CYCLES / 2;
+    int failures = 0;
+    mode_reg = 0x80;
+    g_disc_speed_divisor = 1;
+    xa_stream_active = 0;
+    s_warm_route_active = 0;
+    reading = 0;
+    pending.pending = 0;
+    iso_handle = (void *)(uintptr_t)1;
+    last_sector_lba = 0;
+    stat_reg = CDSTAT_MOTOR | CDSTAT_READ | CDSTAT_PLAY;
+    irq_flag = 0;
+    param_fifo[0] = 0x00;
+    param_fifo[1] = 0x10;
+    param_fifo[2] = 0x00;
+    param_count = 3;
+
+    exec_command(0x02);
+    irq_flag = 0;
+    exec_command(0x06);
+
+    if (read_delay != expected) failures |= 1;
+    if ((stat_reg & (CDSTAT_READ | CDSTAT_PLAY | CDSTAT_SEEK)) != CDSTAT_SEEK)
+        failures |= 2;
+    if (setloc_pending) failures |= 4;
+    irq_flag = 0;
+    process_read_stream((uint32_t)expected);
+    if ((stat_reg & (CDSTAT_READ | CDSTAT_PLAY | CDSTAT_SEEK)) != CDSTAT_READ)
+        failures |= 8;
+    if (setloc_seek_far) failures |= 16;
+    if (last_sector_lba != s_setloc_lba) failures |= 32;
+    return failures;
+}
+
+void cdrom_test_set_setloc_state(int far_state, int pending_state) {
+    setloc_seek_far = far_state != 0;
+    setloc_pending = pending_state != 0;
+}
+
+int cdrom_test_get_setloc_state(void) {
+    return pack_setloc_snapshot_state();
+}
+#endif
+
 void debug_force_cd_reinsert(void) {
     /* Stop the old transfer before exposing a physical tray-open event. */
     stop_read_stream();

runtime/tests/test_cdrom_seek_semantics.c

@@ -0,0 +1,130 @@
+/*
+ * Validate the production CD-ROM seek helpers and snapshot round trip.
+ *
+ * Build/run: ctest -R cdrom_seek_semantics_test
+ */
+#include "cdrom.h"
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+uint64_t psx_cycle_count;
+uint32_t i_stat;
+uint32_t g_debug_current_func_addr;
+uint32_t g_debug_last_store_pc;
+uint64_t s_frame_count;
+
+/* cdrom.c is compiled as one production translation unit. Most host edges are
+ * inert linker stubs. iso_read_sector is the deliberate exception: the
+ * implicit-seek sequence uses it to complete the target-sector delivery. */
+void psx_irq_raise(uint32_t bit, uint32_t detail) { (void)bit; (void)detail; }
+void event_ring_record(uint16_t kind, uint8_t detail) { (void)kind; (void)detail; }
+void event_ring_record_aux(uint16_t kind, uint8_t detail, uint32_t aux) {
+    (void)kind; (void)detail; (void)aux;
+}
+void audio_trace_event(uint16_t kind, uint32_t a, uint32_t b) {
+    (void)kind; (void)a; (void)b;
+}
+uint32_t interrupts_get_cycles_since_vblank(void) { return 0; }
+int dma_cdrom_transfer_active(void) { return 0; }
+void spu_cd_audio_push(const int16_t *stereo, int frames) {
+    (void)stereo; (void)frames;
+}
+void spu_cd_audio_reset(void) {}
+int psx_netplay_active(void) { return 0; }
+int psx_netplay_cd_bisect_active(void) { return 0; }
+int psx_netplay_is_resimulating(void) { return 0; }
+uint32_t psx_netplay_sim_tick(void) { return 0; }
+
+void *iso_open(const char *path) { (void)path; return NULL; }
+void iso_close(void *handle) { (void)handle; }
+int iso_read_sector(void *handle, uint32_t lba, uint8_t *buffer, int size) {
+    (void)handle; (void)lba;
+    memset(buffer, 0, (size_t)size);
+    return 1;
+}
+int iso_read_raw_sector(void *handle, uint32_t lba, uint8_t *buffer, int size) {
+    (void)handle; (void)lba; (void)buffer; (void)size; return 0;
+}
+int iso_read_subq(void *handle, uint32_t lba, uint8_t *buffer, int size,
+                  int *valid) {
+    (void)handle; (void)lba; (void)buffer; (void)size;
+    if (valid) *valid = 0;
+    return 0;
+}
+int iso_has_subq_replacements(void *handle) { (void)handle; return 0; }
+uint32_t iso_sector_count(void *handle) { (void)handle; return 0; }
+int iso_track_count(void *handle) { (void)handle; return 0; }
+uint32_t iso_track_start_lba(void *handle, int track) {
+    (void)handle; (void)track; return 0;
+}
+uint32_t iso_track_pregap_lba(void *handle, int track) {
+    (void)handle; (void)track; return 0;
+}
+int iso_track_is_audio(void *handle, int track) {
+    (void)handle; (void)track; return 0;
+}
+
+int cdrom_test_explicit_seek_ownership(void);
+int cdrom_test_implicit_seek_sequence(void);
+int cdrom_test_init_clears_setloc(void);
+void cdrom_test_set_setloc_state(int far_state, int pending_state);
+int cdrom_test_get_setloc_state(void);
+
+static int failures;
+
+#define EXPECTED_CDROM_SNAPSHOT_BYTES 4931u
+
+#define CHECK(condition, label) do {                                         \
+    if (!(condition)) {                                                      \
+        fprintf(stderr, "FAIL: %s\n", (label));                              \
+        failures++;                                                          \
+    }                                                                        \
+} while (0)
+
+int main(void) {
+    uint32_t snapshot_size;
+    uint8_t *snapshot;
+
+    CHECK(cdrom_test_init_clears_setloc() == 0,
+          "controller reset clears pending Setloc state");
+    CHECK(cdrom_test_explicit_seek_ownership() == 0,
+          "SeekL/SeekP clears the old stream, pending INT1, ring, and BFRD");
+    CHECK(cdrom_test_implicit_seek_sequence() == 0,
+          "Setloc plus ReadN keeps SEEK until the first sector is eligible");
+
+    snapshot_size = cdrom_snapshot_bytes();
+    CHECK(snapshot_size == EXPECTED_CDROM_SNAPSHOT_BYTES,
+          "CD-ROM snapshot wire size remains backward-compatible");
+    snapshot = (uint8_t *)malloc(snapshot_size);
+    CHECK(snapshot != NULL, "snapshot allocation");
+    if (snapshot) {
+        cdrom_test_set_setloc_state(1, 1);
+        cdrom_snapshot_write(snapshot);
+        cdrom_test_set_setloc_state(0, 0);
+        CHECK(cdrom_snapshot_read(snapshot, snapshot_size) == 1,
+              "snapshot parser accepts the unchanged wire size");
+        CHECK(cdrom_test_get_setloc_state() == 3,
+              "snapshot restores far-seek and pending-Setloc bits");
+
+        /* Old snapshots stored only setloc_seek_far in this slot. The value
+         * 1 must still decode as far=true, pending=false. */
+        cdrom_test_set_setloc_state(1, 0);
+        cdrom_snapshot_write(snapshot);
+        cdrom_test_set_setloc_state(0, 1);
+        CHECK(cdrom_snapshot_read(snapshot, snapshot_size) == 1,
+              "snapshot parser accepts the old Setloc field value");
+        CHECK(cdrom_test_get_setloc_state() == 1,
+              "old Setloc value restores without a pending seek");
+        free(snapshot);
+    }
+
+    if (failures) {
+        fprintf(stderr, "FAILED (%d)\n", failures);
+        return 1;
+    }
+    puts("CD-ROM seek semantics: ALL PASS");
+    return 0;
+}