From 378686611c2ec4646b49c329304bdaf104f8892a Mon Sep 17 00:00:00 2001 From: Victor Brekenfeld Date: Sun, 26 Apr 2020 20:47:52 +0200 Subject: [PATCH] drm: Let swap_buffers return real error --- src/backend/drm/atomic/surface.rs | 17 ++++++++++------- src/backend/drm/common/fallback.rs | 2 +- src/backend/drm/gbm/egl.rs | 9 ++++++++- src/backend/drm/gbm/mod.rs | 9 +++++++++ src/backend/drm/gbm/surface.rs | 22 ++++++++++------------ src/backend/drm/legacy/surface.rs | 15 +++++++++------ src/backend/drm/mod.rs | 4 +--- 7 files changed, 48 insertions(+), 30 deletions(-) diff --git a/src/backend/drm/atomic/surface.rs b/src/backend/drm/atomic/surface.rs index 842e04f..8d2ba57 100644 --- a/src/backend/drm/atomic/surface.rs +++ b/src/backend/drm/atomic/surface.rs @@ -15,7 +15,6 @@ use failure::ResultExt as FailureResultExt; use super::Dev; use crate::backend::drm::{common::Error, DevPath, RawSurface, Surface}; use crate::backend::graphics::CursorBackend; -use crate::backend::graphics::SwapBuffersError; #[derive(Debug, Clone, PartialEq, Eq)] pub struct CursorState { @@ -508,9 +507,9 @@ impl RawSurface for AtomicDrmSurfaceInternal { Ok(()) } - fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), SwapBuffersError> { + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), Error> { if !self.dev.active.load(Ordering::SeqCst) { - return Err(SwapBuffersError::AlreadySwapped); + return Err(Error::DeviceInactive); } let req = self @@ -521,14 +520,18 @@ impl RawSurface for AtomicDrmSurfaceInternal { Some(framebuffer), None, None, - ) //current.mode) - .map_err(|_| SwapBuffersError::ContextLost)?; + )?; + trace!(self.logger, "Queueing page flip: {:#?}", req); self.atomic_commit( &[AtomicCommitFlags::PageFlipEvent, AtomicCommitFlags::Nonblock], req, ) - .map_err(|_| SwapBuffersError::ContextLost)?; + .compat().map_err(|source| Error::Access { + errmsg: "Page flip commit failed", + dev: self.dev_path(), + source + })?; Ok(()) } @@ -971,7 +974,7 @@ impl RawSurface for AtomicDrmSurface { self.0.commit(framebuffer) } - fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), SwapBuffersError> { + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), Error> { RawSurface::page_flip(&*self.0, framebuffer) } } diff --git a/src/backend/drm/common/fallback.rs b/src/backend/drm/common/fallback.rs index 3051351..13c08ae 100644 --- a/src/backend/drm/common/fallback.rs +++ b/src/backend/drm/common/fallback.rs @@ -311,7 +311,7 @@ where { fallback_surface_impl!(commit_pending, &Self, bool); fallback_surface_impl!(commit, &Self, Result<(), E>, fb: framebuffer::Handle); - fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), SwapBuffersError> { + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), E> { match self { FallbackSurface::Preference(dev) => RawSurface::page_flip(dev, framebuffer), FallbackSurface::Fallback(dev) => RawSurface::page_flip(dev, framebuffer), diff --git a/src/backend/drm/gbm/egl.rs b/src/backend/drm/gbm/egl.rs index 719642f..085a1fd 100644 --- a/src/backend/drm/gbm/egl.rs +++ b/src/backend/drm/gbm/egl.rs @@ -89,6 +89,13 @@ unsafe impl NativeSurface for GbmSurface { fn swap_buffers(&self) -> ::std::result::Result<(), SwapBuffersError> { // this is safe since `eglSwapBuffers` will have been called exactly once // if this is used by our egl module, which is why this trait is unsafe. - unsafe { self.page_flip() } + match unsafe { self.page_flip() } { + Ok(()) => Ok(()), + Err(Error::FrontBuffersExhausted) => Err(SwapBuffersError::AlreadySwapped), + Err(err) => { + warn!(self.0.logger, "Page flip failed: {}", err); + Err(SwapBuffersError::Unknown(0)) + } + } } } diff --git a/src/backend/drm/gbm/mod.rs b/src/backend/drm/gbm/mod.rs index 91861a9..a2d75b4 100644 --- a/src/backend/drm/gbm/mod.rs +++ b/src/backend/drm/gbm/mod.rs @@ -39,9 +39,18 @@ pub enum Error), /// Lock of GBM surface front buffer failed #[error("Lock of GBM surface font buffer failed")] FrontBufferLockFailed, + /// No additional buffers are available + #[error("No additional buffers are available. Did you swap twice?")] + FrontBuffersExhausted, + /// Internal state was modified + #[error("Internal state was modified. Did you change gbm userdata?")] + InvalidInternalState, /// The GBM device was destroyed #[error("The GBM device was destroyed")] DeviceDestroyed, diff --git a/src/backend/drm/gbm/surface.rs b/src/backend/drm/gbm/surface.rs index bf5ef9b..e4ec54b 100644 --- a/src/backend/drm/gbm/surface.rs +++ b/src/backend/drm/gbm/surface.rs @@ -4,12 +4,12 @@ use super::Error; use drm::control::{connector, crtc, framebuffer, Device as ControlDevice, Mode}; use gbm::{self, BufferObject, BufferObjectFlags, Format as GbmFormat, SurfaceBufferHandle}; use image::{ImageBuffer, Rgba}; +use failure::ResultExt; use std::cell::{Cell, RefCell}; use std::rc::Rc; use crate::backend::graphics::CursorBackend; -use crate::backend::graphics::SwapBuffersError; pub(super) struct GbmSurfaceInternal { pub(super) dev: Rc>>, @@ -32,7 +32,7 @@ impl GbmSurfaceInternal { // drop and release the old buffer } - pub unsafe fn page_flip(&self) -> ::std::result::Result<(), SwapBuffersError> { + pub unsafe fn page_flip(&self) -> Result<(), Error<<::Surface as Surface>::Error>> { let res = { let nb = self.next_buffer.take(); let res = nb.is_some(); @@ -42,7 +42,7 @@ impl GbmSurfaceInternal { if res { // We cannot call lock_front_buffer anymore without releasing the previous buffer, which will happen when the page flip is done warn!(self.logger, "Tried to swap with an already queued flip"); - return Err(SwapBuffersError::AlreadySwapped); + return Err(Error::FrontBuffersExhausted); } // supporting only one buffer would cause a lot of inconvinience and @@ -53,13 +53,13 @@ impl GbmSurfaceInternal { .surface .borrow() .lock_front_buffer() - .expect("Surface only has one front buffer. Not supported by smithay"); + .map_err(|_| Error::FrontBufferLockFailed)?; // create a framebuffer if the front buffer does not have one already // (they are reused by gbm) let maybe_fb = next_bo .userdata() - .map_err(|_| SwapBuffersError::ContextLost)? + .map_err(|_| Error::InvalidInternalState)? .cloned(); let fb = if let Some(info) = maybe_fb { info @@ -67,7 +67,8 @@ impl GbmSurfaceInternal { let fb = self .crtc .add_planar_framebuffer(&*next_bo, &[0; 4], 0) - .map_err(|_| SwapBuffersError::ContextLost)?; + .compat() + .map_err(Error::FramebufferCreationFailed)?; next_bo.set_userdata(fb).unwrap(); fb }; @@ -75,14 +76,11 @@ impl GbmSurfaceInternal { if self.recreated.get() { debug!(self.logger, "Commiting new state"); - if let Err(err) = self.crtc.commit(fb) { - error!(self.logger, "Error commiting crtc: {}", err); - return Err(SwapBuffersError::ContextLost); - } + self.crtc.commit(fb).map_err(Error::Underlying)?; self.recreated.set(false); } else { trace!(self.logger, "Queueing Page flip"); - RawSurface::page_flip(&self.crtc, fb)?; + RawSurface::page_flip(&self.crtc, fb).map_err(Error::Underlying)?; } self.current_frame_buffer.set(Some(fb)); @@ -275,7 +273,7 @@ impl GbmSurface { /// /// When used in conjunction with an EGL context, this must be called exactly once /// after page-flipping the associated context. - pub unsafe fn page_flip(&self) -> ::std::result::Result<(), SwapBuffersError> { + pub unsafe fn page_flip(&self) -> ::std::result::Result<(), ::Error> { self.0.page_flip() } diff --git a/src/backend/drm/legacy/surface.rs b/src/backend/drm/legacy/surface.rs index 47ff30f..8121733 100644 --- a/src/backend/drm/legacy/surface.rs +++ b/src/backend/drm/legacy/surface.rs @@ -12,7 +12,6 @@ use std::sync::{RwLock, atomic::Ordering}; use crate::backend::drm::{common::Error, DevPath, RawSurface, Surface}; use crate::backend::graphics::CursorBackend; -use crate::backend::graphics::SwapBuffersError; use super::Dev; @@ -285,11 +284,11 @@ impl RawSurface for LegacyDrmSurfaceInternal { }) } - fn page_flip(&self, framebuffer: framebuffer::Handle) -> ::std::result::Result<(), SwapBuffersError> { + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), Error> { trace!(self.logger, "Queueing Page flip"); if !self.dev.active.load(Ordering::SeqCst) { - return Err(SwapBuffersError::AlreadySwapped); + return Err(Error::DeviceInactive); } ControlDevice::page_flip( @@ -298,8 +297,12 @@ impl RawSurface for LegacyDrmSurfaceInternal { framebuffer, &[PageFlipFlags::PageFlipEvent], None, - ) - .map_err(|_| SwapBuffersError::ContextLost) + ).compat() + .map_err(|source| Error::Access { + errmsg: "Failed to page flip", + dev: self.dev_path(), + source, + }) } } @@ -511,7 +514,7 @@ impl RawSurface for LegacyDrmSurface { self.0.commit(framebuffer) } - fn page_flip(&self, framebuffer: framebuffer::Handle) -> ::std::result::Result<(), SwapBuffersError> { + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), Error> { RawSurface::page_flip(&*self.0, framebuffer) } } diff --git a/src/backend/drm/mod.rs b/src/backend/drm/mod.rs index 534ab9d..1eb3e76 100644 --- a/src/backend/drm/mod.rs +++ b/src/backend/drm/mod.rs @@ -51,8 +51,6 @@ use calloop::mio::Interest; use calloop::InsertError; use calloop::{LoopHandle, Source}; -use super::graphics::SwapBuffersError; - #[cfg(feature = "backend_drm_atomic")] pub mod atomic; #[cfg(feature = "backend_drm")] @@ -233,7 +231,7 @@ pub trait RawSurface: Surface + ControlDevice + BasicDevice { /// This operation is not blocking and will produce a `vblank` event once swapping is done. /// Make sure to [set a `DeviceHandler`](Device::set_handler) and /// [register the belonging `Device`](device_bind) before to receive the event in time. - fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), SwapBuffersError>; + fn page_flip(&self, framebuffer: framebuffer::Handle) -> Result<(), ::Error>; } /// Trait representing open devices that *may* return a `Path`