gbm: fix EGLSurface recreation

src/backend/drm/gbm/egl.rs

@@ -87,19 +87,20 @@ unsafe impl<D: RawDevice + 'static> NativeSurface for GbmSurface<D> {
         self.0.surface.borrow().as_raw() as *const _
     }
 
-    fn swap_buffers<F>(&self, flip: F) -> ::std::result::Result<(), SwapBuffersError>
+    fn needs_recreation(&self) -> bool {
-    where
+        self.0.crtc.commit_pending()
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
+    }
-    {
+
-        if self.0.crtc.commit_pending() || {
+    fn recreate(&self) -> bool {
-            let fb = self.0.front_buffer.take();
+        if let Err(err) = GbmSurface::recreate(self) {
-            let res = fb.is_none();
+            error!(self.0.logger, "Failure recreating internal resources: {:?}", err);
-            self.0.front_buffer.set(fb);
+            false
-            res
-        } {
-            self.recreate(flip).map_err(|_| SwapBuffersError::ContextLost)
         } else {
-            self.page_flip(flip)
+            true
         }
     }
+
+    fn swap_buffers(&self) -> ::std::result::Result<(), SwapBuffersError> {
+        self.page_flip()
+    }
 }

src/backend/drm/gbm/mod.rs

@@ -128,7 +128,7 @@ impl<D: RawDevice + ControlDevice + 'static> Device for GbmDevice<D> {
         info!(self.logger, "Initializing GbmSurface");
 
         let drm_surface = Device::create_surface(&mut **self.dev.borrow_mut(), crtc, mode, connectors)
-                .chain_err(|| ErrorKind::UnderlyingBackendError)?;
+            .chain_err(|| ErrorKind::UnderlyingBackendError)?;
 
         let (w, h) = mode.size();
         let surface = self
@@ -161,6 +161,7 @@ impl<D: RawDevice + ControlDevice + 'static> Device for GbmDevice<D> {
             current_frame_buffer: Cell::new(None),
             front_buffer: Cell::new(None),
             next_buffer: Cell::new(None),
+            recreated: Cell::new(true),
             logger: self.logger.new(o!("crtc" => format!("{:?}", crtc))),
         });
         self.backends.borrow_mut().insert(crtc, Rc::downgrade(&backend));

src/backend/drm/gbm/session.rs

@@ -45,11 +45,11 @@ impl<S: SessionObserver + 'static, D: RawDevice + ControlDevice + AsSessionObser
             for (crtc, backend) in backends.borrow().iter() {
                 if let Some(backend) = backend.upgrade() {
                     // restart rendering loop, if it was previously running
-                    if let Some(Err(err)) = backend.current_frame_buffer.get().map(|fb|
+                    if let Some(Err(err)) = backend
-                        backend
+                        .current_frame_buffer
-                            .crtc
+                        .get()
-                            .page_flip(fb.handle())
+                        .map(|fb| backend.crtc.page_flip(fb.handle()))
-                    ) {
+                    {
                         warn!(self.logger, "Failed to restart rendering loop. Error: {}", err);
                     }
                     // reset cursor

src/backend/drm/gbm/surface.rs

@@ -22,6 +22,7 @@ pub(super) struct GbmSurfaceInternal<D: RawDevice + 'static> {
     pub(super) current_frame_buffer: Cell<Option<framebuffer::Info>>,
     pub(super) front_buffer: Cell<Option<SurfaceBufferHandle<framebuffer::Info>>>,
     pub(super) next_buffer: Cell<Option<SurfaceBufferHandle<framebuffer::Info>>>,
+    pub(super) recreated: Cell<bool>,
     pub(super) logger: ::slog::Logger,
 }
 
@@ -29,17 +30,12 @@ impl<D: RawDevice + 'static> GbmSurfaceInternal<D> {
     pub(super) fn unlock_buffer(&self) {
         // after the page swap is finished we need to release the rendered buffer.
         // this is called from the PageFlipHandler
-        if let Some(next_buffer) = self.next_buffer.replace(None) {
+        trace!(self.logger, "Releasing old front buffer");
-            trace!(self.logger, "Releasing old front buffer");
+        self.front_buffer.set(self.next_buffer.replace(None));
-            self.front_buffer.set(Some(next_buffer));
+        // drop and release the old buffer
-            // drop and release the old buffer
-        }
     }
 
-    pub fn page_flip<F>(&self, flip: F) -> ::std::result::Result<(), SwapBuffersError>
+    pub fn page_flip(&self) -> ::std::result::Result<(), SwapBuffersError> {
-    where
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
-    {
         let res = {
             let nb = self.next_buffer.take();
             let res = nb.is_some();
@@ -52,9 +48,6 @@ impl<D: RawDevice + 'static> GbmSurfaceInternal<D> {
             return Err(SwapBuffersError::AlreadySwapped);
         }
 
-        // flip normally
-        flip()?;
-
         // supporting only one buffer would cause a lot of inconvinience and
         // would most likely result in a lot of flickering.
         // neither weston, wlc or wlroots bother with that as well.
@@ -81,17 +74,21 @@ impl<D: RawDevice + 'static> GbmSurfaceInternal<D> {
         self.next_buffer.set(Some(next_bo));
 
         trace!(self.logger, "Queueing Page flip");
-        self.crtc.page_flip(fb.handle())?;
+        if self.recreated.get() {
+            self.crtc
+                .commit(fb.handle())
+                .map_err(|_| SwapBuffersError::ContextLost)?;
+            self.recreated.set(false);
+        } else {
+            self.crtc.page_flip(fb.handle())?;
+        }
 
         self.current_frame_buffer.set(Some(fb));
 
         Ok(())
     }
 
-    pub fn recreate<F>(&self, flip: F) -> Result<()>
+    pub fn recreate(&self) -> Result<()> {
-    where
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
-    {
         let (w, h) = self.pending_mode().size();
 
         // Recreate the surface and the related resources to match the new
@@ -107,44 +104,17 @@ impl<D: RawDevice + 'static> GbmSurfaceInternal<D> {
                 BufferObjectFlags::SCANOUT | BufferObjectFlags::RENDERING,
             ).chain_err(|| ErrorKind::SurfaceCreationFailed)?;
 
-        // Clean up next_buffer
+        // Clean up buffers
-        {
+        if let Some(Ok(Some(fb))) = self.next_buffer.take().map(|mut bo| bo.take_userdata()) {
-            if let Some(mut old_bo) = self.next_buffer.take() {
+            if let Err(err) = framebuffer::destroy(&self.crtc, fb.handle()) {
-                if let Ok(Some(fb)) = old_bo.take_userdata() {
+                warn!(
-                    if let Err(err) = framebuffer::destroy(&self.crtc, fb.handle()) {
+                    self.logger,
-                        warn!(
+                    "Error releasing old back_buffer framebuffer: {:?}", err
-                            self.logger,
+                );
-                            "Error releasing old back_buffer framebuffer: {:?}", err
-                        );
-                    }
-                }
             }
         }
 
-        flip()?;
+        if let Some(Ok(Some(fb))) = self.front_buffer.take().map(|mut bo| bo.take_userdata()) {
-        
-        // Cleanup front_buffer and init the first screen on the new front_buffer
-        // (must be done before calling page_flip for the first time)
-        let old_front_bo = self.front_buffer.replace({
-            let mut front_bo = surface
-                .lock_front_buffer()
-                .chain_err(|| ErrorKind::FrontBufferLockFailed)?;
-
-            debug!(self.logger, "FrontBuffer color format: {:?}", front_bo.format());
-
-            // we also need a new framebuffer for the front buffer
-            let fb = framebuffer::create(&self.crtc, &*front_bo)
-                .chain_err(|| ErrorKind::UnderlyingBackendError)?;
-
-            self.crtc
-                .commit(fb.handle())
-                .chain_err(|| ErrorKind::UnderlyingBackendError)?;
-
-            self.current_frame_buffer.set(Some(fb));
-            front_bo.set_userdata(fb).unwrap();
-            Some(front_bo)
-        });
-        if let Some(Ok(Some(fb))) = old_front_bo.map(|mut bo| bo.take_userdata()) {
             if let Err(err) = framebuffer::destroy(&self.crtc, fb.handle()) {
                 warn!(
                     self.logger,
@@ -156,6 +126,8 @@ impl<D: RawDevice + 'static> GbmSurfaceInternal<D> {
         // Drop the old surface after cleanup
         *self.surface.borrow_mut() = surface;
 
+        self.recreated.set(true);
+
         Ok(())
     }
 }
@@ -308,18 +280,12 @@ impl<D: RawDevice + 'static> Drop for GbmSurfaceInternal<D> {
 pub struct GbmSurface<D: RawDevice + 'static>(pub(super) Rc<GbmSurfaceInternal<D>>);
 
 impl<D: RawDevice + 'static> GbmSurface<D> {
-    pub fn page_flip<F>(&self, flip: F) -> ::std::result::Result<(), SwapBuffersError>
+    pub fn page_flip(&self) -> ::std::result::Result<(), SwapBuffersError> {
-    where
+        self.0.page_flip()
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
-    {
-        self.0.page_flip(flip)
     }
 
-    pub fn recreate<F>(&self, flip: F) -> Result<()>
+    pub fn recreate(&self) -> Result<()> {
-    where
+        self.0.recreate()
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
-    {
-        self.0.recreate(flip)
     }
 }
 

src/backend/egl/native.rs

@@ -167,14 +167,31 @@ unsafe impl NativeDisplay<Wayland> for WinitWindow {
 pub unsafe trait NativeSurface {
     /// Return a raw pointer egl will accept for surface creation.
     fn ptr(&self) -> ffi::NativeWindowType;
+
+    /// Will be called to check if any internal resources will need
+    /// to be recreated. Old resources must be used until `recreate`
+    /// was called.
+    ///
+    /// Only needs to be recreated, if this shall sometimes return true.
+    /// The default implementation always returns false.
+    fn needs_recreation(&self) -> bool {
+        false
+    }
+
+    /// Instructs the surface to recreate internal resources
+    ///
+    /// Must only be implemented if `needs_recreation` can return `true`.
+    /// Returns true on success.
+    /// If this call was successful `ptr()` *should* return something different.
+    fn recreate(&self) -> bool {
+        true
+    }
+
     /// Adds additional semantics when calling EGLSurface::swap_buffers
     ///
-    /// Only implement if required by the backend, flip must be called during this call.
+    /// Only implement if required by the backend.
-    fn swap_buffers<F>(&self, flip: F) -> ::std::result::Result<(), SwapBuffersError>
+    fn swap_buffers(&self) -> ::std::result::Result<(), SwapBuffersError> {
-    where
+        Ok(())
-        F: FnOnce() -> ::std::result::Result<(), SwapBuffersError>,
-    {
-        flip()
     }
 }
 

src/backend/egl/surface.rs

@@ -2,7 +2,9 @@
 
 use super::{error::*, ffi, native, EGLContext};
 use backend::graphics::SwapBuffersError;
+use nix::libc::c_int;
 use std::{
+    cell::Cell,
     ops::{Deref, DerefMut},
     rc::{Rc, Weak},
 };
@@ -12,7 +14,9 @@ pub struct EGLSurface<N: native::NativeSurface> {
     context: Weak<ffi::egl::types::EGLContext>,
     display: Weak<ffi::egl::types::EGLDisplay>,
     native: N,
-    surface: ffi::egl::types::EGLSurface,
+    surface: Cell<ffi::egl::types::EGLSurface>,
+    config_id: ffi::egl::types::EGLConfig,
+    surface_attributes: Vec<c_int>,
 }
 
 impl<N: native::NativeSurface> Deref for EGLSurface<N> {
@@ -50,28 +54,49 @@ impl<N: native::NativeSurface> EGLSurface<N> {
             context: Rc::downgrade(&context.context),
             display: Rc::downgrade(&context.display),
             native,
-            surface,
+            surface: Cell::new(surface),
+            config_id: context.config_id,
+            surface_attributes: context.surface_attributes.clone(),
         })
     }
 
     /// Swaps buffers at the end of a frame.
     pub fn swap_buffers(&self) -> ::std::result::Result<(), SwapBuffersError> {
-        self.native.swap_buffers(|| {
+        let surface = self.surface.get();
+
+        if !surface.is_null() {
             if let Some(display) = self.display.upgrade() {
-                let ret = unsafe { ffi::egl::SwapBuffers((*display) as *const _, self.surface as *const _) };
+                let ret = unsafe { ffi::egl::SwapBuffers((*display) as *const _, surface as *const _) };
 
                 if ret == 0 {
                     match unsafe { ffi::egl::GetError() } as u32 {
-                        ffi::egl::CONTEXT_LOST => Err(SwapBuffersError::ContextLost),
+                        ffi::egl::CONTEXT_LOST => return Err(SwapBuffersError::ContextLost),
-                        err => Err(SwapBuffersError::Unknown(err)),
+                        err => return Err(SwapBuffersError::Unknown(err)),
-                    }
+                    };
                 } else {
-                    Ok(())
+                    return Ok(());
                 }
             } else {
-                Err(SwapBuffersError::ContextLost)
+                return Err(SwapBuffersError::ContextLost);
             }
-        })
+            self.native.swap_buffers()?;
+        };
+
+        if self.native.needs_recreation() || surface.is_null() {
+            if let Some(display) = self.display.upgrade() {
+                self.native.recreate();
+                self.surface.set(unsafe {
+                    ffi::egl::CreateWindowSurface(
+                        *display,
+                        self.config_id,
+                        self.native.ptr(),
+                        self.surface_attributes.as_ptr(),
+                    )
+                });
+            }
+        }
+
+        Ok(())
     }
 
     /// Makes the OpenGL context the current context in the current thread.
@@ -84,8 +109,8 @@ impl<N: native::NativeSurface> EGLSurface<N> {
         if let (Some(display), Some(context)) = (self.display.upgrade(), self.context.upgrade()) {
             let ret = ffi::egl::MakeCurrent(
                 (*display) as *const _,
-                self.surface as *const _,
+                self.surface.get() as *const _,
-                self.surface as *const _,
+                self.surface.get() as *const _,
                 (*context) as *const _,
             );
 
@@ -106,8 +131,8 @@ impl<N: native::NativeSurface> EGLSurface<N> {
     pub fn is_current(&self) -> bool {
         if self.context.upgrade().is_some() {
             unsafe {
-                ffi::egl::GetCurrentSurface(ffi::egl::DRAW as _) == self.surface as *const _
+                ffi::egl::GetCurrentSurface(ffi::egl::DRAW as _) == self.surface.get() as *const _
-                    && ffi::egl::GetCurrentSurface(ffi::egl::READ as _) == self.surface as *const _
+                    && ffi::egl::GetCurrentSurface(ffi::egl::READ as _) == self.surface.get() as *const _
             }
         } else {
             false
@@ -119,7 +144,7 @@ impl<N: native::NativeSurface> Drop for EGLSurface<N> {
     fn drop(&mut self) {
         if let Some(display) = self.display.upgrade() {
             unsafe {
-                ffi::egl::DestroySurface((*display) as *const _, self.surface as *const _);
+                ffi::egl::DestroySurface((*display) as *const _, self.surface.get() as *const _);
             }
         }
     }