fix(outputs): complete CRTC matching before live validation (#45)

Co-authored-by: Samuel Onoja <[email protected]>

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<usize, String> {
+    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::<Result<Vec<_>, _>>()?;
+    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::<Vec<_>>();
+    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<Item = &'a crate::output_policy::DesiredOutput>,
+    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::<Vec<_>>();
+    // 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::<Vec<_>>();
+        let selected = desired
+            .outputs
+            .iter()
+            .zip(crtc_assignment::assign(&requests))
+            .filter(|(_, assignment)| assignment.is_some())
+            .map(|(output, _)| output.key.clone())
+            .collect::<Vec<_>>();
+        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::<Vec<_>>();
+        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::<Vec<_>>();
+                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");

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<crtc::Handle>,
 }
 
-/// 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<Option<crtc::Handle>> {
-    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::<Vec<_>>();
-
-    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<Edge>, 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<crtc::Handle>]) -> (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<Option<crtc::Handle>>,
+            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::<Vec<_>>();
+                        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::<Vec<_>>();
+                let mut best = (0, 0, 0);
+                enumerate(&requests, &mut Vec::new(), &mut best);
+                let actual = assign(&requests);
+                let used = actual.iter().flatten().collect::<HashSet<_>>();
+                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.