gfx_display: stop display threads without waiting on the main thread

src/dev/gr2/gr2disp.rs

@@ -116,8 +116,6 @@ impl Gr2 {
             }
         }
 
-        if let Some(r) = self.renderer.lock().as_mut() {
-            r.stop();
-        }
+        self.display_stop.finish(&self.renderer);
     }
 }

src/dev/gr2/mod.rs

@@ -234,6 +234,7 @@ pub struct Gr2 {
 
     pub screen: Mutex<Rex3Screen>,
     pub renderer: Mutex<Option<Box<dyn Renderer>>>,
+    display_stop: crate::gfx_display::DisplayStop,
     pub screenshot_pending: AtomicBool,
     screenshot_counter: AtomicU32,
     retrace_cb: Mutex<Option<Arc<dyn Fn(bool) + Send + Sync>>>,
@@ -273,6 +274,7 @@ impl Gr2 {
             addr_of_mut!((*p).re3_thread).write(Mutex::new(None));
             addr_of_mut!((*p).screen).write(Mutex::new(Rex3Screen::new()));
             addr_of_mut!((*p).renderer).write(Mutex::new(None));
+            addr_of_mut!((*p).display_stop).write(Default::default());
             addr_of_mut!((*p).screenshot_pending).write(AtomicBool::new(false));
             addr_of_mut!((*p).screenshot_counter).write(AtomicU32::new(0));
             addr_of_mut!((*p).retrace_cb).write(Mutex::new(None));
@@ -989,6 +991,7 @@ impl Gr2 {
 
 impl crate::gfx_display::GfxDisplay for Gr2 {
     fn renderer_slot(&self) -> &Mutex<Option<Box<dyn Renderer>>> { &self.renderer }
+    fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop }
     fn screen(&self) -> &Mutex<Rex3Screen> { &self.screen }
     fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); }
     fn cycles(&self) -> CyclesPtr { self.cycles.get() }
@@ -1122,6 +1125,7 @@ impl Device for Gr2 {
     }
 
     fn stop(&self) {
+        self.display_stop.request();
         if !self.running.swap(false, Ordering::SeqCst) {
             return;
         }

src/dev/mgras/disp.rs

@@ -342,9 +342,7 @@ impl Mgras {
         }
         // The renderer's GL state belongs to this thread (its context is
         // current here), so it is torn down here and nowhere else.
-        if let Some(r) = self.renderer.lock().as_mut() {
-            r.stop();
-        }
+        self.display_stop.finish(&self.renderer);
     }
 
     /// Start the engines and the display refresh thread.
@@ -363,6 +361,7 @@ impl Mgras {
     }
 
     pub fn stop_display(&self) {
+        self.display_stop.request();
         self.running.store(false, Ordering::Release);
         if let Some(h) = self.refresh.lock().take() {
             let _ = h.join();

src/dev/mgras/mod.rs

@@ -367,6 +367,7 @@ pub struct Mgras {
     running: AtomicBool,
     refresh: SpinLock<Option<thread::JoinHandle<()>>>,
     renderer: Mutex<Option<Box<dyn Renderer>>>,
+    display_stop: crate::gfx_display::DisplayStop,
     /// The finished frame, handed to the renderer as a prebuilt picture
     /// (`Rex3Screen::prebuilt`), as GR2 does.
     screen: Mutex<crate::disp::Rex3Screen>,
@@ -537,6 +538,7 @@ impl Mgras {
             addr_of_mut!((*p).dirty).write(AtomicBool::new(true));
             addr_of_mut!((*p).refresh).write(SpinLock::new(None));
             addr_of_mut!((*p).renderer).write(Mutex::new(None));
+            addr_of_mut!((*p).display_stop).write(Default::default());
             addr_of_mut!((*p).screen).write(Mutex::new(crate::disp::Rex3Screen::new()));
             addr_of_mut!((*p).heartbeat).write(heartbeat);
             addr_of_mut!((*p).fasttick).write(fasttick);
@@ -1598,6 +1600,7 @@ impl Mgras {
 
 impl crate::gfx_display::GfxDisplay for Mgras {
     fn renderer_slot(&self) -> &Mutex<Option<Box<dyn Renderer>>> { &self.renderer }
+    fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop }
     fn screen(&self) -> &Mutex<crate::disp::Rex3Screen> { &self.screen }
     fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); }
     fn cycles(&self) -> crate::cpu::mips_core::CyclesPtr { *self.cycles.lock() }

src/dev/ng1/rex3.rs

@@ -1388,6 +1388,7 @@ pub struct Rex3 {
     pub diag: AtomicU64,
     debug_state: Mutex<DebugState>,
     pub renderer: Mutex<Option<Box<dyn Renderer>>>,
+    display_stop: crate::gfx_display::DisplayStop,
     /// Compiled shaders, keyed by `(dm0, dm1_normalized, clipmode_key)`.
     ///
     /// Always present, not gated on `rex-jit`: the LLVM-compiled shaders in
@@ -1582,6 +1583,7 @@ impl Rex3 {
             debug_state: Mutex::new(DebugState::default()),
             diag: AtomicU64::new(0),
             renderer: Mutex::new(None),
+            display_stop: Default::default(),
             // IRIS_NO_JIT (set by the sandboxed Mac App Store GUI build) forces
             // the interpreter: Cranelift's mmap+mprotect executable pages aren't
             // MAP_JIT, so the App Sandbox kills the process with SIGKILL/
@@ -3241,9 +3243,7 @@ impl Rex3 {
             }
         }
 
-        if let Some(renderer) = self.renderer.lock().as_mut() {
-            renderer.stop();
-        }
+        self.display_stop.finish(&self.renderer);
     }
 
     /// Returns `true` if the register offset was recognized and updated state.
@@ -3623,6 +3623,7 @@ impl Device for Rex3 {
         if let Some(s) = self.stop_trace() {
             eprintln!("REX3: trace closed: {s}");
         }
+        self.display_stop.request();
         self.running.store(false, Ordering::SeqCst);
 
         // Send exit command to ensure processor thread wakes up and terminates.
@@ -4920,6 +4921,7 @@ impl BusDevice for Rex3 {
 
 impl crate::gfx_display::GfxDisplay for Rex3 {
     fn renderer_slot(&self) -> &Mutex<Option<Box<dyn Renderer>>> { &self.renderer }
+    fn display_stop(&self) -> &crate::gfx_display::DisplayStop { &self.display_stop }
     fn screen(&self) -> &Mutex<Rex3Screen> { &self.screen }
     fn request_screenshot(&self) { self.screenshot_pending.store(true, Ordering::Relaxed); }
     fn screenshot_pending(&self) -> bool { self.screenshot_pending.load(Ordering::Relaxed) }

src/display_stop_tests.rs

@@ -0,0 +1,201 @@
+//! Stopping a machine from the main thread must not wait on its renderer.
+//!
+//! On macOS a renderer's `stop` needs the main thread, which is the thread
+//! stopping the machine (see `gfx_display::DisplayStop`). The renderer here
+//! does the same: its `stop` asks for an answer from the test thread. One
+//! machine per display family, stopped from a reset and then from main:
+//! Newport (REX3), GR2, IMPACT (MGRAS) and the O2.
+
+use std::path::{Path, PathBuf};
+use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering::SeqCst};
+use std::sync::{mpsc, Arc};
+use std::time::{Duration, Instant};
+
+use crate::config::{CpuModel, GraphicsBoard, MachineConfig, MachineProfile};
+use crate::dev::ng1::rex3::Renderer;
+use crate::workstation::Workstation;
+
+/// A stop that waits on the main thread never returns.
+const BOUND: Duration = Duration::from_secs(8);
+
+/// A renderer that counts presents and runs `on_stop` when torn down.
+#[derive(Clone)]
+pub(crate) struct Probe {
+    presents: Arc<AtomicUsize>,
+    pub(crate) stopped: Arc<AtomicBool>,
+    on_stop: Arc<dyn Fn() + Send + Sync>,
+}
+
+impl Probe {
+    pub(crate) fn new(on_stop: impl Fn() + Send + Sync + 'static) -> Self {
+        Self { presents: Arc::default(), stopped: Arc::default(), on_stop: Arc::new(on_stop) }
+    }
+
+    /// A probe that notes whether `disks_running` was true when torn down.
+    pub(crate) fn watching(disks_running: impl Fn() -> bool + Send + Sync + 'static) -> (Self, Arc<AtomicBool>) {
+        let seen = Arc::new(AtomicBool::default());
+        let flag = seen.clone();
+        (Self::new(move || if disks_running() { flag.store(true, SeqCst) }), seen)
+    }
+}
+
+impl Renderer for Probe {
+    fn present(
+        &mut self, _: &mut crate::disp::Rex3Screen, _: &mut crate::debug_overlay::DebugOverlay,
+        _: &mut crate::disp::StatusBar, _: &mut crate::disp::StatusBarTexture, _: &crate::disp::BarStats,
+        _: bool, _: Option<&[u32]>, _: Option<&[u32]>,
+    ) {
+        self.presents.fetch_add(1, SeqCst);
+    }
+
+    fn stop(&mut self) {
+        (self.on_stop)();
+        self.stopped.store(true, SeqCst);
+    }
+}
+
+/// A scratch directory for one test's files (the Octane and O2 read a PROM
+/// from a file; these tests run no guest code, so zeros will do). Tests run
+/// side by side in one process and each removes its own directory, so each
+/// takes a name of its own.
+pub(crate) fn scratch(test: &str) -> PathBuf {
+    let dir = std::env::temp_dir().join(format!("iris-display-stop-{test}-{}", std::process::id()));
+    std::fs::create_dir_all(&dir).unwrap();
+    // A REX3 saves the shapes it drew when it stops, to $HOME/.iris unless
+    // told otherwise. The path is the same for every test, as in
+    // `exit_persistence_tests::serial`.
+    std::env::set_var("IRIS_REX_JIT_PROFILE", std::env::temp_dir().join("iris-display-stop-rex-jit-profile.bin"));
+    dir
+}
+
+pub(crate) fn config(dir: &Path, profile: MachineProfile, board: GraphicsBoard) -> MachineConfig {
+    let (mb, cpu) = match profile {
+        MachineProfile::OctaneIp30 => (64, CpuModel::R10000),
+        MachineProfile::O2Ip32 => (32, CpuModel::R5000),
+        _ => (16, CpuModel::default()),
+    };
+    let mut cfg = crate::bench_runner::bench_config([mb, 0, 0, 0], cpu);
+    cfg.headless = false;
+    cfg.machine.profile = profile;
+    cfg.monitor_port = Some(0);
+    cfg.nvram = dir.join("nvram").to_string_lossy().into_owned();
+    cfg.nveeprom = dir.join("nveeprom").to_string_lossy().into_owned();
+    if matches!(profile, MachineProfile::OctaneIp30 | MachineProfile::O2Ip32) {
+        let prom = dir.join("prom.bin");
+        std::fs::write(&prom, vec![0u8; 1 << 20]).unwrap();
+        cfg.prom = prom.to_string_lossy().into_owned();
+    }
+    // The O2's GBE is its own display; it has no `[graphics] board`.
+    cfg.graphics.board = (profile != MachineProfile::O2Ip32).then_some(board);
+    if board == GraphicsBoard::Newport {
+        // A Newport shows nothing until a video mode is programmed. Set it
+        // here rather than wait for a guest to get that far.
+        cfg.graphics.resolution = crate::dev::ng1::vc2_timings::NewportResolution::Res1280x1024;
+    }
+    cfg
+}
+
+pub(crate) fn big_stack<T: Send + 'static>(f: impl FnOnce() -> T + Send + 'static) -> T {
+    std::thread::Builder::new().stack_size(64 * 1024 * 1024).spawn(f).unwrap().join().unwrap()
+}
+
+/// Stop `machine` on a thread called `name`, and drop it there too if `last`
+/// (the O2 stops its display in its drop). The machine comes back unless it
+/// was dropped, or `None` if that did not finish within [`BOUND`].
+fn stop_as(name: &str, mut machine: Box<dyn Workstation>, last: bool) -> Option<Option<Box<dyn Workstation>>> {
+    let (done, finished) = mpsc::channel();
+    std::thread::Builder::new().name(name.into()).spawn(move || {
+        machine.stop();
+        if last {
+            drop(machine);
+            let _ = done.send(None);
+        } else {
+            let _ = done.send(Some(machine));
+        }
+    }).unwrap();
+    finished.recv_timeout(BOUND).ok()
+}
+
+#[test]
+fn stopping_from_the_main_thread_never_waits_on_the_renderer() {
+    let dir = scratch("main-thread");
+    let families = [
+        (MachineProfile::IndyIp24, GraphicsBoard::Newport),
+        (MachineProfile::Indigo2Ip22, GraphicsBoard::Xz),
+        (MachineProfile::OctaneIp30, GraphicsBoard::SolidImpact),
+        (MachineProfile::O2Ip32, GraphicsBoard::Newport),
+    ];
+    let mut failures = Vec::new();
+    for (profile, board) in families {
+        // Stopped from the main thread, the renderer's stop waits for an
+        // answer that never comes. Stopped from another thread (a reset) the
+        // main thread answers, and the renderer must still be released.
+        let answered = Arc::new(AtomicBool::new(true));
+        let gate = answered.clone();
+        let probe = Probe::new(move || while !gate.load(SeqCst) { std::thread::sleep(Duration::from_millis(5)) });
+        // The reset comes first and the machine starts again for the stop
+        // from main. The O2's display thread ends only in its drop, so it
+        // needs a machine for each.
+        let reuse = profile != MachineProfile::O2Ip32;
+        let mut machine: Option<Box<dyn Workstation>> = None;
+        for (name, from_main) in [("reset", false), ("main", true)] {
+            let label = format!("{profile:?} {board:?} stopped from {name}");
+            let before = probe.presents.load(SeqCst);
+            let m = match machine.take() {
+                Some(mut m) => {
+                    m.restart_peripherals();
+                    m
+                }
+                None => {
+                    let cfg = config(&dir, profile, board);
+                    let mut m = big_stack(move || crate::workstation::build(cfg).expect("machine"));
+                    *m.get_display().expect("a display").renderer_slot().lock() = Some(Box::new(probe.clone()));
+                    m.start();
+                    m
+                }
+            };
+            let up = Instant::now() + Duration::from_secs(10);
+            while probe.presents.load(SeqCst) == before && Instant::now() < up {
+                std::thread::sleep(Duration::from_millis(10));
+            }
+            if probe.presents.load(SeqCst) == before {
+                failures.push(format!("{label}: the display never presented"));
+                break;
+            }
+            probe.stopped.store(false, SeqCst);
+            answered.store(!from_main, SeqCst);
+            match stop_as(name, m, from_main || !reuse) {
+                None => failures.push(format!("{label}: stop did not return within {BOUND:?}")),
+                Some(back) => {
+                    machine = back;
+                    if !from_main && !probe.stopped.load(SeqCst) {
+                        failures.push(format!("{label}: the renderer was not released"));
+                    }
+                }
+            }
+            answered.store(true, SeqCst); // let a hung stop finish
+        }
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+    assert!(failures.is_empty(), "\n{}", failures.join("\n"));
+}
+
+/// The graphics board can hang in its stop, so the SCSI controllers and
+/// IOC3 must already be stopped when its renderer is torn down.
+#[test]
+fn the_octane_stops_its_disks_before_its_graphics() {
+    use crate::traits::Device;
+    let dir = scratch("octane-order");
+    let cfg = config(&dir, MachineProfile::OctaneIp30, GraphicsBoard::SolidImpact);
+    let mut machine = big_stack(move || crate::ip30_machine::OctaneMachine::new(&cfg).expect("Octane"));
+    let (scsi, ioc3) = (machine.scsi.clone(), machine.ioc3.clone());
+    let (probe, overlap) = Probe::watching(move || ioc3.is_running() || scsi.iter().any(|s| s.is_running()));
+    *machine.gfx.as_ref().expect("graphics").display().renderer_slot().lock() = Some(Box::new(probe.clone()));
+    machine.restart_peripherals();
+    assert!(!machine.scsi.is_empty() && machine.scsi.iter().all(|s| s.is_running()), "the SCSI controllers are running");
+    assert!(machine.ioc3.is_running(), "IOC3 is running");
+    machine.stop();
+    assert!(probe.stopped.load(SeqCst), "the display thread released its renderer");
+    assert!(!overlap.load(SeqCst), "a disk controller was still running when the graphics stopped");
+    let _ = std::fs::remove_dir_all(&dir);
+}

src/gfx_display.rs

@@ -6,13 +6,18 @@
 //! last-presented screen (screenshots, VNC), the hardware cursor, and the CPU
 //! cycle counter.
 
+use std::sync::atomic::{AtomicBool, Ordering};
+
 use parking_lot::Mutex;
 
 use crate::disp::Rex3Screen;
 use crate::cpu::mips_core::CyclesPtr;
 use crate::dev::ng1::rex3::Renderer;
 
 pub trait GfxDisplay: Send + Sync {
+    /// The board's [`DisplayStop`], which its stop and its display thread use.
+    fn display_stop(&self) -> &DisplayStop;
+
     /// Slot the front-end installs its renderer into. The board's display
     /// thread presents every frame through whatever is installed here.
     fn renderer_slot(&self) -> &Mutex<Option<Box<dyn Renderer>>>;
@@ -53,6 +58,37 @@ pub trait GfxDisplay: Send + Sync {
     fn hw_cursor(&self) -> Option<HwCursor> { None }
 }
 
+/// Whether the display thread tears its renderer down when it ends.
+///
+/// Releasing a renderer's GL context has to happen on the thread that made
+/// it current, so the display thread does it as its last act. On macOS
+/// glutin runs that release on the main thread, and the main thread is the
+/// one stopping the machine when the window closes, waiting in the join.
+/// Neither goes on, and the disks are never flushed. So a board calls
+/// [`request`](Self::request) first thing in its stop, and its display
+/// thread ends with [`finish`](Self::finish), which leaves the renderer
+/// alone when the stop came from the main thread. That stop is final: the
+/// process is ending, and the renderer cannot be set up again.
+#[derive(Default)]
+pub struct DisplayStop(AtomicBool);
+
+impl DisplayStop {
+    pub fn request(&self) {
+        // std has no is-main-thread test. The platform ones exist where the
+        // hang does (pthread_main_np on macOS), but Rust names the main
+        // thread, and the name lets a test on Linux stand in for it.
+        self.0.store(std::thread::current().name() == Some("main"), Ordering::Release);
+    }
+
+    pub fn finish(&self, renderer: &Mutex<Option<Box<dyn Renderer>>>) {
+        if !self.0.load(Ordering::Acquire) {
+            if let Some(r) = renderer.lock().as_mut() {
+                r.stop();
+            }
+        }
+    }
+}
+
 /// The hardware cursor: where the board draws its glyph and which of the
 /// glyph's pixels are opaque. The glyph's top-left is the pointer position
 /// minus the hotspot, which the hardware does not know.