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).

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) {
+        match state.device_added(dev, path.into(), dh) {
-            warn!("Failed to add device {}: {:?}", path.display(), err);
+            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) {
+                    match state.device_changed(dev) {
-                        error!(?err, "Failed to update drm device {}.", path.display(),);
+                        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) {
+                    match state.device_added(dev, path.into(), &dh) {
-                        error!(?err, "Failed to add drm device {}.", path.display(),);
+                        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(
+                backend
-                    false,
+                    .apply_config_for_outputs(
-                    loop_handle,
+                        false,
-                    self.dynamic_conf.screen_filter(),
+                        loop_handle,
-                    shell.clone(),
+                        self.dynamic_conf.screen_filter(),
-                    workspace_state,
+                        shell.clone(),
-                    xdg_activation_state,
+                        workspace_state,
-                    startup_done,
+                        xdg_activation_state,
-                    clock,
+                        startup_done,
-                ) {
+                        clock,
-                    error!(?err, "Failed to reset config.");
+                    )
-                } else {
+                    .context("Failed to reset config")?;
-                    for (output, enabled) in found_outputs {
+
-                        if enabled == OutputState::Enabled {
+                for (output, enabled) in found_outputs {
-                            output_state.enable_head(&output);
+                    if enabled == OutputState::Enabled {
-                        } else {
+                        output_state.enable_head(&output);
-                            output_state.disable_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(
+            let mut backend = backend.lock();
-                false,
+            backend
-                loop_handle,
+                .apply_config_for_outputs(
-                self.dynamic_conf.screen_filter(),
+                    false,
-                shell.clone(),
+                    loop_handle,
-                workspace_state,
+                    self.dynamic_conf.screen_filter(),
-                xdg_activation_state,
+                    shell.clone(),
-                startup_done,
+                    workspace_state,
-                clock,
+                    xdg_activation_state,
-            ) {
+                    startup_done.clone(),
-                warn!(?err, "Failed to set new config.",);
+                    clock,
-            } else {
+                )
-                for output in outputs {
+                .context("Failed to set new config")?;
-                    if output
+
-                        .user_data()
+            for output in outputs {
-                        .get::<RefCell<OutputConfig>>()
+                if output
-                        .unwrap()
+                    .user_data()
-                        .borrow()
+                    .get::<RefCell<OutputConfig>>()
-                        .enabled
+                    .unwrap()
-                        == OutputState::Enabled
+                    .borrow()
-                    {
+                    .enabled
-                        output_state.enable_head(&output);
+                    == OutputState::Enabled
-                    } else {
+                {
-                        output_state.disable_head(&output);
+                    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(

src/dbus/mod.rs

@@ -1,7 +1,12 @@
-use crate::state::{BackendData, Common, State};
+use crate::{
+    state::{BackendData, Common, State},
+    utils::prelude::OutputExt,
+};
 use anyhow::{Context, Result};
 use calloop::{InsertError, LoopHandle, RegistrationToken};
+use cosmic_comp_config::output::comp::OutputState;
 use std::collections::HashMap;
+use tracing::{error, warn};
 use zbus::blocking::{fdo::DBusProxy, Connection};
 
 #[cfg(feature = "systemd")]
@@ -24,12 +29,26 @@ pub fn init(evlh: &LoopHandle<'static, State>) -> Result<Vec<RegistrationToken>>
                             }
                             _ => Vec::new(),
                         };
+                        let mut added = Vec::new();
                         for node in nodes {
-                            if let Err(err) = state.device_changed(node.dev_id()) {
+                            match state.device_changed(node.dev_id()) {
-                                tracing::error!(?err, "Failed to update drm device {}.", node);
+                                Ok(outputs) => added.extend(outputs),
+                                Err(err) => {
+                                    tracing::error!(?err, "Failed to update drm device {}.", node)
+                                }
+                            }
+                        }
+                        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.refresh_output_config();
 
                         ()
                     }

src/input/mod.rs

@@ -69,7 +69,7 @@ use smithay::{
         tablet_manager::{TabletDescriptor, TabletSeatTrait},
     },
 };
-use tracing::{error, trace};
+use tracing::{error, trace, warn};
 use xkbcommon::xkb::{Keycode, Keysym};
 
 use std::{
@@ -1525,17 +1525,38 @@ impl State {
             InputEvent::SwitchToggle { event } => {
                 #[cfg(feature = "systemd")]
                 if event.switch() == Some(Switch::Lid) && self.common.inhibit_lid_fd.is_some() {
-                    if event.state() == SwitchState::On {
+                    let backend = self.backend.lock();
-                        self.backend
+                    let output = backend
-                            .lock()
+                        .all_outputs()
+                        .iter()
+                        .find(|o| o.is_internal())
+                        .cloned();
+                    let closed = event.state() == SwitchState::On;
+
+                    if closed {
+                        backend
                             .disable_internal_output(&mut self.common.output_configuration_state);
                     } else {
-                        self.backend
+                        backend.enable_internal_output(&mut self.common.output_configuration_state);
-                            .lock()
-                            .enable_internal_output(&mut self.common.output_configuration_state);
                     }
+                    std::mem::drop(backend);
 
-                    self.refresh_output_config();
+                    if let Err(err) = self.refresh_output_config() {
+                        if !closed {
+                            warn!(?err, "Failed to re-enable internal connector");
+                            if let Some(output) = output {
+                                use cosmic_comp_config::output::comp::OutputState;
+
+                                output.config_mut().enabled = OutputState::Disabled;
+                                if let Err(err) = self.refresh_output_config() {
+                                    error!("Unrecoverable output configuration error: {}", err);
+                                }
+                            }
+                        } else {
+                            // Disabling an output should never fail.
+                            error!("Unrecoverable output configuration error: {}", err);
+                        }
+                    }
                 }
             }
         }

src/state.rs

@@ -775,7 +775,7 @@ impl State {
         #[cfg(feature = "systemd")]
         {
             use smithay::backend::session::Session;
-            use tracing::{debug, error};
+            use tracing::{debug, error, warn};
 
             let outputs = self.backend.lock().all_outputs();
             let is_active = match &self.backend {
@@ -793,17 +793,42 @@ impl State {
                             debug!("Inhibiting lid switch");
                             self.common.inhibit_lid_fd = Some(fd);
 
-                            if crate::dbus::logind::lid_closed().unwrap_or(false) {
+                            let backend = self.backend.lock();
-                                self.backend.lock().disable_internal_output(
+                            let output = backend
+                                .all_outputs()
+                                .iter()
+                                .find(|o| o.is_internal())
+                                .cloned();
+                            let closed = crate::dbus::logind::lid_closed().unwrap_or(false);
+
+                            if closed {
+                                backend.disable_internal_output(
                                     &mut self.common.output_configuration_state,
                                 );
                             } else {
-                                self.backend.lock().enable_internal_output(
+                                backend.enable_internal_output(
                                     &mut self.common.output_configuration_state,
                                 );
                             }
+                            std::mem::drop(backend);
 
-                            self.refresh_output_config();
+                            if let Err(err) = self.refresh_output_config() {
+                                if !closed {
+                                    warn!(?err, "Failed to re-enable internal connector");
+                                    if let Some(output) = output {
+                                        output.config_mut().enabled = OutputState::Disabled;
+                                        if let Err(err) = self.refresh_output_config() {
+                                            error!(
+                                                "Unrecoverable output configuration error: {}",
+                                                err
+                                            );
+                                        }
+                                    }
+                                } else {
+                                    // Disabling an output should never fail.
+                                    error!("Unrecoverable output configuration error: {}", err);
+                                }
+                            }
                         }
                         Err(err) => {
                             error!("Failed to inhibit lid switch: {}", err);
@@ -814,11 +839,24 @@ impl State {
                 if let Some(_fd) = self.common.inhibit_lid_fd.take() {
                     debug!("Removing inhibitor-lock on lid switch");
 
-                    self.backend
+                    let backend = self.backend.lock();
-                        .lock()
+                    let output = backend
-                        .enable_internal_output(&mut self.common.output_configuration_state);
+                        .all_outputs()
+                        .iter()
+                        .find(|o| o.is_internal())
+                        .cloned();
+                    backend.enable_internal_output(&mut self.common.output_configuration_state);
+                    std::mem::drop(backend);
 
-                    self.refresh_output_config();
+                    if let Err(err) = self.refresh_output_config() {
+                        warn!(?err, "Failed to re-enable internal connector");
+                        if let Some(output) = output {
+                            output.config_mut().enabled = OutputState::Disabled;
+                            if let Err(err) = self.refresh_output_config() {
+                                error!("Unrecoverable output configuration error: {}", err);
+                            }
+                        }
+                    }
                     // drop _fd
                 }
             }