From ebe444870fa9a431abb332cf6f4c4da2c634b1d3 Mon Sep 17 00:00:00 2001 From: Simon Gellis Date: Mon, 10 Feb 2025 22:39:36 -0500 Subject: [PATCH] Sync memory between emulator thread and UI --- src/emulator.rs | 32 +++++++- src/memory.rs | 147 +++++++++++++++++++++++++++++------- src/window/vram/chardata.rs | 97 ------------------------ 3 files changed, 149 insertions(+), 127 deletions(-) diff --git a/src/emulator.rs b/src/emulator.rs index 96df37d..066e318 100644 --- a/src/emulator.rs +++ b/src/emulator.rs @@ -7,7 +7,7 @@ use std::{ sync::{ atomic::{AtomicBool, Ordering}, mpsc::{self, RecvError, TryRecvError}, - Arc, + Arc, Weak, }, }; @@ -17,7 +17,11 @@ use bytemuck::NoUninit; use egui_toast::{Toast, ToastKind, ToastOptions}; use tracing::{error, warn}; -use crate::{audio::Audio, graphics::TextureSink}; +use crate::{ + audio::Audio, + graphics::TextureSink, + memory::{MemoryRange, MemoryRegion}, +}; use shrooms_vb_core::{Sim, StopReason, EXPECTED_FRAME_SIZE}; pub use shrooms_vb_core::{VBKey, VBRegister, VBWatchpointType}; @@ -165,8 +169,10 @@ pub struct Emulator { renderers: HashMap, messages: HashMap>, debuggers: HashMap, + watched_regions: HashMap>, eye_contents: Vec, audio_samples: Vec, + buffer: Vec, } impl Emulator { @@ -189,8 +195,10 @@ impl Emulator { renderers: HashMap::new(), messages: HashMap::new(), debuggers: HashMap::new(), + watched_regions: HashMap::new(), eye_contents: vec![0u8; 384 * 224 * 2], audio_samples: Vec::with_capacity(EXPECTED_FRAME_SIZE), + buffer: vec![], }) } @@ -367,6 +375,10 @@ impl Emulator { } } + fn watch_memory(&mut self, range: MemoryRange, region: Weak) { + self.watched_regions.insert(range, region); + } + pub fn run(&mut self) { loop { let idle = self.tick(); @@ -391,6 +403,18 @@ impl Emulator { } } } + self.watched_regions.retain(|range, region| { + let Some(region) = region.upgrade() else { + return false; + }; + let Some(sim) = self.sims.get_mut(range.sim.to_index()) else { + return false; + }; + self.buffer.clear(); + sim.read_memory(range.start, range.length, &mut self.buffer); + region.update(&self.buffer); + true + }); } } @@ -557,6 +581,9 @@ impl Emulator { sim.write_memory(start, &buffer); let _ = done.send(buffer); } + EmulatorCommand::WatchMemory(range, region) => { + self.watch_memory(range, region); + } EmulatorCommand::AddBreakpoint(sim_id, address) => { let Some(sim) = self.sims.get_mut(sim_id.to_index()) else { return; @@ -647,6 +674,7 @@ pub enum EmulatorCommand { WriteRegister(SimId, VBRegister, u32), ReadMemory(SimId, u32, usize, Vec, oneshot::Sender>), WriteMemory(SimId, u32, Vec, oneshot::Sender>), + WatchMemory(MemoryRange, Weak), AddBreakpoint(SimId, u32), RemoveBreakpoint(SimId, u32), AddWatchpoint(SimId, u32, usize, VBWatchpointType), diff --git a/src/memory.rs b/src/memory.rs index dcafe7a..6c20f37 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -1,15 +1,18 @@ use std::{ collections::HashMap, - sync::{Arc, Mutex, MutexGuard}, + fmt::Debug, + sync::{atomic::AtomicU64, Arc, RwLock, RwLockReadGuard, TryLockError, Weak}, }; use bytemuck::BoxBytes; +use itertools::Itertools; +use tracing::warn; use crate::emulator::{EmulatorClient, EmulatorCommand, SimId}; pub struct MemoryMonitor { client: EmulatorClient, - regions: HashMap>>, + regions: HashMap>, } impl MemoryMonitor { @@ -21,20 +24,19 @@ impl MemoryMonitor { } pub fn view(&mut self, sim: SimId, start: u32, length: usize) -> MemoryView { - let region = MemoryRegion { sim, start, length }; - let memory = self.regions.entry(region).or_insert_with(|| { - let mut buf = aligned_memory(start, length); - let (tx, rx) = oneshot::channel(); - self.client - .send_command(EmulatorCommand::ReadMemory(sim, start, length, vec![], tx)); - let bytes = pollster::block_on(rx).unwrap(); - buf.copy_from_slice(&bytes); - #[expect(clippy::arc_with_non_send_sync)] // TODO: remove after bytemuck upgrade - Arc::new(Mutex::new(buf)) - }); - MemoryView { - memory: memory.clone(), - } + let range = MemoryRange { sim, start, length }; + let region = self + .regions + .get(&range) + .and_then(|r| r.upgrade()) + .unwrap_or_else(|| { + let region = Arc::new(MemoryRegion::new(start, length)); + self.regions.insert(range, Arc::downgrade(®ion)); + self.client + .send_command(EmulatorCommand::WatchMemory(range, Arc::downgrade(®ion))); + region + }); + MemoryView { region } } } @@ -52,21 +54,17 @@ fn aligned_memory(start: u32, length: usize) -> BoxBytes { } pub struct MemoryView { - memory: Arc>, + region: Arc, } -// SAFETY: BoxBytes is supposed to be Send+Sync, will be in a new version -unsafe impl Send for MemoryView {} impl MemoryView { pub fn borrow(&self) -> MemoryRef<'_> { - MemoryRef { - inner: self.memory.lock().unwrap(), - } + self.region.borrow() } } pub struct MemoryRef<'a> { - inner: MutexGuard<'a, BoxBytes>, + inner: RwLockReadGuard<'a, BoxBytes>, } impl MemoryRef<'_> { @@ -83,9 +81,102 @@ impl MemoryRef<'_> { } } -#[derive(Clone, Copy, PartialEq, Eq, Hash)] -struct MemoryRegion { - sim: SimId, - start: u32, - length: usize, +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub struct MemoryRange { + pub sim: SimId, + pub start: u32, + pub length: usize, +} + +const BUFFERS: usize = 4; +pub struct MemoryRegion { + gens: [AtomicU64; BUFFERS], + bufs: [RwLock; BUFFERS], +} +impl Debug for MemoryRegion { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("MemoryRegion") + .field("gens", &self.gens) + .finish_non_exhaustive() + } +} +// SAFETY: BoxBytes is meant to be Send+Sync, will be in a future version +unsafe impl Send for MemoryRegion {} +// SAFETY: BoxBytes is meant to be Send+Sync, will be in a future version +unsafe impl Sync for MemoryRegion {} + +impl MemoryRegion { + fn new(start: u32, length: usize) -> Self { + Self { + gens: std::array::from_fn(|i| AtomicU64::new(i as u64)), + bufs: std::array::from_fn(|_| RwLock::new(aligned_memory(start, length))), + } + } + + pub fn borrow(&self) -> MemoryRef<'_> { + /* + * When reading memory, a thread will grab the newest buffer (with the highest gen) + * It will only fail to grab the lock if the writer already has it, + * but the writer prioritizes older buffers (with lower gens). + * So this method will only block if the writer produces three full buffers + * in the time it takes the reader to do four atomic reads and grab a lock. + * In the unlikely event this happens... just try again. + */ + loop { + let newest_index = self + .gens + .iter() + .map(|i| i.load(std::sync::atomic::Ordering::Acquire)) + .enumerate() + .max_by_key(|(_, gen)| *gen) + .map(|(i, _)| i) + .unwrap(); + let inner = match self.bufs[newest_index].try_read() { + Ok(inner) => inner, + Err(TryLockError::Poisoned(e)) => e.into_inner(), + Err(TryLockError::WouldBlock) => { + continue; + } + }; + break MemoryRef { inner }; + } + } + + pub fn update(&self, data: &[u8]) { + let gens: Vec = self + .gens + .iter() + .map(|i| i.load(std::sync::atomic::Ordering::Acquire)) + .collect(); + let next_gen = gens.iter().max().unwrap() + 1; + let indices = gens + .into_iter() + .enumerate() + .sorted_by_key(|(_, val)| *val) + .map(|(i, _)| i); + for index in indices { + let mut lock = match self.bufs[index].try_write() { + Ok(inner) => inner, + Err(TryLockError::Poisoned(e)) => e.into_inner(), + Err(TryLockError::WouldBlock) => { + continue; + } + }; + lock.copy_from_slice(data); + self.gens[index].store(next_gen, std::sync::atomic::Ordering::Release); + return; + } + /* + * We have four buffers, and (at time of writing) only three threads interacting with memory: + * - The UI thread, reading small regions of memory + * - The "vram renderer" thread, reading large regions of memory + * - The emulation thread, writing memory every so often + * So it should be impossible for all four buffers to have a read lock at the same time, + * and (because readers always read the newest buffer) at least one of the oldest three + * buffers will be free the entire time we're in this method. + * TL;DR this should never happen. + * But if it does, do nothing. This isn't medical software, better to show stale data than crash. + */ + warn!("all buffers were locked by a reader at the same time") + } } diff --git a/src/window/vram/chardata.rs b/src/window/vram/chardata.rs index b317026..dc73b64 100644 --- a/src/window/vram/chardata.rs +++ b/src/window/vram/chardata.rs @@ -359,100 +359,3 @@ impl CharDataRenderer { utils::parse_palette(palette, brts) } } - -/* -struct CharDataLoader { - chardata: MemoryView, - brightness: MemoryView, - palettes: MemoryView, -} - -impl CharDataLoader { - pub fn new(sim_id: SimId, memory: &mut MemoryMonitor) -> Self { - Self { - chardata: memory.view(sim_id, 0x00078000, 0x8000), - brightness: memory.view(sim_id, 0x0005f824, 8), - palettes: memory.view(sim_id, 0x0005f860, 16), - } - } - - fn update_character(&self, image: &mut VramImage, palette: VramPalette, index: usize) { - if index >= 2048 { - return; - } - let palette = self.load_palette(palette); - let chardata = self.chardata.borrow(); - let character = chardata.range::(index * 8, 8); - for (row, pixels) in character.iter().enumerate() { - for col in 0..8 { - let char = (pixels >> (col * 2)) & 0x03; - image.write((col, row), palette[char as usize]); - } - } - } - - fn update_character_data(&self, image: &mut VramImage, palette: VramPalette) { - let palette = self.load_palette(palette); - let chardata = self.chardata.borrow(); - for (row, pixels) in chardata.range::(0, 8 * 2048).iter().enumerate() { - let char_index = row / 8; - let row_index = row % 8; - let x = (char_index % 16) * 8; - let y = (char_index / 16) * 8 + row_index; - for col in 0..8 { - let char = (pixels >> (col * 2)) & 0x03; - image.write((x + col, y), palette[char as usize]); - } - } - } - - fn load_palette(&self, palette: VramPalette) -> [u8; 4] { - let Some(offset) = palette.offset() else { - return utils::GENERIC_PALETTE; - }; - let palette = self.palettes.borrow().read(offset); - let brightnesses = self.brightness.borrow(); - let brts = brightnesses.range(0, 8); - utils::parse_palette(palette, brts) - } -} - -impl VramImageLoader for CharDataLoader { - type Resource = CharDataResource; - - fn id(&self) -> &str { - concat!(module_path!(), "::CharDataLoader") - } - - fn add(&self, resource: &Self::Resource) -> Option { - match resource { - CharDataResource::Character { palette, index } => { - let mut image = VramImage::new(8, 8); - self.update_character(&mut image, *palette, *index); - Some(image) - } - CharDataResource::CharacterData { palette } => { - let mut image = VramImage::new(8 * 16, 8 * 128); - self.update_character_data(&mut image, *palette); - Some(image) - } - } - } - - fn update<'a>( - &'a self, - resources: impl Iterator, - ) { - for (resource, image) in resources { - match resource { - CharDataResource::Character { palette, index } => { - self.update_character(image, *palette, *index) - } - CharDataResource::CharacterData { palette } => { - self.update_character_data(image, *palette) - } - } - } - } -} - */