docs: Add more explanations to the legacy-drm-code

src/backend/drm/legacy/mod.rs

@@ -2,11 +2,20 @@
 //! [`RawDevice`](RawDevice) and [`RawSurface`](RawSurface)
 //! implementations using the legacy mode-setting infrastructure.
 //!
+//! Legacy mode-setting refers to the now outdated, but still supported direct manager api
+//! of the linux kernel. Adaptations of this api can be found in BSD kernels.
+//!
+//! The newer and objectively better api is known as atomic-modesetting (or nuclear-page-flip),
+//! however this api is not supported by every driver, so this is provided for backwards compatibility.
+//! Currenly there are no features in smithay, that are exclusive to the atomic api.
+//!
 //! Usually this implementation will be wrapped into a [`GbmDevice`](::backend::drm::gbm::GbmDevice).
 //! Take a look at `anvil`s source code for an example of this.
 //!
 //! For an example how to use this standalone, take a look at the `raw_legacy_drm` example.
 //!
+//! For detailed overview of these abstractions take a look at the module documentation of backend::drm.
+//!
 
 use super::{common::Error, DevPath, Device, DeviceHandler, RawDevice};
 
@@ -66,6 +75,8 @@ impl<A: AsRawFd + 'static> Drop for Dev<A> {
             // Here we restore the tty to it's previous state.
             // In case e.g. getty was running on the tty sets the correct framebuffer again,
             // so that getty will be visible.
+            // We do exit correctly, if this fails, but the user will be presented with
+            // a black screen, if no display handler takes control again.
             let old_state = self.old_state.clone();
             for (handle, (info, connectors)) in old_state {
                 if let Err(err) = self.set_crtc(
@@ -88,7 +99,7 @@ impl<A: AsRawFd + 'static> Drop for Dev<A> {
 }
 
 impl<A: AsRawFd + 'static> LegacyDrmDevice<A> {
-    /// Create a new [`LegacyDrmDevice`] from an open drm node
+    /// Create a new [`LegacyDrmDevice`] from an open drm node.
     ///
     /// # Arguments
     ///
@@ -115,6 +126,8 @@ impl<A: AsRawFd + 'static> LegacyDrmDevice<A> {
             .map_err(Error::UnableToGetDeviceId)?
             .st_rdev;
 
+        // we wrap some of the internal state in another struct to share with
+        // the surfaces and event loop handlers.
         let active = Arc::new(AtomicBool::new(true));
         let mut dev = Dev {
             fd: dev,
@@ -124,13 +137,17 @@ impl<A: AsRawFd + 'static> LegacyDrmDevice<A> {
             logger: log.clone(),
         };
 
-        // we want to modeset, so we better be the master, if we run via a tty session
+        // We want to modeset, so we better be the master, if we run via a tty session.
+        // This is only needed on older kernels. Newer kernels grant this permission,
+        // if no other process is already the *master*. So we skip over this error.
         if dev.acquire_master_lock().is_err() {
             warn!(log, "Unable to become drm master, assuming unprivileged mode");
             dev.privileged = false;
         };
 
-        // enumerate (and save) the current device state
+        // Enumerate (and save) the current device state.
+        // We need to keep the previous device configuration to restore the state later,
+        // so we query everything, that we can set.
         let res_handles = ControlDevice::resource_handles(&dev)
             .compat()
             .map_err(|source| Error::Access {
@@ -165,11 +182,18 @@ impl<A: AsRawFd + 'static> LegacyDrmDevice<A> {
             }
         }
 
+        // If the user does not explicitly requests us to skip this,
+        // we clear out the complete connector<->crtc mapping on device creation.
+        //
+        // The reason is, that certain operations may be racy otherwise, as surfaces can
+        // exist on different threads. As a result, we cannot enumerate the current state
+        // on surface creation (it might be changed on another thread during the enumeration).
+        // An easy workaround is to set a known state on device creation.
         if disable_connectors {
             dev.set_connector_state(res_handles.connectors().iter().copied(), false)?;
 
             for crtc in res_handles.crtcs() {
-                // null commit
+                // null commit (necessary to trigger removal on the kernel side with the legacy api.)
                 dev.set_crtc(*crtc, None, (0, 0), &[], None)
                     .compat()
                     .map_err(|source| Error::Access {
@@ -199,6 +223,7 @@ impl<A: AsRawFd + 'static> Dev<A> {
         connectors: impl Iterator<Item = connector::Handle>,
         enabled: bool,
     ) -> Result<(), Error> {
+        // for every connector...
         for conn in connectors {
             let info = self
                 .get_connector(conn)
@@ -208,7 +233,9 @@ impl<A: AsRawFd + 'static> Dev<A> {
                     dev: self.dev_path(),
                     source,
                 })?;
+            // that is currently connected ...
             if info.state() == connector::State::Connected {
+                // get a list of it's properties.
                 let props = self
                     .get_properties(conn)
                     .compat()
@@ -218,7 +245,9 @@ impl<A: AsRawFd + 'static> Dev<A> {
                         source,
                     })?;
                 let (handles, _) = props.as_props_and_values();
+                // for every handle ...
                 for handle in handles {
+                    // get information of that property
                     let info = self
                         .get_property(*handle)
                         .compat()
@@ -227,7 +256,9 @@ impl<A: AsRawFd + 'static> Dev<A> {
                             dev: self.dev_path(),
                             source,
                         })?;
+                    // to find out, if we got the handle of the "DPMS" property ...
                     if info.name().to_str().map(|x| x == "DPMS").unwrap_or(false) {
+                        // so we can use that to turn on / off the connector
                         self.set_property(
                             conn,
                             *handle,

src/backend/drm/legacy/session.rs

@@ -72,6 +72,8 @@ impl<A: AsRawFd + 'static> LegacyDrmDeviceObserver<A> {
                 for surface in backends.borrow().values().filter_map(Weak::upgrade) {
                     // other ttys that use no cursor, might not clear it themselves.
                     // This makes sure our cursor won't stay visible.
+                    //
+                    // This usually happens with getty, and a cursor on top of a kernel console looks very weird.
                     let _ = (*device).set_cursor(
                         surface.crtc,
                         Option::<&drm::control::dumbbuffer::DumbBuffer>::None,
@@ -119,6 +121,11 @@ impl<A: AsRawFd + 'static> LegacyDrmDeviceObserver<A> {
 }
 
 impl<A: AsRawFd + 'static> LegacyDrmDeviceObserver<A> {
+    // reset state enumerates the actual current state of the drm device
+    // and applies that to the `current_state` as saved in the surfaces.
+    //
+    // This re-sync is necessary after a tty swap, as the pipeline might
+    // be left in a different state.
     fn reset_state(&mut self) -> Result<(), Error> {
         // lets enumerate it the current state
         if let Some(dev) = self.dev.upgrade() {

src/backend/drm/legacy/surface.rs

@@ -70,10 +70,13 @@ impl<A: AsRawFd + 'static> CursorBackend for LegacyDrmSurfaceInternal<A> {
 
         trace!(self.logger, "Setting the new imported cursor");
 
+        // set_cursor2 allows us to set the hotspot, but is not supported by every implementation.
         if self
             .set_cursor2(self.crtc, Some(buffer), (hotspot.0 as i32, hotspot.1 as i32))
             .is_err()
         {
+            // the cursor will be slightly misplaced, when using the function for hotspots other then (0, 0),
+            // but that is still better then no cursor.
             self.set_cursor(self.crtc, Some(buffer))
                 .compat()
                 .map_err(|source| Error::Access {
@@ -221,7 +224,7 @@ impl<A: AsRawFd + 'static> RawSurface for LegacyDrmSurfaceInternal<A> {
             self.dev.set_connector_state(removed.copied(), false)?;
 
             if conn_removed {
-                // We need to do a null commit to free graphics pipelines
+                // null commit (necessary to trigger removal on the kernel side with the legacy api.)
                 self.set_crtc(self.crtc, None, (0, 0), &[], None)
                     .compat()
                     .map_err(|source| Error::Access {
@@ -246,6 +249,7 @@ impl<A: AsRawFd + 'static> RawSurface for LegacyDrmSurfaceInternal<A> {
         }
 
         debug!(self.logger, "Setting screen");
+        // do a modeset and attach the given framebuffer
         self.set_crtc(
             self.crtc,
             Some(framebuffer),
@@ -266,6 +270,11 @@ impl<A: AsRawFd + 'static> RawSurface for LegacyDrmSurfaceInternal<A> {
 
         *current = pending.clone();
 
+        // set crtc does not trigger page_flip events, so we immediately queue a flip
+        // with the same framebuffer.
+        // this will result in wasting a frame, because this flip will need to wait
+        // for `set_crtc`, but is necessary to drive the event loop and thus provide
+        // a more consistent api.
         ControlDevice::page_flip(
             self,
             self.crtc,
@@ -316,7 +325,8 @@ impl<A: AsRawFd + 'static> LegacyDrmSurfaceInternal<A> {
             "Initializing drm surface with mode {:?} and connectors {:?}", mode, connectors
         );
 
-        // Try to enumarate the current state to set the initial state variable correctly
+        // Try to enumarate the current state to set the initial state variable correctly.
+        // We need an accurate state to handle `commit_pending`.
         let crtc_info = dev.get_crtc(crtc).compat().map_err(|source| Error::Access {
             errmsg: "Error loading crtc info",
             dev: dev.dev_path(),
@@ -377,6 +387,13 @@ impl<A: AsRawFd + 'static> LegacyDrmSurfaceInternal<A> {
         Ok(surface)
     }
 
+    // we use this function to verify, if a certain connector/mode combination
+    // is valid on our crtc. We do this with the most basic information we have:
+    // - is there a matching encoder
+    // - does the connector support the provided Mode.
+    //
+    // Better would be some kind of test commit to ask the driver,
+    // but that only exists for the atomic api.
     fn check_connector(&self, conn: connector::Handle, mode: &Mode) -> Result<bool, Error> {
         let info = self
             .get_connector(conn)