config: Make read_outputs failable

Previously we ignored when we had no output configuration
**and** failed to apply the automatically created one.

This leads to two problems:
- If this happens on startup, we end up with no outputs being added to the shell and we quit.
- If this happens later, we might end up in an inconsistent state, where the shell thinks we have an output, when it didn't light up for similar reasons.

Thus `read_outputs` is failable and handling that very much depends on
the where is was called from, because `read_outputs` doesn't know what
configuration was active before.

Thus make it failable and provide useful mitigations everywhere
possible:
- Try to enable just one output in case we fail on startup.
- Don't enable any additional outputs, when we fail on hotplug.
- Log the error like previously in any other case (and come up with more
  mitigations, once we understand these cases better).

Author: Drakulix

src/backend/kms/device.rs

@@ -171,9 +171,14 @@ pub fn init_egl(gbm: &GbmDevice<DrmDeviceFd>) -> Result<EGLInternals> {
 }
 
 impl State {
-    pub fn device_added(&mut self, dev: dev_t, path: &Path, dh: &DisplayHandle) -> Result<()> {
+    pub fn device_added(
+        &mut self,
+        dev: dev_t,
+        path: &Path,
+        dh: &DisplayHandle,
+    ) -> Result<Vec<Output>> {
         if !self.backend.kms().session.is_active() {
-            return Ok(());
+            return Ok(Vec::new());
         }
 
         if let Some(allowlist) = dev_list_var("COSMIC_DRM_ALLOW_DEVICES") {
@@ -195,7 +200,7 @@ impl State {
                         "Skipping device {} due to COSMIC_DRM_ALLOW_DEVICE list.",
                         path.display()
                     );
-                    return Ok(());
+                    return Ok(Vec::new());
                 }
             }
         }
@@ -212,7 +217,7 @@ impl State {
                             "Skipping device {} due to COSMIC_DRM_BLOCK_DEVICE list.",
                             path.display()
                         );
-                        return Ok(());
+                        return Ok(Vec::new());
                     }
                 }
             }
@@ -384,12 +389,12 @@ impl State {
             .add_heads(wl_outputs.iter());
 
         self.backend.kms().refresh_used_devices()?;
-        Ok(())
+        Ok(wl_outputs)
     }
 
-    pub fn device_changed(&mut self, dev: dev_t) -> Result<()> {
+    pub fn device_changed(&mut self, dev: dev_t) -> Result<Vec<Output>> {
         if !self.backend.kms().session.is_active() {
-            return Ok(());
+            return Ok(Vec::new());
         }
 
         let drm_node = DrmNode::from_dev_id(dev)?;
@@ -475,7 +480,7 @@ impl State {
         }
 
         self.backend.kms().refresh_used_devices()?;
-        Ok(())
+        Ok(outputs_added)
     }
 
     pub fn device_removed(&mut self, dev: dev_t, dh: &DisplayHandle) -> Result<()> {
@@ -548,7 +553,7 @@ impl State {
         Ok(())
     }
 
-    pub fn refresh_output_config(&mut self) {
+    pub fn refresh_output_config(&mut self) -> Result<()> {
         self.common.config.read_outputs(
             &mut self.common.output_configuration_state,
             &mut self.backend,
@@ -558,8 +563,9 @@ impl State {
             &self.common.xdg_activation_state,
             self.common.startup_done.clone(),
             &self.common.clock,
-        );
+        )?;
         self.common.refresh();
+        Ok(())
     }
 }
 

src/backend/kms/mod.rs

@@ -133,16 +133,40 @@ pub fn init_backend(
     });
 
     // manually add already present gpus
+    let mut outputs = Vec::new();
     for (dev, path) in udev_dispatcher.as_source_ref().device_list() {
-        if let Err(err) = state.device_added(dev, path.into(), dh) {
-            warn!("Failed to add device {}: {:?}", path.display(), err);
+        match state.device_added(dev, path.into(), dh) {
+            Ok(added) => outputs.extend(added),
+            Err(err) => warn!("Failed to add device {}: {:?}", path.display(), err),
         }
     }
 
     if let Err(err) = state.backend.kms().select_primary_gpu(dh) {
         warn!("Failed to determine primary gpu: {}", err);
     }
-    state.refresh_output_config();
+
+    if let Err(err) = state.refresh_output_config() {
+        info!(
+            ?err,
+            "Couldn't enable all found outputs, trying to disable outputs."
+        );
+        if let Some(pos) = outputs
+            .iter()
+            .position(|o| o.is_internal())
+            .or((!outputs.is_empty()).then_some(0))
+        {
+            for (i, output) in outputs.iter().enumerate() {
+                output.config_mut().enabled = if i == pos {
+                    OutputState::Enabled
+                } else {
+                    OutputState::Disabled
+                };
+            }
+            if let Err(err) = state.refresh_output_config() {
+                error!("Couldn't enable any output: {}", err);
+            }
+        }
+    }
 
     // start x11
     let primary = state.backend.kms().primary_node.read().unwrap().clone();
@@ -281,9 +305,10 @@ fn init_udev(
                 .with_context(|| format!("Failed to update drm device: {}", device_id)),
             UdevEvent::Removed { device_id } => state
                 .device_removed(device_id, &dh)
-                .with_context(|| format!("Failed to remove drm device: {}", device_id)),
+                .with_context(|| format!("Failed to remove drm device: {}", device_id))
+                .map(|_| Vec::new()),
         } {
-            Ok(()) => {
+            Ok(added) => {
                 debug!("Successfully handled udev event.");
 
                 {
@@ -297,7 +322,17 @@ fn init_udev(
                     }
                 }
 
-                state.refresh_output_config();
+                if let Err(err) = state.refresh_output_config() {
+                    warn!("Unable to load output config: {}", err);
+                    if !added.is_empty() {
+                        for output in added {
+                            output.config_mut().enabled = OutputState::Disabled;
+                        }
+                        if let Err(err) = state.refresh_output_config() {
+                            error!("Unrecoverable config error: {}", err);
+                        }
+                    }
+                }
             }
             Err(err) => {
                 error!(?err, "Error while handling udev event.")
@@ -339,6 +374,7 @@ impl State {
         let dispatcher = dispatcher.clone();
         loop_handle.insert_idle(move |state| {
             // add new devices, update devices now
+            let mut added = Vec::new();
             for (dev, path) in dispatcher.as_source_ref().device_list() {
                 let drm_node = match DrmNode::from_dev_id(dev) {
                     Ok(node) => node,
@@ -348,19 +384,33 @@ impl State {
                     }
                 };
                 if state.backend.kms().drm_devices.contains_key(&drm_node) {
-                    if let Err(err) = state.device_changed(dev) {
-                        error!(?err, "Failed to update drm device {}.", path.display(),);
+                    match state.device_changed(dev) {
+                        Ok(outputs) => added.extend(outputs),
+                        Err(err) => {
+                            error!(?err, "Failed to update drm device {}.", path.display(),)
+                        }
                     }
                 } else {
                     let dh = state.common.display_handle.clone();
-                    if let Err(err) = state.device_added(dev, path.into(), &dh) {
-                        error!(?err, "Failed to add drm device {}.", path.display(),);
+                    match state.device_added(dev, path.into(), &dh) {
+                        Ok(outputs) => added.extend(outputs),
+                        Err(err) => error!(?err, "Failed to add drm device {}.", path.display(),),
                     }
                 }
             }
 
             // update outputs
-            state.refresh_output_config();
+            if let Err(err) = state.refresh_output_config() {
+                warn!("Unable to load output config: {}", err);
+                if !added.is_empty() {
+                    for output in added {
+                        output.config_mut().enabled = OutputState::Disabled;
+                    }
+                    if let Err(err) = state.refresh_output_config() {
+                        error!("Unrecoverable config error: {}", err);
+                    }
+                }
+            }
             state.common.refresh();
         });
         loop_signal.wakeup();

src/backend/winit.rs

@@ -227,7 +227,7 @@ pub fn init_backend(
         .add_heads(std::iter::once(&output));
     {
         state.common.add_output(&output);
-        state.common.config.read_outputs(
+        if let Err(err) = state.common.config.read_outputs(
             &mut state.common.output_configuration_state,
             &mut state.backend,
             &state.common.shell,
@@ -236,7 +236,9 @@ pub fn init_backend(
             &state.common.xdg_activation_state,
             state.common.startup_done.clone(),
             &state.common.clock,
-        );
+        ) {
+            error!("Unrecoverable output config error: {}", err);
+        }
         state.common.refresh();
     }
     state.launch_xwayland(None);

src/backend/x11.rs

@@ -370,7 +370,7 @@ pub fn init_backend(
         .add_heads(std::iter::once(&output));
     {
         state.common.add_output(&output);
-        state.common.config.read_outputs(
+        if let Err(err) = state.common.config.read_outputs(
             &mut state.common.output_configuration_state,
             &mut state.backend,
             &state.common.shell,
@@ -379,7 +379,9 @@ pub fn init_backend(
             &state.common.xdg_activation_state,
             state.common.startup_done.clone(),
             &state.common.clock,
-        );
+        ) {
+            error!("Unrecoverable output configuration error: {}", err);
+        }
         state.common.refresh();
     }
     state.launch_xwayland(None);

src/config/mod.rs

@@ -8,6 +8,7 @@ use crate::{
         output_configuration::OutputConfigurationState, workspace::WorkspaceUpdateGuard,
     },
 };
+use anyhow::Context;
 use cosmic_config::{ConfigGet, CosmicConfigEntry};
 use cosmic_settings_config::window_rules::ApplicationException;
 use cosmic_settings_config::{shortcuts, window_rules, Shortcuts};
@@ -386,7 +387,7 @@ impl Config {
         xdg_activation_state: &XdgActivationState,
         startup_done: Arc<AtomicBool>,
         clock: &Clock<Monotonic>,
-    ) {
+    ) -> anyhow::Result<()> {
         let outputs = output_state.outputs().collect::<Vec<_>>();
         let mut infos = outputs
             .iter()
@@ -471,24 +472,24 @@ impl Config {
                     found_outputs.push((output.clone(), enabled));
                 }
 
-                if let Err(err) = backend.apply_config_for_outputs(
-                    false,
-                    loop_handle,
-                    self.dynamic_conf.screen_filter(),
-                    shell.clone(),
-                    workspace_state,
-                    xdg_activation_state,
-                    startup_done,
-                    clock,
-                ) {
-                    error!(?err, "Failed to reset config.");
-                } else {
-                    for (output, enabled) in found_outputs {
-                        if enabled == OutputState::Enabled {
-                            output_state.enable_head(&output);
-                        } else {
-                            output_state.disable_head(&output);
-                        }
+                backend
+                    .apply_config_for_outputs(
+                        false,
+                        loop_handle,
+                        self.dynamic_conf.screen_filter(),
+                        shell.clone(),
+                        workspace_state,
+                        xdg_activation_state,
+                        startup_done,
+                        clock,
+                    )
+                    .context("Failed to reset config")?;
+
+                for (output, enabled) in found_outputs {
+                    if enabled == OutputState::Enabled {
+                        output_state.enable_head(&output);
+                    } else {
+                        output_state.disable_head(&output);
                     }
                 }
             } else {
@@ -529,37 +530,39 @@ impl Config {
                 w += output.geometry().size.w as u32;
             }
 
-            if let Err(err) = backend.lock().apply_config_for_outputs(
-                false,
-                loop_handle,
-                self.dynamic_conf.screen_filter(),
-                shell.clone(),
-                workspace_state,
-                xdg_activation_state,
-                startup_done,
-                clock,
-            ) {
-                warn!(?err, "Failed to set new config.",);
-            } else {
-                for output in outputs {
-                    if output
-                        .user_data()
-                        .get::<RefCell<OutputConfig>>()
-                        .unwrap()
-                        .borrow()
-                        .enabled
-                        == OutputState::Enabled
-                    {
-                        output_state.enable_head(&output);
-                    } else {
-                        output_state.disable_head(&output);
-                    }
+            let mut backend = backend.lock();
+            backend
+                .apply_config_for_outputs(
+                    false,
+                    loop_handle,
+                    self.dynamic_conf.screen_filter(),
+                    shell.clone(),
+                    workspace_state,
+                    xdg_activation_state,
+                    startup_done.clone(),
+                    clock,
+                )
+                .context("Failed to set new config")?;
+
+            for output in outputs {
+                if output
+                    .user_data()
+                    .get::<RefCell<OutputConfig>>()
+                    .unwrap()
+                    .borrow()
+                    .enabled
+                    == OutputState::Enabled
+                {
+                    output_state.enable_head(&output);
+                } else {
+                    output_state.disable_head(&output);
                 }
             }
-
             output_state.update();
             self.write_outputs(output_state.outputs());
         }
+
+        Ok(())
     }
 
     pub fn write_outputs(