Address some review comments

examples/raw_drm.rs

@@ -104,7 +104,7 @@ fn main() {
     );
     let first_buffer: Slot<DumbBuffer<FdWrapper>, _> = swapchain.acquire().unwrap().unwrap();
     let framebuffer = surface.add_framebuffer(first_buffer.handle(), 32, 32).unwrap();
-    first_buffer.set_userdata(framebuffer);
+    *first_buffer.userdata() = Some(framebuffer);
 
     // Get the device as an allocator into the
     device.set_handler(DrmHandlerImpl {
@@ -141,7 +141,7 @@ impl DeviceHandler for DrmHandlerImpl {
             let next = self.swapchain.acquire().unwrap().unwrap();
             if next.userdata().is_none() {
                 let fb = self.surface.add_framebuffer(next.handle(), 32, 32).unwrap();
-                next.set_userdata(fb);
+                *next.userdata() = Some(fb);
             }
 
             // now we could render to the mapping via software rendering.

src/backend/allocator/dmabuf.rs

@@ -1,4 +1,14 @@
 //! Module for [dmabuf](https://01.org/linuxgraphics/gfx-docs/drm/driver-api/dma-buf.html) buffers.
+//! 
+//! `Dmabuf`s act alike to smart pointers and can be freely cloned and passed around.
+//! Once the last `Dmabuf` reference is dropped, its file descriptor is closed and
+//! underlying resources are freed.
+//! 
+//! If you want to hold on to a potentially alive dmabuf without blocking the free up
+//! of the underlying resouces, you may `downgrade` a `Dmabuf` reference to a `WeakDmabuf`.
+//! 
+//! This can be especially useful in resources where other parts of the stack should decide upon
+//! the lifetime of the buffer. E.g. when you are only caching associated resources for a dmabuf.
 
 use super::{Buffer, Format, Modifier};
 use std::os::unix::io::RawFd;
@@ -65,6 +75,9 @@ impl Buffer for Dmabuf {
 }
 
 impl Dmabuf {
+    // Note: the `src` Buffer is only used a reference for size and format.
+    // The contents are determined by the provided file descriptors, which
+    // do not need to refer to the same buffer `src` does.
     pub(crate) fn new(
         src: &impl Buffer,
         planes: usize,

src/backend/allocator/gbm.rs

@@ -1,10 +1,14 @@
-//! Module for Buffers created using [libgbm](reexports::gbm)
+//! Module for Buffers created using [libgbm](reexports::gbm).
+//! 
+//! The re-exported `GbmDevice` implements the [`Allocator`](super::Allocator) trait
+//! and `GbmBuffer` satisfies the [`Buffer`](super::Buffer) trait while also allowing
+//! conversions to and from [dmabufs](super::dmabuf).
 
 use super::{
     dmabuf::{AsDmabuf, Dmabuf},
     Allocator, Buffer, Format, Fourcc, Modifier,
 };
-use gbm::{BufferObject as GbmBuffer, BufferObjectFlags, Device as GbmDevice};
+pub use gbm::{BufferObject as GbmBuffer, BufferObjectFlags as GbmBufferFlags, Device as GbmDevice};
 use std::os::unix::io::AsRawFd;
 
 impl<A: AsRawFd + 'static, T> Allocator<GbmBuffer<T>> for GbmDevice<A> {
@@ -12,9 +16,9 @@ impl<A: AsRawFd + 'static, T> Allocator<GbmBuffer<T>> for GbmDevice<A> {
 
     fn create_buffer(&mut self, width: u32, height: u32, format: Format) -> std::io::Result<GbmBuffer<T>> {
         if format.modifier == Modifier::Invalid || format.modifier == Modifier::Linear {
-            let mut usage = BufferObjectFlags::SCANOUT | BufferObjectFlags::RENDERING;
+            let mut usage = GbmBufferFlags::SCANOUT | GbmBufferFlags::RENDERING;
             if format.modifier == Modifier::Linear {
-                usage |= BufferObjectFlags::LINEAR;
+                usage |= GbmBufferFlags::LINEAR;
             }
             self.create_buffer_object(width, height, format.code, usage)
         } else {
@@ -104,10 +108,10 @@ impl<T> AsDmabuf for GbmBuffer<T> {
 
 impl Dmabuf {
     /// Import a Dmabuf using libgbm, creating a gbm Buffer Object to the same underlying data.
-    pub fn import<A: AsRawFd + 'static, T>(
+    pub fn import_to<A: AsRawFd + 'static, T>(
         &self,
         gbm: &GbmDevice<A>,
-        usage: BufferObjectFlags,
+        usage: GbmBufferFlags,
     ) -> std::io::Result<GbmBuffer<T>> {
         let buf = &*self.0;
         if self.has_modifier() || buf.num_planes > 1 || buf.offsets[0] != 0 {
@@ -130,7 +134,7 @@ impl Dmabuf {
                 buf.strides[0],
                 buf.format.code,
                 if buf.format.modifier == Modifier::Linear {
-                    usage | BufferObjectFlags::LINEAR
+                    usage | GbmBufferFlags::LINEAR
                 } else {
                     usage
                 },

src/backend/allocator/swapchain.rs

@@ -32,10 +32,9 @@ pub const SLOT_CAP: usize = 4;
 /// You then hold on to the returned buffer during rendering and swapping and free it once it is displayed.
 /// Efficient re-use of the buffers is done by the swapchain.
 ///
-/// If you have associated resources for each buffer, that can also be re-used (e.g. framebuffer Handles for a `DrmDevice`),
+/// If you have associated resources for each buffer that can be reused (e.g. framebuffer `Handle`s for a `DrmDevice`),
-/// you can store then in the buffer slots userdata, where it gets freed, if the buffer gets allocated, but
+/// you can store then in the `Slot`s userdata field. If a buffer is re-used, its userdata is preserved for the next time
-/// is still valid, if the buffer was just re-used. So instead of creating a framebuffer handle for each new
+/// it is returned by `acquire()`.
-/// buffer, you can skip creation, if the userdata already contains a framebuffer handle.
 pub struct Swapchain<A: Allocator<B>, B: Buffer, U: 'static> {
     /// Allocator used by the swapchain
     pub allocator: A,
@@ -44,7 +43,7 @@ pub struct Swapchain<A: Allocator<B>, B: Buffer, U: 'static> {
     height: u32,
     format: Format,
 
-    slots: [Slot<B, U>; SLOT_CAP],
+    slots: [Arc<InternalSlot<B, U>>; SLOT_CAP],
 }
 
 /// Slot of a swapchain containing an allocated buffer and its userdata.
@@ -52,45 +51,27 @@ pub struct Swapchain<A: Allocator<B>, B: Buffer, U: 'static> {
 /// Can be cloned and passed around freely, the buffer is marked for re-use
 /// once all copies are dropped. Holding on to this struct will block the
 /// buffer in the swapchain.
-pub struct Slot<B: Buffer, U: 'static> {
+pub struct Slot<B: Buffer, U: 'static>(Arc<InternalSlot<B, U>>);
-    buffer: Arc<Option<B>>,
+
-    acquired: Arc<AtomicBool>,
+struct InternalSlot<B: Buffer, U: 'static> {
-    userdata: Arc<Mutex<Option<U>>>,
+    buffer: Option<B>,
+    acquired: AtomicBool,
+    userdata: Mutex<Option<U>>,
 }
 
 impl<B: Buffer, U: 'static> Slot<B, U> {
-    /// Set userdata for this slot.
-    pub fn set_userdata(&self, data: U) -> Option<U> {
-        self.userdata.lock().unwrap().replace(data)
-    }
-
     /// Retrieve userdata for this slot.
     pub fn userdata(&self) -> MutexGuard<'_, Option<U>> {
-        self.userdata.lock().unwrap()
+        self.0.userdata.lock().unwrap()
-    }
-
-    /// Clear userdata contained in this slot.
-    pub fn clear_userdata(&self) -> Option<U> {
-        self.userdata.lock().unwrap().take()
     }
 }
 
-impl<B: Buffer, U: 'static> Default for Slot<B, U> {
+impl<B: Buffer, U: 'static> Default for InternalSlot<B, U> {
     fn default() -> Self {
-        Slot {
+        InternalSlot {
-            buffer: Arc::new(None),
+            buffer: None,
-            acquired: Arc::new(AtomicBool::new(false)),
+            acquired: AtomicBool::new(false),
-            userdata: Arc::new(Mutex::new(None)),
+            userdata: Mutex::new(None),
-        }
-    }
-}
-
-impl<B: Buffer, U: 'static> Clone for Slot<B, U> {
-    fn clone(&self) -> Self {
-        Slot {
-            buffer: self.buffer.clone(),
-            acquired: self.acquired.clone(),
-            userdata: self.userdata.clone(),
         }
     }
 }
@@ -98,13 +79,13 @@ impl<B: Buffer, U: 'static> Clone for Slot<B, U> {
 impl<B: Buffer, U: 'static> Deref for Slot<B, U> {
     type Target = B;
     fn deref(&self) -> &B {
-        Option::as_ref(&*self.buffer).unwrap()
+        Option::as_ref(&self.0.buffer).unwrap()
     }
 }
 
 impl<B: Buffer, U: 'static> Drop for Slot<B, U> {
     fn drop(&mut self) {
-        self.acquired.store(false, Ordering::SeqCst);
+        self.0.acquired.store(false, Ordering::SeqCst);
     }
 }
 
@@ -130,19 +111,17 @@ where
     /// The swapchain has an internal maximum of four re-usable buffers.
     /// This function returns the first free one.
     pub fn acquire(&mut self) -> Result<Option<Slot<B, U>>, A::Error> {
-        if let Some(free_slot) = self.slots.iter_mut().find(|s| !s.acquired.load(Ordering::SeqCst)) {
+        if let Some(free_slot) = self.slots.iter_mut().find(|s| !s.acquired.swap(true, Ordering::SeqCst)) {
             if free_slot.buffer.is_none() {
-                free_slot.buffer = Arc::new(Some(self.allocator.create_buffer(
+                let mut free_slot = Arc::get_mut(free_slot).expect("Acquired was false, but Arc is not unique?");
+                free_slot.buffer = Some(self.allocator.create_buffer(
                     self.width,
                     self.height,
                     self.format,
-                )?));
+                )?);
             }
             assert!(free_slot.buffer.is_some());
-
+            return Ok(Some(Slot(free_slot.clone())));
-            if !free_slot.acquired.swap(true, Ordering::SeqCst) {
-                return Ok(Some(free_slot.clone()));
-            }
         }
 
         // no free slots

src/backend/drm/mod.rs

@@ -85,7 +85,7 @@ pub struct Planes {
     /// The cursor plane of the crtc, if available
     pub cursor: Option<plane::Handle>,
     /// Overlay planes supported by the crtc, if available
-    pub overlay: Option<Vec<plane::Handle>>,
+    pub overlay: Vec<plane::Handle>,
 }
 
 fn planes(dev: &impl ControlDevice, crtc: &crtc::Handle, has_universal_planes: bool) -> Result<Planes, DrmError> {
@@ -134,10 +134,10 @@ fn planes(dev: &impl ControlDevice, crtc: &crtc::Handle, has_universal_planes: b
         } else {
             None
         },
-        overlay: if has_universal_planes && !overlay.is_empty() {
+        overlay: if has_universal_planes {
-            Some(overlay)
+            overlay
         } else {
-            None
+            Vec::new()
         },
     })
 }

src/backend/drm/render.rs

@@ -192,7 +192,7 @@ where
 
         let bo = import_dmabuf(&drm, &gbm, &dmabuf)?;
         let fb = bo.userdata().unwrap().unwrap().fb;
-        buffer.set_userdata((dmabuf, bo));
+        *buffer.userdata() = Some((dmabuf, bo));
 
         match drm.test_buffer(fb, &mode, true) {
             Ok(_) => {
@@ -470,7 +470,7 @@ where
     {
         if slot.userdata().is_none() {
             let bo = import_dmabuf(&self.drm, &self.gbm, &dmabuf)?;
-            slot.set_userdata((dmabuf, bo));
+            *slot.userdata() = Some((dmabuf, bo));
         }
 
         self.queued_fb = Some(slot);
@@ -540,7 +540,7 @@ where
     E3: std::error::Error + 'static,
 {
     // TODO check userdata and return early
-    let mut bo = buffer.import(&gbm, BufferObjectFlags::SCANOUT)?;
+    let mut bo = buffer.import_to(&gbm, BufferObjectFlags::SCANOUT)?;
     let modifier = match bo.modifier().unwrap() {
         Modifier::Invalid => None,
         x => Some(x),