diff --git a/src/backend/drm/atomic/mod.rs b/src/backend/drm/atomic/mod.rs index c877956..b40bd83 100644 --- a/src/backend/drm/atomic/mod.rs +++ b/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 Drop for Dev { // 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( req: &mut AtomicModeReq, old_state: &[(T, PropertyValueSet)], @@ -215,6 +225,8 @@ impl AtomicDrmDevice { 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 AtomicDrmDevice { 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 AtomicDrmDevice { 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 AtomicDrmDevice { 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 AtomicDrmDevice { 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(); diff --git a/src/backend/drm/atomic/session.rs b/src/backend/drm/atomic/session.rs index 9d4b39e..5b0d365 100644 --- a/src/backend/drm/atomic/session.rs +++ b/src/backend/drm/atomic/session.rs @@ -75,6 +75,8 @@ impl AtomicDrmDeviceObserver { 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 AtomicDrmDeviceObserver { } 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 AtomicDrmDeviceObserver { 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 AtomicDrmDeviceObserver { }; 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); diff --git a/src/backend/drm/atomic/surface.rs b/src/backend/drm/atomic/surface.rs index 481e3d7..d1c14c6 100644 --- a/src/backend/drm/atomic/surface.rs +++ b/src/backend/drm/atomic/surface.rs @@ -111,6 +111,10 @@ impl AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 { let (w, h) = mode.size(); let db = self @@ -367,6 +376,7 @@ impl Surface for AtomicDrmSurfaceInternal { } 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 RawSurface for AtomicDrmSurfaceInternal { "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 RawSurface for AtomicDrmSurfaceInternal { 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 RawSurface for AtomicDrmSurfaceInternal { 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 RawSurface for AtomicDrmSurfaceInternal { 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 RawSurface for AtomicDrmSurfaceInternal { 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 RawSurface for AtomicDrmSurfaceInternal { } } +// this whole implementation just queues the cursor state for the next commit. impl CursorBackend for AtomicDrmSurfaceInternal { type CursorFormat = dyn Buffer; type Error = Error; @@ -708,8 +728,13 @@ impl AtomicDrmSurfaceInternal { mode: Option, blob: Option>, ) -> Result { + // 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 AtomicDrmSurfaceInternal { ); } + // 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 AtomicDrmSurfaceInternal { ); } + // 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 AtomicDrmSurfaceInternal { ); } + // 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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 AtomicDrmSurfaceInternal { 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, 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 AtomicDrmSurfaceInternal { )) } + // 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();