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

src/backend/drm/atomic/mod.rs

@@ -2,6 +2,14 @@
 //! [`RawDevice`](RawDevice) and [`RawSurface`](RawSurface)
 //! implementations using the atomic mode-setting infrastructure.
 //!
+//! Atomic mode-setting (previously referred to a nuclear page-flip) is a new api of the Direct Rendering
+//! Manager subsystem of the linux kernel. Adaptations of this api can be found in BSD kernels.
+//!
+//! This api is objectively better than the outdated legacy-api, but not supported by every driver.
+//! Initialization will fail, if the api is unsupported. The legacy-api is also wrapped by smithay
+//! and may be used instead in these cases. Currently there are no features in smithay that are
+//! exclusive to the atomic api.
+//!
 //! Usually this implementation will wrapped into a [`GbmDevice`](::backend::drm::gbm::GbmDevice).
 //! Take a look at `anvil`s source code for an example of this.
 //!
@@ -88,9 +96,11 @@ impl<A: AsRawFd + 'static> Drop for Dev<A> {
             // Here we restore the card/tty's 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.
 
+            // create an atomic mode request consisting of all properties we captured on creation.
             let mut req = AtomicModeReq::new();
-
             fn add_multiple_props<T: ResourceHandle>(
                 req: &mut AtomicModeReq,
                 old_state: &[(T, PropertyValueSet)],
@@ -215,6 +225,8 @@ impl<A: AsRawFd + 'static> AtomicDrmDevice<A> {
 
         let dev_id = fstat(fd.as_raw_fd()).map_err(Error::UnableToGetDeviceId)?.st_rdev;
 
+        // we wrap some of the internal states in another struct to share with
+        // the surfaces and event loop handlers.
         let active = Arc::new(AtomicBool::new(true));
         let mut dev = Dev {
             fd,
@@ -225,13 +237,22 @@ impl<A: AsRawFd + 'static> AtomicDrmDevice<A> {
             logger: log.clone(),
         };
 
-        // we want to modeset, so we better be the master, if we run via a tty session
+        // we need to be the master to do modesetting 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;
         };
 
-        // enable the features we need
+        // Enable the features we need.
+        // We technically could use the atomic api without universal plane support.
+        // But the two are almost exclusively implemented together, as plane synchronization
+        // is one of the killer-features of the atomic api.
+        //
+        // We could bould more abstractions in smithay for devices with partial support,
+        // but for now we role with the oldest possible api (legacy) and the newest feature set
+        // we can use (atomic + universal planes), although we barely use planes yet.
         dev.set_client_capability(ClientCapability::UniversalPlanes, true)
             .compat()
             .map_err(|source| Error::Access {
@@ -247,7 +268,7 @@ impl<A: AsRawFd + 'static> AtomicDrmDevice<A> {
                 source,
             })?;
 
-        // enumerate (and save) the current device state
+        // Enumerate (and save) the current device state.
         let res_handles = ControlDevice::resource_handles(&dev)
             .compat()
             .map_err(|source| Error::Access {
@@ -266,11 +287,16 @@ impl<A: AsRawFd + 'static> AtomicDrmDevice<A> {
         let mut old_state = dev.old_state.clone();
         let mut mapping = dev.prop_mapping.clone();
 
+        // This helper function takes a snapshot of the current device properties.
+        // (everything in the atomic api is set via properties.)
         dev.add_props(res_handles.connectors(), &mut old_state.0)?;
         dev.add_props(res_handles.crtcs(), &mut old_state.1)?;
         dev.add_props(res_handles.framebuffers(), &mut old_state.2)?;
         dev.add_props(planes, &mut old_state.3)?;
 
+        // And because the mapping is not consistent across devices,
+        // we also need to lookup the handle for a property name.
+        // And we do this a fair bit, so lets cache that mapping.
         dev.map_props(res_handles.connectors(), &mut mapping.0)?;
         dev.map_props(res_handles.crtcs(), &mut mapping.1)?;
         dev.map_props(res_handles.framebuffers(), &mut mapping.2)?;
@@ -280,6 +306,17 @@ impl<A: AsRawFd + 'static> AtomicDrmDevice<A> {
         dev.prop_mapping = mapping;
         trace!(log, "Mapping: {:#?}", dev.prop_mapping);
 
+        // 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. Surfaces can
+        // exist on different threads: as a result, we cannot really enumerate the current state
+        // (it might be changed on another thread during the enumeration). And commits can fail,
+        // if e.g. a connector is already bound to another surface, which is difficult to analyse at runtime.
+        //
+        // An easy workaround is to set a known state on device creation, so we can only
+        // run into these errors on our own and not because previous compositors left the device
+        // in a funny state.
         if disable_connectors {
             // Disable all connectors as initial state
             let mut req = AtomicModeReq::new();

src/backend/drm/atomic/session.rs

@@ -75,6 +75,8 @@ impl<A: AsRawFd + 'static> AtomicDrmDeviceObserver<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.
                 if let Err(err) = surface.clear_plane(surface.planes.cursor) {
                     warn!(
                         self.logger,
@@ -123,6 +125,13 @@ impl<A: AsRawFd + 'static> AtomicDrmDeviceObserver<A> {
     }
 
     fn reset_state(&mut self) -> Result<(), Error> {
+        // reset state sets the connectors into a known state (all disabled),
+        // for the same reasons we do this on device creation.
+        //
+        // We might end up with conflicting commit requirements, if we want to restore our state,
+        // on top of the state the previous compositor left the device in.
+        // This is because we do commits per surface and not per device, so we do a global
+        // commit here, to fix any conflicts.
         if let Some(dev) = self.dev.upgrade() {
             let res_handles = ControlDevice::resource_handles(&*dev)
                 .compat()
@@ -172,6 +181,11 @@ impl<A: AsRawFd + 'static> AtomicDrmDeviceObserver<A> {
                     source,
                 })?;
 
+            // because we change the state and disabled everything,
+            // we want to force a commit (instead of a page-flip) on all used surfaces
+            // for the next rendering step to re-activate the connectors.
+            //
+            // Lets do that, by creating a garbage/non-matching current-state.
             if let Some(backends) = self.backends.upgrade() {
                 for surface in backends.borrow().values().filter_map(Weak::upgrade) {
                     let mut current = surface.state.write().unwrap();
@@ -187,7 +201,7 @@ impl<A: AsRawFd + 'static> AtomicDrmDeviceObserver<A> {
                     };
                     surface.use_mode(mode)?;
 
-                    // drop cursor state
+                    // drop cursor state to force setting the cursor again.
                     surface.cursor.position.set(None);
                     surface.cursor.hotspot.set((0, 0));
                     surface.cursor.framebuffer.set(None);

src/backend/drm/atomic/surface.rs

@@ -111,6 +111,10 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
                 source,
             })?;
 
+        // the current set of connectors are those, that already have the correct `CRTC_ID` set.
+        // so we collect them for `current_state` and set the user-given once in `pending_state`.
+        //
+        // If they don't match, `commit_pending` will return true and they will be changed on the next `commit`.
         let mut current_connectors = HashSet::new();
         for conn in res_handles.connectors() {
             let crtc_prop = dev
@@ -152,6 +156,9 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
             connectors: connectors.iter().copied().collect(),
         };
 
+        // we need to find planes for this crtc.
+        // (cursor and primary planes are usually available once for every crtc,
+        //  so this is a very naive algorithm.)
         let (primary, cursor) =
             AtomicDrmSurfaceInternal::find_planes(&dev, crtc).ok_or(Error::NoSuitablePlanes {
                 crtc,
@@ -175,6 +182,8 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
         Ok(surface)
     }
 
+    // we need a framebuffer to do test commits, which we use to verify our pending state.
+    // here we create a dumbbuffer for that purpose.
     fn create_test_buffer(&self, mode: &Mode) -> Result<framebuffer::Handle, Error> {
         let (w, h) = mode.size();
         let db = self
@@ -367,6 +376,7 @@ impl<A: AsRawFd + 'static> Surface for AtomicDrmSurfaceInternal<A> {
     }
 
     fn set_connectors(&self, connectors: &[connector::Handle]) -> Result<(), Error> {
+        // the test would also prevent this, but the error message is far less helpful
         if connectors.is_empty() {
             return Err(Error::SurfaceWithoutConnectors(self.crtc));
         }
@@ -466,6 +476,7 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
             "Preparing Commit.\n\tCurrent: {:?}\n\tPending: {:?}\n", *current, *pending
         );
 
+        // we need the differences to know, which connectors need to change properties
         let current_conns = current.connectors.clone();
         let pending_conns = pending.connectors.clone();
         let mut removed = current_conns.difference(&pending_conns);
@@ -493,6 +504,7 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
 
         trace!(self.logger, "Testing screen config");
 
+        // test the new config and return the request if it would be accepted by the driver.
         let req = {
             let req = self.build_request(
                 &mut added,
@@ -533,8 +545,11 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
         let result = self
             .atomic_commit(
                 &[
+                    // on the atomic api we can modeset and trigger a page_flip event on the same call!
                     AtomicCommitFlags::PageFlipEvent,
                     AtomicCommitFlags::AllowModeset,
+                    // we also do not need to wait for completion, like with `set_crtc`.
+                    // and have tested this already, so we do not expect any errors later down the line.
                     AtomicCommitFlags::Nonblock,
                 ],
                 req,
@@ -558,6 +573,7 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
             return Err(Error::DeviceInactive);
         }
 
+        // page flips work just like commits with fewer parameters..
         let req = self.build_request(
             &mut [].iter(),
             &mut [].iter(),
@@ -567,6 +583,9 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
             None,
         )?;
 
+        // .. and without `AtomicCommitFlags::AllowModeset`.
+        // If we would set anything here, that would require a modeset, this would fail,
+        // indicating a problem in our assumptions.
         trace!(self.logger, "Queueing page flip: {:?}", req);
         self.atomic_commit(
             &[AtomicCommitFlags::PageFlipEvent, AtomicCommitFlags::Nonblock],
@@ -583,6 +602,7 @@ impl<A: AsRawFd + 'static> RawSurface for AtomicDrmSurfaceInternal<A> {
     }
 }
 
+// this whole implementation just queues the cursor state for the next commit.
 impl<A: AsRawFd + 'static> CursorBackend for AtomicDrmSurfaceInternal<A> {
     type CursorFormat = dyn Buffer;
     type Error = Error;
@@ -708,8 +728,13 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
         mode: Option<Mode>,
         blob: Option<property::Value<'static>>,
     ) -> Result<AtomicModeReq, Error> {
+        // okay, here we build the actual requests used by the surface.
         let mut req = AtomicModeReq::new();
 
+        // requests consist out of a set of properties and their new values
+        // for different drm objects (crtc, plane, connector, ...).
+
+        // for every connector that is new, we need to set our crtc_id
         for conn in new_connectors {
             req.add_property(
                 *conn,
@@ -718,6 +743,10 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
             );
         }
 
+        // for every connector that got removed, we need to set no crtc_id.
+        // (this is a bit problematic, because this means we need to remove, commit, add, commit
+        // in the right order to move a connector to another surface. otherwise we disable the
+        // the connector here again...)
         for conn in removed_connectors {
             req.add_property(
                 *conn,
@@ -726,16 +755,19 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
             );
         }
 
+        // we need to set the new mode, if there is one
         if let Some(blob) = blob {
             req.add_property(self.crtc, self.crtc_prop_handle(self.crtc, "MODE_ID")?, blob);
         }
 
+        // we also need to set this crtc active
         req.add_property(
             self.crtc,
             self.crtc_prop_handle(self.crtc, "ACTIVE")?,
             property::Value::Boolean(true),
         );
 
+        // and we need to set the framebuffer for our primary plane
         if let Some(fb) = framebuffer {
             req.add_property(
                 planes.primary,
@@ -744,12 +776,14 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
             );
         }
 
+        // if there is a new mode, we shoudl also make sure the plane is connected
         if let Some(mode) = mode {
             req.add_property(
                 planes.primary,
                 self.plane_prop_handle(planes.primary, "CRTC_ID")?,
                 property::Value::CRTC(Some(self.crtc)),
             );
+            // we can take different parts of a plane ...
             req.add_property(
                 planes.primary,
                 self.plane_prop_handle(planes.primary, "SRC_X")?,
@@ -763,6 +797,7 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
             req.add_property(
                 planes.primary,
                 self.plane_prop_handle(planes.primary, "SRC_W")?,
+                // these are 16.16. fixed point
                 property::Value::UnsignedRange((mode.size().0 as u64) << 16),
             );
             req.add_property(
@@ -770,6 +805,7 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
                 self.plane_prop_handle(planes.primary, "SRC_H")?,
                 property::Value::UnsignedRange((mode.size().1 as u64) << 16),
             );
+            // .. onto different coordinated on the crtc, but we just use a 1:1 mapping.
             req.add_property(
                 planes.primary,
                 self.plane_prop_handle(planes.primary, "CRTC_X")?,
@@ -795,6 +831,8 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
         let cursor_pos = self.cursor.position.get();
         let cursor_fb = self.cursor.framebuffer.get();
 
+        // if there is a cursor, we add the cursor plane to the request as well.
+        // this synchronizes cursor movement with rendering, which reduces flickering.
         if let (Some(pos), Some(fb)) = (cursor_pos, cursor_fb) {
             match self.get_framebuffer(fb).compat().map_err(|source| Error::Access {
                 errmsg: "Error getting cursor fb",
@@ -804,11 +842,13 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
                 Ok(cursor_info) => {
                     let hotspot = self.cursor.hotspot.get();
 
+                    // again like the primary plane we need to set crtc and framebuffer.
                     req.add_property(
                         planes.cursor,
                         self.plane_prop_handle(planes.cursor, "CRTC_ID")?,
                         property::Value::CRTC(Some(self.crtc)),
                     );
+                    // copy the whole plane
                     req.add_property(
                         planes.cursor,
                         self.plane_prop_handle(planes.cursor, "SRC_X")?,
@@ -829,6 +869,7 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
                         self.plane_prop_handle(planes.cursor, "SRC_H")?,
                         property::Value::UnsignedRange((cursor_info.size().1 as u64) << 16),
                     );
+                    // but this time add this at some very specific coordinates of the crtc
                     req.add_property(
                         planes.cursor,
                         self.plane_prop_handle(planes.cursor, "CRTC_X")?,
@@ -865,6 +906,9 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
         Ok(req)
     }
 
+    // primary and cursor planes are almost always unique to a crtc.
+    // otherwise we would be in trouble and would need to figure this out
+    // on the device level to find the best plane combination.
     fn find_planes(card: &Dev<A>, crtc: crtc::Handle) -> Option<(plane::Handle, plane::Handle)> {
         let res = card.resource_handles().expect("Could not list resources");
         let planes = card.plane_handles().expect("Could not list planes");
@@ -918,6 +962,10 @@ impl<A: AsRawFd + 'static> AtomicDrmSurfaceInternal<A> {
         ))
     }
 
+    // this helper function clears the contents of a single plane.
+    // this is mostly used to remove the cursor, e.g. on tty switch,
+    // as other compositors might not make use of other planes,
+    // leaving our cursor as a relict of a better time on the screen.
     pub(super) fn clear_plane(&self, plane: plane::Handle) -> Result<(), Error> {
         let mut req = AtomicModeReq::new();