From 0589f5685bbe1582fa84d9695f0dec129dbcbaf8 Mon Sep 17 00:00:00 2001 From: Samuel Onoja Date: Wed, 14 Oct 2026 13:28:23 +0100 Subject: [PATCH] fix(outputs): complete CRTC matching before live validation (#45) Co-authored-by: Samuel Onoja --- compositor/src/backends/direct.rs | 241 ++++++++++++++++- .../src/backends/direct/crtc_assignment.rs | 242 +++++++++++++++--- docs/configuration/outputs.md | 7 + 3 files changed, 451 insertions(+), 39 deletions(-) diff --git a/compositor/src/backends/direct.rs b/compositor/src/backends/direct.rs index 1b718f5..8c285e1 100644 --- a/compositor/src/backends/direct.rs +++ b/compositor/src/backends/direct.rs @@ -773,12 +773,7 @@ pub(crate) fn validate_live_outputs(state: &Ferese, profiles: &[OutputProfile]) &backend.manual_outputs, ); resolve_output_positions(backend, &mut desired)?; - let mut usable = 0; - for device in backend.devices.values() { - let scan = select_outputs(device, &backend.monitors, &desired, backend.low_power).map_err(|e| e.to_string())?; - transaction::validate_device(device, &scan).map_err(|error| error.to_string())?; - usable += scan.selections.len(); - } + let usable = validate_output_configuration(backend, &desired)?; if usable == 0 { return Err("display configuration would leave no usable output".into()); } @@ -1564,10 +1559,59 @@ fn remember_applied_configuration(state: &mut Ferese) { fn validate_desired_outputs(state: &Ferese) -> Result<(), String> { let backend = state.direct_backend.as_ref().ok_or("direct backend unavailable")?; - for device in backend.devices.values() { - let scan = select_outputs(device, &backend.monitors, &backend.desired_outputs, backend.low_power) - .map_err(|error| error.to_string())?; - transaction::validate_device(device, &scan).map_err(|error| error.to_string())?; + validate_output_configuration(backend, &backend.desired_outputs).map(|_| ()) +} + +fn validate_output_configuration( + backend: &DirectBackendState, + desired: &DesiredOutputConfiguration, +) -> Result { + let scans = backend + .devices + .values() + .map(|device| { + select_outputs(device, &backend.monitors, desired, backend.low_power) + .map(|scan| (device, scan)) + .map_err(|error| error.to_string()) + }) + .collect::, _>>()?; + let selected = scans + .iter() + .flat_map(|(device, scan)| { + let device_key = drm_device_key(&device.drm); + scan.selections + .iter() + .map(move |selection| format!("{device_key}/{}", selection.connector)) + }) + .collect::>(); + validate_output_plan(desired, &selected, || { + for (device, scan) in &scans { + transaction::validate_device(device, scan).map_err(|error| error.to_string())?; + } + Ok(()) + })?; + Ok(selected.len()) +} + +/// Keep plan completeness separate from the hardware probe so a partial scan +/// cannot be accepted merely because the driver accepts its selected subset. +fn validate_output_plan( + desired: &DesiredOutputConfiguration, + selected: &[String], + test_hardware: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + validate_selected_outputs(&desired.outputs, selected)?; + test_hardware() +} + +fn validate_selected_outputs<'a>( + outputs: impl IntoIterator, + selected: &[String], +) -> Result<(), String> { + for output in outputs.into_iter().filter(|output| output.settings.enabled) { + if !selected.contains(&output.key) { + return Err(format!("enabled output {} has no usable CRTC assignment", output.key)); + } } Ok(()) } @@ -2064,6 +2108,19 @@ fn select_outputs( } } + let prefix = format!("{device_key}/"); + let selected = selections + .iter() + .map(|selection| format!("{prefix}{}", selection.connector)) + .collect::>(); + // Also guard the rescan immediately before applying a device transaction: + // hotplug can invalidate the earlier configuration-wide validation. + validate_selected_outputs( + desired.outputs.iter().filter(|output| output.key.starts_with(&prefix)), + &selected, + ) + .map_err(io::Error::other)?; + selections.sort_by_key(|selection| selection.settings.position.is_none()); Ok(OutputScan { selections, @@ -2485,6 +2542,170 @@ mod tests { use super::*; + fn three_monitor_plan(layout: crate::config::OutputLayout) -> DesiredOutputConfiguration { + let monitors = ["A", "B", "C"].map(|key| Monitor { + key: key.into(), + connector: key.into(), + identity: key.into(), + aliases: vec![], + internal: key == "A", + usable: true, + }); + let profile = OutputProfile { + name: "three".into(), + outputs: monitors + .iter() + .map(|monitor| crate::output_policy::defaults(monitor.key.clone())) + .collect(), + layout, + confirm_timeout: 15, + lid_policy: crate::config::LidPolicy::Ignore, + lid_closed: None, + mirror_source: None, + }; + crate::output_policy::select_profile(&monitors, false, &[profile], &ManualOverride::default()) + } + + #[test] + fn complete_live_plan_still_requires_hardware_success() { + let desired = three_monitor_plan(crate::config::OutputLayout::Extend); + let selected = vec!["A".into(), "B".into(), "C".into()]; + assert!(validate_output_plan(&desired, &selected, || Ok(())).is_ok()); + assert_eq!( + validate_output_plan(&desired, &selected, || Err("atomic TEST_ONLY failed".into())), + Err("atomic TEST_ONLY failed".into()) + ); + } + + #[test] + fn requiredness_does_not_allow_a_connected_enabled_output_to_be_omitted() { + let mut desired = three_monitor_plan(crate::config::OutputLayout::Extend); + desired.outputs[2].settings.required = false; + let error = validate_output_plan(&desired, &["A".into(), "B".into()], || Ok(())).unwrap_err(); + assert!(error.contains("C")); + desired.outputs[2].settings.enabled = false; + assert!(validate_output_plan(&desired, &["A".into(), "B".into()], || Ok(())).is_ok()); + } + + #[test] + fn disconnected_monitors_affect_profile_matching_and_do_not_need_assignments() { + let connected = ["A", "B"].map(|key| Monitor { + key: key.into(), + connector: key.into(), + identity: key.into(), + aliases: vec![], + internal: key == "A", + usable: true, + }); + let mut profile = OutputProfile { + name: "dock".into(), + outputs: ["A", "B", "C"] + .map(|key| crate::output_policy::defaults(key.into())) + .into(), + layout: crate::config::OutputLayout::Extend, + confirm_timeout: 15, + lid_policy: crate::config::LidPolicy::Ignore, + lid_closed: None, + mirror_source: None, + }; + for required in [true, false] { + profile.outputs[2].required = required; + let desired = + crate::output_policy::select_profile(&connected, false, &[profile.clone()], &ManualOverride::default()); + assert_eq!(desired.profile.as_deref(), if required { None } else { Some("dock") }); + assert_eq!(desired.outputs.len(), 2); + assert!(validate_output_plan(&desired, &["A".into(), "B".into()], || Ok(())).is_ok()); + } + } + + #[test] + fn device_scans_cannot_satisfy_another_gpus_missing_output() { + let mut desired = three_monitor_plan(crate::config::OutputLayout::Extend); + desired.outputs[0].key = "gpu-one/A".into(); + desired.outputs[1].key = "gpu-two/B".into(); + desired.outputs[2].key = "gpu-two/C".into(); + let selected = vec!["gpu-one/A".into(), "gpu-two/B".into(), "gpu-one/C".into()]; + assert!(validate_output_plan(&desired, &selected, || Ok(())).is_err()); + // A device-local rescan checks only its own enabled outputs. + assert!( + validate_selected_outputs( + desired + .outputs + .iter() + .filter(|output| output.key.starts_with("gpu-one/")), + &selected + ) + .is_ok() + ); + } + + #[test] + fn impossible_assignment_reaches_live_validation_as_an_error() { + let desired = three_monitor_plan(crate::config::OutputLayout::Extend); + let crtc = std::num::NonZeroU32::new(1).unwrap().into(); + let requests = (0..3) + .map(|_| crtc_assignment::Candidates { + applied: None, + kernel: None, + compatible: vec![crtc], + }) + .collect::>(); + let selected = desired + .outputs + .iter() + .zip(crtc_assignment::assign(&requests)) + .filter(|(_, assignment)| assignment.is_some()) + .map(|(output, _)| output.key.clone()) + .collect::>(); + assert!(validate_output_plan(&desired, &selected, || Ok(())).is_err()); + } + + #[test] + fn feasible_three_monitor_assignment_passes_live_plan_validation() { + let desired = three_monitor_plan(crate::config::OutputLayout::Extend); + let requests = [&[1, 2][..], &[1, 3][..], &[1, 3][..]].map(|compatible| crtc_assignment::Candidates { + applied: None, + kernel: None, + compatible: compatible + .iter() + .map(|value| std::num::NonZeroU32::new(*value).unwrap().into()) + .collect(), + }); + let selected = desired + .outputs + .iter() + .zip(crtc_assignment::assign(&requests)) + .filter(|(_, assignment)| assignment.is_some()) + .map(|(output, _)| output.key.clone()) + .collect::>(); + assert_eq!(selected.len(), 3); + assert!(validate_output_plan(&desired, &selected, || Ok(())).is_ok()); + } + + #[test] + fn live_plan_rejects_partial_extend_and_mirror_before_hardware_validation() { + for layout in [crate::config::OutputLayout::Extend, crate::config::OutputLayout::Mirror] { + let desired = three_monitor_plan(layout); + for omitted in ["A", "B", "C"] { + let selected = ["A", "B", "C"] + .into_iter() + .filter(|key| *key != omitted) + .map(String::from) + .collect::>(); + let probed = std::cell::Cell::new(false); + let result = validate_output_plan(&desired, &selected, || { + probed.set(true); + Ok(()) // A driver can accept the incomplete two-output request. + }); + assert!( + result.is_err(), + "accepted a partial {layout:?} configuration without {omitted}" + ); + assert!(!probed.get()); + } + } + } + #[test] fn repeated_unconfirmed_changes_keep_original_rollback_and_manual_state() { let a = ("confirmed A", "automatic"); diff --git a/compositor/src/backends/direct/crtc_assignment.rs b/compositor/src/backends/direct/crtc_assignment.rs index 7bbfb39..3df6e60 100644 --- a/compositor/src/backends/direct/crtc_assignment.rs +++ b/compositor/src/backends/direct/crtc_assignment.rs @@ -1,5 +1,3 @@ -use std::collections::HashSet; - use smithay::reexports::drm::control::crtc; pub(super) struct Candidates { @@ -8,45 +6,114 @@ pub(super) struct Candidates { pub compatible: Vec, } -/// Reserve surviving outputs before assigning newly enabled connectors. Kernel -/// encoder routing can be stale for a disabled connector; applied ownership wins. +/// Maximum-cardinality, minimum-cost bipartite matching. Cardinality wins over +/// preferences: augment until no path remains, even if its cost is positive. +/// For that cardinality, retain the most valid applied assignments, then the +/// most kernel assignments. Stable connector/compatible-CRTC order breaks ties. +/// One applied edge outweighs every possible kernel edge, since applied ownership +/// is authoritative and encoder routing can be stale after disabling an output. pub(super) fn assign(requests: &[Candidates]) -> Vec> { - let mut assigned = vec![None; requests.len()]; - let mut used = HashSet::new(); - - for applied in [true, false] { - for (request, assigned) in requests.iter().zip(&mut assigned) { - let preferred = if applied { request.applied } else { request.kernel }; - - if assigned.is_none() - && let Some(crtc) = preferred.filter(|crtc| request.compatible.contains(crtc)) - && used.insert(crtc) - { - *assigned = Some(crtc); + let mut crtcs = Vec::new(); + for request in requests { + for crtc in &request.compatible { + if !crtcs.contains(crtc) { + crtcs.push(*crtc); } } } - - // Give connectors with fewer choices the first free CRTC. - let mut remaining = (0..requests.len()) - .filter(|index| assigned[*index].is_none()) - .collect::>(); - - remaining.sort_by_key(|index| requests[*index].compatible.len()); - - for index in remaining { - if let Some(crtc) = requests[index].compatible.iter().find(|crtc| !used.contains(crtc)) { - used.insert(*crtc); - assigned[index] = Some(*crtc); + let source = requests.len() + crtcs.len(); + let sink = source + 1; + let mut edges = Vec::new(); + let mut assignments = Vec::new(); + let applied_weight = requests.len() as i64 + 1; + for (index, request) in requests.iter().enumerate() { + add_edge(&mut edges, source, index, 0); + for (crtc_index, crtc) in crtcs.iter().enumerate() { + if request.compatible.contains(crtc) { + let cost = -applied_weight * i64::from(request.applied == Some(*crtc)) + - i64::from(request.kernel == Some(*crtc)); + assignments.push((index, *crtc, edges.len())); + add_edge(&mut edges, index, requests.len() + crtc_index, cost); + } + } + } + for crtc_index in 0..crtcs.len() { + add_edge(&mut edges, requests.len() + crtc_index, sink, 0); + } + while augment(&mut edges, source, sink) {} + let mut assigned = vec![None; requests.len()]; + for (index, crtc, edge) in assignments { + if !edges[edge].available { + assigned[index] = Some(crtc); } } - assigned } +struct Edge { + from: usize, + to: usize, + cost: i64, + available: bool, +} + +fn add_edge(edges: &mut Vec, from: usize, to: usize, cost: i64) { + edges.push(Edge { + from, + to, + cost, + available: true, + }); + edges.push(Edge { + from: to, + to: from, + cost: -cost, + available: false, + }); +} + +/// Find the cheapest residual augmenting path. Reverse edges allow earlier +/// assignments to move. Bellman-Ford handles negative preference costs; starting +/// from an empty matching and using shortest paths leaves no negative cost cycle. +/// Failed searches leave the matching untouched. Strict improvements and stable +/// edge traversal make equal-cost choices deterministic without hash iteration. +fn augment(edges: &mut [Edge], source: usize, sink: usize) -> bool { + let mut distances = vec![None; sink + 1]; + let mut previous = vec![None; sink + 1]; + distances[source] = Some(0); + for _ in 0..sink { + let mut changed = false; + for (index, edge) in edges.iter().enumerate() { + if edge.available + && let Some(distance) = distances[edge.from] + && distances[edge.to].is_none_or(|current| distance + edge.cost < current) + { + distances[edge.to] = Some(distance + edge.cost); + previous[edge.to] = Some(index); + changed = true; + } + } + if !changed { + break; + } + } + if previous[sink].is_none() { + return false; + } + let mut vertex = sink; + while vertex != source { + let index = previous[vertex].expect("reachable augmenting path"); + edges[index].available = false; + edges[index ^ 1].available = true; + vertex = edges[index].from; + } + true +} + #[cfg(test)] mod tests { use super::*; + use std::collections::HashSet; fn crtc(value: u32) -> crtc::Handle { std::num::NonZeroU32::new(value).unwrap().into() @@ -60,6 +127,123 @@ mod tests { } } + #[test] + fn every_connector_order_finds_a_complete_matching() { + let graph = [&[1, 2][..], &[1, 3][..], &[1, 3][..]]; + for order in [[0, 1, 2], [0, 2, 1], [1, 0, 2], [1, 2, 0], [2, 0, 1], [2, 1, 0]] { + let requests = order.map(|index| candidates(None, None, graph[index])); + let result = assign(&requests); + assert_eq!(result.iter().flatten().count(), 3, "order {order:?}"); + assert_eq!(assign(&requests), result, "deterministic for the same inventory"); + } + } + + #[test] + fn equal_preferences_use_connector_order() { + let result = assign(&[candidates(Some(1), None, &[1, 2]), candidates(Some(1), None, &[1, 2])]); + assert_eq!(result, vec![Some(crtc(1)), Some(crtc(2))]); + } + + #[test] + fn preserving_two_applied_outputs_outweighs_preserving_the_first_one() { + assert_eq!( + assign(&[ + candidates(Some(1), None, &[1, 4]), + candidates(Some(2), None, &[2, 4]), + candidates(Some(3), None, &[2, 3]), + candidates(None, None, &[1, 3]), + ]), + vec![Some(crtc(4)), Some(crtc(2)), Some(crtc(3)), Some(crtc(1))] + ); + } + + #[test] + fn maximum_cardinality_and_preference_objective_match_exhaustive_oracle() { + fn score(requests: &[Candidates], assignment: &[Option]) -> (usize, usize, usize) { + let retained = [true, false].map(|applied| { + requests + .iter() + .zip(assignment) + .filter(|(request, actual)| { + let preferred = if applied { request.applied } else { request.kernel }; + preferred.is_some() && preferred == **actual + }) + .count() + }); + (assignment.iter().flatten().count(), retained[0], retained[1]) + } + + fn enumerate( + requests: &[Candidates], + prefix: &mut Vec>, + best: &mut (usize, usize, usize), + ) { + if prefix.len() == requests.len() { + *best = std::cmp::max(*best, score(requests, prefix)); + return; + } + let index = prefix.len(); + prefix.push(None); + enumerate(requests, prefix, best); + prefix.pop(); + for crtc in &requests[index].compatible { + if !prefix.contains(&Some(*crtc)) { + prefix.push(Some(*crtc)); + enumerate(requests, prefix, best); + prefix.pop(); + } + } + } + // All 512 three-by-three graphs, including empty and impossible ones. + // Vary both preference classes, including invalid/conflicting preferences. + for graph in 0..512 { + for seed in 0..8 { + let requests = (0..3) + .map(|index| { + let compatible = (1..=3) + .filter(|crtc| graph & (1 << (index * 3 + crtc - 1)) != 0) + .collect::>(); + let applied = ((seed + index) % 4 != 0).then_some((seed + index) % 4); + let kernel = ((seed * 3 + index) % 4 != 0).then_some((seed * 3 + index) % 4); + candidates(applied, kernel, &compatible) + }) + .collect::>(); + let mut best = (0, 0, 0); + enumerate(&requests, &mut Vec::new(), &mut best); + let actual = assign(&requests); + let used = actual.iter().flatten().collect::>(); + assert_eq!(used.len(), actual.iter().flatten().count()); + assert!( + requests + .iter() + .zip(&actual) + .all(|(request, crtc)| crtc.is_none_or(|crtc| request.compatible.contains(&crtc))) + ); + assert_eq!(score(&requests, &actual), best, "graph {graph}, preferences {seed}"); + } + } + } + + #[test] + fn three_flexible_connectors_need_an_augmenting_path() { + let result = assign(&[ + candidates(None, None, &[1, 2]), + candidates(None, None, &[1, 3]), + candidates(None, None, &[1, 3]), + ]); + assert_eq!(result.iter().flatten().count(), 3); + } + + #[test] + fn completeness_can_require_moving_two_applied_outputs() { + let result = assign(&[ + candidates(Some(1), Some(1), &[1, 2]), + candidates(Some(2), Some(2), &[2, 3]), + candidates(None, None, &[1]), + ]); + assert_eq!(result, vec![Some(crtc(2)), Some(crtc(3)), Some(crtc(1))]); + } + #[test] fn external_to_two_outputs_keeps_external_crtc_regardless_of_connector_order() { // Extend and mirror both require two independent scanout assignments. diff --git a/docs/configuration/outputs.md b/docs/configuration/outputs.md index 7bf5b70..8f4bcd9 100644 --- a/docs/configuration/outputs.md +++ b/docs/configuration/outputs.md @@ -74,6 +74,13 @@ filter by connector type. A layout that would disable the final usable display falls back to a working display. Profiles that explicitly disable all their listed outputs are invalid. +`required` controls whether a profile matches when a monitor is absent. Every +connected monitor enabled by the resolved policy, including optional monitors +and mirror targets, needs its own compatible CRTC assignment. Ferese rejects an +incomplete assignment before applying the configuration. A complete assignment +still needs to pass driver validation; connector compatibility alone does not +establish that the hardware accepts the modes and planes. + Monitor identities use EDID manufacturer, product/model and serial, rather than a hash of the entire EDID. Without a usable serial, the identity includes the GPU's sysfs hardware path and connector. For duplicate serials, Ferese adds the