diff --git a/apps/pdfcraft/src/main.rs b/apps/pdfcraft/src/main.rs index 3d7f61a5..de88d640 100644 --- a/apps/pdfcraft/src/main.rs +++ b/apps/pdfcraft/src/main.rs @@ -297,7 +297,9 @@ fn write_control_file(path: &str, port: u16, token: &str) -> std::io::Result<()> /// It also saves battery. Machines with one GPU are unaffected. /// - On Linux, draw on a GPU that a monitor is plugged into. On a desktop whose monitors all hang /// off the discrete GPU, drawing on the integrated one leaves the window black under Wayland -/// compositors on NVIDIA. Among the GPUs that drive a display, the integrated one still wins. +/// compositors on NVIDIA. When several GPUs drive a display, the one driving a built-in panel +/// wins (a hybrid laptop, issue #8); without a built-in panel (a desktop with a monitor on each +/// GPU, issue #445) the discrete one wins, as the integrated one may fail to present there. /// - On Windows, use Direct3D 12, falling back to OpenGL, and never load Vulkan drivers unless /// `WGPU_BACKEND` asks for them. Creating a Vulkan instance loads every installed Vulkan driver /// into the process, and a faulty one (an Intel driver in issue #37) crashed PdfCraft before @@ -333,52 +335,78 @@ fn configure_gpu(native: &mut eframe::NativeOptions) { } } -/// PCI `(vendor, device)` ids of the GPUs with a connected monitor, read from the DRM connectors -/// under `drm` (`card1-DP-3/status` is `connected`, `card1/device/{vendor,device}` hold `0x10de`). +/// A GPU with a connected monitor. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +struct DisplayGpu { + /// PCI `(vendor, device)` ids. + pci: (u32, u32), + /// Whether one of its connected connectors is a built-in panel (`eDP`, `LVDS` or `DSI`). + internal_panel: bool, +} + +/// Whether a DRM connector name (`eDP-1`, `LVDS-1`, `DSI-1`) is a laptop's built-in panel. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn is_internal_panel(connector: &str) -> bool { + ["eDP", "LVDS", "DSI"].iter().any(|kind| connector.starts_with(kind)) +} + +/// The GPUs with a connected monitor, read from the DRM connectors under `drm` (`card1-DP-3/status` +/// is `connected`, `card1/device/{vendor,device}` hold `0x10de`). #[cfg(target_os = "linux")] -fn linux_display_gpus(drm: &std::path::Path) -> Vec<(u32, u32)> { +fn linux_display_gpus(drm: &std::path::Path) -> Vec { let read_hex = |p: std::path::PathBuf| -> Option { let s = std::fs::read_to_string(p).ok()?; u32::from_str_radix(s.trim().trim_start_matches("0x"), 16).ok() }; - let mut gpus = Vec::new(); + let mut gpus: Vec = Vec::new(); let Ok(entries) = std::fs::read_dir(drm) else { return gpus }; // A machine has a handful of connectors; the cap only bounds a pathological sysfs. for entry in entries.flatten().take(256) { let name = entry.file_name(); - let Some((card, _connector)) = name.to_str().and_then(|n| n.split_once('-')) else { continue }; + let Some((card, connector)) = name.to_str().and_then(|n| n.split_once('-')) else { continue }; let connected = std::fs::read_to_string(entry.path().join("status")).is_ok_and(|s| s.trim() == "connected"); if !connected { continue; } let device = drm.join(card).join("device"); - if let (Some(v), Some(d)) = (read_hex(device.join("vendor")), read_hex(device.join("device"))) - && !gpus.contains(&(v, d)) - { - gpus.push((v, d)); + let (Some(v), Some(d)) = (read_hex(device.join("vendor")), read_hex(device.join("device"))) else { continue }; + let internal_panel = is_internal_panel(connector); + match gpus.iter_mut().find(|g| g.pci == (v, d)) { + Some(gpu) => gpu.internal_panel |= internal_panel, + None => gpus.push(DisplayGpu { pci: (v, d), internal_panel }), } } gpus } -/// Index of the adapter to draw with: one that drives a display (by PCI ids) first, then the most -/// frugal kind — integrated, discrete, other, virtual, software. +/// Index of the adapter to draw with: one that drives a display (by PCI ids) first. Among several +/// of those, the one driving a built-in panel (a hybrid laptop, #8), or else, with no built-in +/// panel anywhere (a desktop with a monitor on each GPU, #445), the discrete one. Otherwise the +/// most frugal kind: integrated, discrete, other, virtual, software. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] -fn pick_adapter(adapters: &[(u32, u32, eframe::wgpu::DeviceType)], displays: &[(u32, u32)]) -> Option { +fn pick_adapter(adapters: &[(u32, u32, eframe::wgpu::DeviceType)], displays: &[DisplayGpu]) -> Option { use eframe::wgpu::DeviceType; + let display = |pci: (u32, u32)| displays.iter().find(|g| g.pci == pci); + let multi_gpu_desktop = displays.len() > 1 && !displays.iter().any(|g| g.internal_panel); adapters .iter() .enumerate() .min_by_key(|(_, (vendor, device, kind))| { - let drives_display = displays.contains(&(*vendor, *device)); - let frugality = match kind { - DeviceType::IntegratedGpu => 0, - DeviceType::DiscreteGpu => 1, - DeviceType::Other => 2, - DeviceType::VirtualGpu => 3, - DeviceType::Cpu => 4, + let shown = display((*vendor, *device)); + let drives_display = shown.is_some(); + let drives_panel = shown.is_some_and(|g| g.internal_panel); + let rank = match kind { + // On a desktop with monitors on both, the integrated GPU can accept the window and + // still fail to present to it (#445). + DeviceType::DiscreteGpu if drives_display && multi_gpu_desktop => 0, + DeviceType::IntegratedGpu => 1, + DeviceType::DiscreteGpu => 2, + DeviceType::Other => 3, + DeviceType::VirtualGpu => 4, + DeviceType::Cpu => 5, }; - (!drives_display, frugality) + (!drives_display, !drives_panel, rank) }) .map(|(i, _)| i) } @@ -431,29 +459,65 @@ mod tests { vec![(NVIDIA.0, NVIDIA.1, DeviceType::DiscreteGpu), (AMD_IGPU.0, AMD_IGPU.1, DeviceType::IntegratedGpu), (0, 0, DeviceType::Cpu)] } + const INTEL_IGPU: (u32, u32) = (0x8086, 0xa780); + + fn monitor(pci: (u32, u32)) -> super::DisplayGpu { + super::DisplayGpu { pci, internal_panel: false } + } + + fn panel(pci: (u32, u32)) -> super::DisplayGpu { + super::DisplayGpu { pci, internal_panel: true } + } + #[test] fn pick_adapter_prefers_the_gpu_driving_the_monitors() { // A desktop whose monitors are all on the discrete GPU: the integrated one shows black. - assert_eq!(super::pick_adapter(&adapters(), &[NVIDIA]), Some(0)); + assert_eq!(super::pick_adapter(&adapters(), &[monitor(NVIDIA)]), Some(0)); } #[test] fn pick_adapter_keeps_the_integrated_gpu_on_hybrid_laptops() { // Issue #8: the panel is on the integrated GPU, an external monitor on the discrete one. - assert_eq!(super::pick_adapter(&adapters(), &[NVIDIA, AMD_IGPU]), Some(1)); - assert_eq!(super::pick_adapter(&adapters(), &[AMD_IGPU]), Some(1)); + assert_eq!(super::pick_adapter(&adapters(), &[monitor(NVIDIA), panel(AMD_IGPU)]), Some(1)); + assert_eq!(super::pick_adapter(&adapters(), &[panel(AMD_IGPU), monitor(NVIDIA)]), Some(1)); + assert_eq!(super::pick_adapter(&adapters(), &[panel(AMD_IGPU)]), Some(1)); + assert_eq!(super::pick_adapter(&adapters(), &[monitor(AMD_IGPU)]), Some(1)); + } + + #[test] + fn pick_adapter_prefers_the_discrete_gpu_on_desktops_with_a_monitor_on_each() { + // Issue #445: one monitor on the Intel iGPU, one on the NVIDIA card, no built-in panel. + use eframe::wgpu::DeviceType; + let desktop = + [(INTEL_IGPU.0, INTEL_IGPU.1, DeviceType::IntegratedGpu), (NVIDIA.0, NVIDIA.1, DeviceType::DiscreteGpu), (0, 0, DeviceType::Cpu)]; + assert_eq!(super::pick_adapter(&desktop, &[monitor(INTEL_IGPU), monitor(NVIDIA)]), Some(1)); + assert_eq!(super::pick_adapter(&desktop, &[monitor(NVIDIA), monitor(INTEL_IGPU)]), Some(1)); + // A GPU driving no display still loses to one that does. + assert_eq!(super::pick_adapter(&adapters(), &[monitor(AMD_IGPU), monitor(INTEL_IGPU)]), Some(1)); } #[test] fn pick_adapter_falls_back_to_low_power_without_a_match() { - assert_eq!(super::pick_adapter(&adapters(), &[(0x8086, 0x1234)]), Some(1)); - assert_eq!(super::pick_adapter(&[], &[NVIDIA]), None); + assert_eq!(super::pick_adapter(&adapters(), &[monitor((0x8086, 0x1234))]), Some(1)); + assert_eq!(super::pick_adapter(&adapters(), &[monitor((0x8086, 0x1234)), monitor((0x8086, 0x5678))]), Some(1)); + assert_eq!(super::pick_adapter(&[], &[monitor(NVIDIA)]), None); + } + + #[test] + fn internal_panels_are_edp_lvds_and_dsi_connectors() { + for name in ["eDP-1", "LVDS-1", "DSI-1"] { + assert!(super::is_internal_panel(name), "{name}"); + } + for name in ["DP-3", "HDMI-A-1", "DVI-D-1", "VGA-1", "Writeback-1", ""] { + assert!(!super::is_internal_panel(name), "{name}"); + } } #[cfg(target_os = "linux")] #[test] fn linux_display_gpus_reads_connected_connectors() -> std::io::Result<()> { let dir = std::env::temp_dir().join(format!("pdfcraft-drm-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); let card = |name: &str, (vendor, device): (u32, u32)| -> std::io::Result<()> { std::fs::create_dir_all(dir.join(name).join("device"))?; std::fs::write(dir.join(name).join("device/vendor"), format!("{vendor:#06x}\n"))?; @@ -469,9 +533,19 @@ mod tests { connector("card1-DP-4", "connected")?; connector("card2-HDMI-A-1", "disconnected")?; connector("card2-Writeback-1", "unknown")?; - let gpus = super::linux_display_gpus(&dir); + let desktop = super::linux_display_gpus(&dir); + // Issue #445: a monitor on the integrated GPU too, still no built-in panel. + connector("card2-HDMI-A-1", "connected")?; + let mut both = super::linux_display_gpus(&dir); + // A hybrid laptop: the integrated GPU drives the built-in panel as well. + connector("card2-eDP-1", "connected")?; + let mut laptop = super::linux_display_gpus(&dir); std::fs::remove_dir_all(&dir)?; - assert_eq!(gpus, vec![NVIDIA]); + assert_eq!(desktop, vec![monitor(NVIDIA)]); + both.sort_by_key(|g| g.pci); + assert_eq!(both, vec![monitor(AMD_IGPU), monitor(NVIDIA)]); + laptop.sort_by_key(|g| g.pci); + assert_eq!(laptop, vec![panel(AMD_IGPU), monitor(NVIDIA)]); assert!(super::linux_display_gpus(std::path::Path::new("/nonexistent/drm")).is_empty()); Ok(()) }