diff --git a/crates/gpu/src/backend.rs b/crates/gpu/src/backend.rs index 041f57737..3d71fd935 100644 --- a/crates/gpu/src/backend.rs +++ b/crates/gpu/src/backend.rs @@ -151,29 +151,71 @@ pub(crate) fn env_off() -> bool { } static MARKER: Mutex> = Mutex::new(None); +/// Scopes of driver work (device creation, shader and pipeline compilation) running now, so +/// that overlapping ones (the window-side warm-up and a first denoise) share one marker file. +static ACTIVE: Mutex = Mutex::new(0); -/// Write this file before the compute device is created and remove it once creation returned -/// (successfully or not). A file still there at the next launch means the process died inside -/// the driver: see [`take_init_marker`]. `None` (the default) writes nothing. +/// Write this file before the compute device is created or any shader / pipeline is compiled +/// (the kernels, the denoise network's), and remove it once that returned (successfully or not). +/// A file still there at the next launch means the process died inside the driver — e.g. Apple's +/// Metal compiler (`MTLCompilerService`) aborting on a kernel, issue #250: see +/// [`take_init_marker`]. `None` (the default) writes nothing. pub fn set_init_marker(path: Option) { *MARKER.lock().unwrap_or_else(|e| e.into_inner()) = path; } -/// Run device creation `f` between writing and removing the init marker (if one is set). -pub(crate) fn with_init_marker(backends: Backends, f: impl FnOnce() -> T) -> T { - let marker = MARKER.lock().unwrap_or_else(|e| e.into_inner()).clone(); - if let Some(m) = &marker { - if let Some(d) = m.parent() { +/// The marker's text: what started, and when. +fn marker_text(what: &str, secs: u64) -> String { + format!("GPU {what} started (unix time {secs})\n") +} + +/// Enter a driver-work scope on `marker`: the first one writes the file. +fn arm(marker: &std::path::Path, active: &Mutex, what: &str) { + let mut n = active.lock().unwrap_or_else(|e| e.into_inner()); + if *n == 0 { + if let Some(d) = marker.parent() { let _ = std::fs::create_dir_all(d); } let secs = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); - let _ = std::fs::write(m, format!("GPU device creation started (backends {backends:?}, unix time {secs})\n")); + let _ = std::fs::write(marker, marker_text(what, secs)); } - let r = f(); + *n += 1; +} + +/// Leave a scope: the last one removes the file. +fn disarm(marker: &std::path::Path, active: &Mutex) { + let mut n = active.lock().unwrap_or_else(|e| e.into_inner()); + *n = n.saturating_sub(1); + if *n == 0 { + let _ = std::fs::remove_file(marker); + } +} + +/// Removes the marker on the way out, a panic included (the process survived). +struct Armed(Option); + +impl Drop for Armed { + fn drop(&mut self) { + if let Some(m) = &self.0 { + disarm(m, &ACTIVE); + } + } +} + +/// Run driver work `f` (`what`: "device creation", "kernel compilation"…) between writing and +/// removing the init marker (if one is set). +pub(crate) fn with_marker(what: &str, f: impl FnOnce() -> T) -> T { + let marker = MARKER.lock().unwrap_or_else(|e| e.into_inner()).clone(); if let Some(m) = &marker { - let _ = std::fs::remove_file(m); + arm(m, &ACTIVE, what); } - r + let _armed = Armed(marker); + f() +} + +/// Run device creation (and the kernel build that follows it) `f` under the init marker. +pub(crate) fn with_init_marker(backends: Backends, f: impl FnOnce() -> T) -> T { + with_marker(&format!("device and kernel creation (backends {backends:?})"), f) } /// If the init marker `path` is present — the last process died while creating the GPU device — @@ -197,6 +239,9 @@ pub fn read_init_marker(path: &std::path::Path) -> Option { mod tests { use super::*; + /// The tests that set the process-wide marker take turns. + static GLOBAL: Mutex<()> = Mutex::new(()); + #[test] fn parses_backend_names() { assert_eq!(parse_backends("dx12"), Some(BackendChoice::Use(Backends::DX12))); @@ -263,6 +308,7 @@ mod tests { #[test] fn init_marker_is_written_during_creation_and_removed_after() { + let _turn = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); let dir = std::env::temp_dir().join(format!("lc-gpu-marker-{}", std::process::id())); let m = dir.join("gpu-init.marker"); let _ = std::fs::remove_dir_all(&dir); @@ -272,7 +318,7 @@ mod tests { assert!(seen, "the marker exists while the device is created"); assert!(!m.exists(), "and is removed afterwards"); // a marker left behind by a crashed process is reported once; reading it leaves it in place - std::fs::write(&m, "GPU device creation started (backends DX12)").unwrap(); + std::fs::write(&m, "GPU device and kernel creation started (backends DX12)").unwrap(); assert!(read_init_marker(&m).unwrap().contains("DX12")); assert!(m.exists(), "read_init_marker leaves the marker for the next launch"); assert!(take_init_marker(&m).unwrap().contains("DX12")); @@ -280,4 +326,41 @@ mod tests { assert_eq!(read_init_marker(&m), None); let _ = std::fs::remove_dir_all(&dir); } + + #[test] + fn overlapping_scopes_share_one_marker_until_the_last_ends() { + let dir = std::env::temp_dir().join(format!("lc-gpu-scopes-{}", std::process::id())); + let m = dir.join("gpu-init.marker"); + let _ = std::fs::remove_dir_all(&dir); + let active = Mutex::new(0); + arm(&m, &active, "device creation"); + assert!(read_init_marker(&m).unwrap().contains("device creation")); + // a second scope (a denoise network's kernels) keeps the first's text and the file + arm(&m, &active, "denoise kernel compilation"); + assert!(read_init_marker(&m).unwrap().contains("device creation")); + disarm(&m, &active); + assert!(m.exists(), "the first scope is still running"); + disarm(&m, &active); + assert!(!m.exists(), "the last scope removes it"); + // a stray extra disarm does not underflow or break the next scope + disarm(&m, &active); + arm(&m, &active, "kernel compilation"); + assert!(m.exists()); + disarm(&m, &active); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_panic_inside_the_scope_still_removes_the_marker() { + let _turn = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let dir = std::env::temp_dir().join(format!("lc-gpu-panic-{}", std::process::id())); + let m = dir.join("gpu-init.marker"); + let _ = std::fs::remove_dir_all(&dir); + set_init_marker(Some(m.clone())); + let r = std::panic::catch_unwind(|| with_marker("kernel compilation", || panic!("compile"))); + set_init_marker(None); + assert!(r.is_err()); + assert!(!m.exists(), "the process survived, so nothing is left to report"); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/gpu/src/ctx.rs b/crates/gpu/src/ctx.rs index 2f4abd4d2..c91d41b23 100644 --- a/crates/gpu/src/ctx.rs +++ b/crates/gpu/src/ctx.rs @@ -321,7 +321,8 @@ impl Gpu { log::error!("gpu: device lost ({reason:?}): {msg}"); crate::mark_broken(&format!("device lost ({reason:?}): {msg}")); }); - let scope = device.push_error_scope(wgpu::ErrorFilter::Validation); + // a kernel the driver's compiler refuses is a validation or an internal error: no GPU, not a half-built one + let scopes = [device.push_error_scope(wgpu::ErrorFilter::Validation), device.push_error_scope(wgpu::ErrorFilter::Internal)]; let consts = constants(); let mut kernels = HashMap::new(); for m in MODULES { @@ -347,7 +348,9 @@ impl Gpu { kernels.insert(*e, Kernel { pipeline, layout: layout.clone(), nbuf: m.bindings.len() }); } } - if let Some(e) = pollster::block_on(scope.pop()) { + let [validation, internal] = scopes; + let (internal, validation) = (pollster::block_on(internal.pop()), pollster::block_on(validation.pop())); + if let Some(e) = internal.or(validation) { log::error!("gpu: kernels failed to build on {}: {e}", info.name); return Err(format!("{}: kernels failed to build ({e})", info.name)); } diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 1eae995a4..a07d3e74f 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -226,9 +226,13 @@ pub(crate) fn device() -> Option<&'static ctx::Gpu> { let _work = exit::enter()?; GPU.get_or_init(|| { let Some(backends) = backend::compute_backends() else { return Err("disabled by LIGHTCRAFT_GPU_BACKEND=off".into()) }; - backend::with_init_marker(backends, || { + let r = backend::with_init_marker(backends, || { std::panic::catch_unwind(|| ctx::Gpu::new(backends)).unwrap_or_else(|_| Err("device creation panicked".into())) - }) + }); + if let Err(why) = &r { + log::warn!("gpu: rendering on the CPU: {why}"); + } + r }) .as_ref() .ok() diff --git a/crates/gpu/src/nn.rs b/crates/gpu/src/nn.rs index 689aecbec..13d93d3df 100644 --- a/crates/gpu/src/nn.rs +++ b/crates/gpu/src/nn.rs @@ -670,7 +670,8 @@ pub fn runner(net: &Net, tile: usize) -> Result { d.device.push_error_scope(wgpu::ErrorFilter::Validation), d.device.push_error_scope(wgpu::ErrorFilter::Internal), ]; - let pipes = build_pipelines(d, &layout); + // the driver compiles the kernels here: a crash in it is reported at the next launch (issue #250) + let pipes = crate::backend::with_marker("denoise kernel compilation", || build_pipelines(d, &layout)); let weights: Vec<(wgpu::Buffer, wgpu::Buffer)> = layers .iter() .map(|l| { diff --git a/docs/gpu-pipeline.md b/docs/gpu-pipeline.md index 8d7472352..fc53aae76 100644 --- a/docs/gpu-pipeline.md +++ b/docs/gpu-pipeline.md @@ -73,16 +73,20 @@ hosts, tests) waits for its running jobs the same way (`JobPool::shutdown`, `cra **Crash sentinel.** The desktop app writes `gpu-init.marker` into its settings folder (next to `ui.json`: `%APPDATA%\LightCraft`, `~/Library/Application Support/LightCraft`, -`~/.config/lightcraft`) just before the compute device is created and removes it as soon as creation -returns, successfully or not. If the marker is still there at the next launch, the process died inside +`~/.config/lightcraft`) just before the compute device is created and its kernels are compiled (and, on the first +AI denoise, before that network's kernels are) and removes it as soon as that returns, successfully or +not (issue #250: Apple's Metal compiler, `MTLCompilerService`, aborting on a kernel on an older Intel +Mac). A kernel the driver refuses without crashing is caught by wgpu error scopes: the log says why +(`gpu: rendering on the CPU: …`) and renders stay on the CPU. `LIGHTCRAFT_GPU=0` (or Settings ▸ +uncheck *Use the GPU for rendering*) forces the CPU path from the start. If the marker is still there at the next launch, the process died inside the driver: LightCraft starts with GPU rendering off (the preference is saved unchecked), removes the marker and says so in a notice. Checking *Use the GPU for rendering* again tries the GPU once more (and re-arms the sentinel). Not with `LIGHTCRAFT_NO_PREFS` (tests, scripts), and not in a `--memory` session, which writes nothing: it neither arms the sentinel nor removes a marker it finds (it still starts with GPU rendering off when one is there, and the next ordinary launch reports and clears it). Killing the app during the ~0.3 s of device creation, or two instances starting at the same moment, can trip it falsely — -harmless: rendering is then on the CPU until the box is checked again. The sentinel only covers the -compute device; a crash while the window's renderer starts is avoided by the backend defaults above +harmless: rendering is then on the CPU until the box is checked again. The sentinel covers the compute +device and every kernel build; a crash while the window's renderer starts is avoided by the backend defaults above or worked around with `LIGHTCRAFT_GPU_BACKEND`. **Troubleshooting a crash at startup (Windows).** Start LightCraft from a `.cmd` file or a terminal diff --git a/docs/parity.md b/docs/parity.md index d0bdc35c2..0dbbc8a8f 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -491,7 +491,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-PREF-ACCOUNT | Account | OOS | 🚫 | | | | LR-PREF-INTERFACE | Interface options | P1 | ✅ | `cmd:app.settings` | filmstrip names/badges, grid badges (auto/always/never), square-grid names, navigator, info overlay | | LR-PREF-DISPLAY | Monitor (display) colour management | P1 | 🟡 | `cmd:app.displayProfile`, `crates/codecs/src/display.rs`, `crates/engine/src/display.rs`, `crates/ui-egui/src/panels/settings.rs`, `docs/display-profiles.md` | Settings ▸ Display: a monitor ICC profile (matrix/TRC exactly, LUT-based through the CMS) chosen by hand; the loupe, Before, Compare, hover previews and the second window render into the display's primaries (its full gamut, CPU and GPU), thumbnails and stand-ins are converted from sRGB; histogram, preview caches and exports stay sRGB; soft proofing's display gamut warning uses the display's gamut. Missing: picking up the profile the OS assigns (X11 `_ICC_PROFILE`, colord, ColorSync, WCS), per-monitor profiles | -| LR-PREF-PERFORMANCE | GPU / performance | P1 | ✅ | `cmd:app.gpu`, `cmd:app.memoryBudget`, `cmd:app.settings` | GPU on/off, preview size (Automatic = the whole-frame loupe render at canvas size, up to 2560 px; or a 1600–5120 px size for it; zoomed views add a window at up to 100 %, issue #323), memory budget (a quarter of it is the idle per-view stage caches' share; the open photo's views are counted and reported, and cut only past four times that, the before side's and the zoom window's first — a Before/After at 1:1 on a 2800 px canvas holds about 1 GB of GPU stages, plus the one 288 MB copy of the original, which `ui.inspect` reports as `sharedSourceBytes`), thumbnail cache size in Settings | +| LR-PREF-PERFORMANCE | GPU / performance | P1 | ✅ | `cmd:app.gpu`, `cmd:app.memoryBudget`, `cmd:app.settings` | GPU on/off (a crash while the GPU device or its kernels are built, issue #250, turns it off for the next launch), preview size (Automatic = the whole-frame loupe render at canvas size, up to 2560 px; or a 1600–5120 px size for it; zoomed views add a window at up to 100 %, issue #323), memory budget (a quarter of it is the idle per-view stage caches' share; the open photo's views are counted and reported, and cut only past four times that, the before side's and the zoom window's first — a Before/After at 1:1 on a 2800 px canvas holds about 1 GB of GPU stages, plus the one 288 MB copy of the original, which `ui.inspect` reports as `sharedSourceBytes`), thumbnail cache size in Settings | | LR-PREF-PEOPLE | Face recognition | P2 | 🟡 | `cmd:faces.enable`, `cmd:faces.models.list`, `cmd:faces.models.inspect`, `cmd:faces.models.install`, `cmd:faces.models.download`, `cmd:faces.models.downloads`, `cmd:faces.models.downloadCancel`, `cmd:faces.models.remove`, `cmd:faces.models.select`, `cmd:dialog.faceModel`, `crates/faces/src/lib.rs`, `crates/ui-egui/src/panels/faces.rs` | Settings ▸ Faces: an off-by-default toggle and the face model list. Models are opt-in and never bundled, the 232 KB YuNet detector included. Press Download (terms first; then a pinned address, size and SHA-256 checked, fetched over pure-Rust https with `lightcraft-fetch`), or add any `.onnx` by picking or dropping it: it is recognised by hash or described from its shape, shows its licence terms and training-data note, runs a self-test, and installs only after an "I accept" box; an installed model is chosen and recognition switched on at once (the newest wins, each model keeps its own embedding cache). Non-commercial models are add-by-file only, never offered for download. Suggestions of names run on the shared pure-Rust CPU interpreter (`crates/faces/src/net.rs`, `crates/faces/src/runtime.rs`; bounded im2col/GEMM, checked ONNX shapes/operators; no external inference runtime; 40 public crops agree within 2.1e-6, one-thread SFace 34–36 ms / AuraFace 233–252 ms, see docs/faces.md) in a background worker and are never applied automatically. Thresholds are starting points calibrated on busts, not yet on live faces | LR-PREF-WATERMARK | Watermark settings | P1 | ✅ | `cmd:export.savePreset`, `crates/ui-egui/src/panels/dialogs.rs` | set in the Export dialog; kept with Export with Previous and in saved export presets | | LR-PREF-SHORTCUTS | Shortcut customisation | — | ✅ | `cmd:app.setShortcut`, `cmd:app.resetShortcuts`, `crates/ui-egui/src/panels/keymap.rs` | an extra (the reference app has no keymap editor): Help ▸ Keyboard Shortcuts (⌘/) lists every command; click a shortcut and press the new keys (Esc cancels), × removes, ↺ restores, Reset All; a key another command had moves over; saved in `ui.json`; native menu accelerators follow. Rating/label digits and secondary keys are listed read-only and yield to a key given to a command |