From 2843c5c374d85bd35d95228287175d4518ea261f Mon Sep 17 00:00:00 2001 From: Victor Brekenfeld Date: Sun, 23 May 2021 15:13:04 +0200 Subject: [PATCH] Address some review comments --- examples/raw_drm.rs | 4 +- src/backend/allocator/dmabuf.rs | 13 ++++++ src/backend/allocator/gbm.rs | 18 ++++---- src/backend/allocator/swapchain.rs | 67 ++++++++++-------------------- src/backend/drm/mod.rs | 8 ++-- src/backend/drm/render.rs | 6 +-- 6 files changed, 56 insertions(+), 60 deletions(-) diff --git a/examples/raw_drm.rs b/examples/raw_drm.rs index e47ff1f..77d653f 100644 --- a/examples/raw_drm.rs +++ b/examples/raw_drm.rs @@ -104,7 +104,7 @@ fn main() { ); let first_buffer: Slot, _> = 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. diff --git a/src/backend/allocator/dmabuf.rs b/src/backend/allocator/dmabuf.rs index 8d2eca6..aae4462 100644 --- a/src/backend/allocator/dmabuf.rs +++ b/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, diff --git a/src/backend/allocator/gbm.rs b/src/backend/allocator/gbm.rs index d3c65d7..1c3678b 100644 --- a/src/backend/allocator/gbm.rs +++ b/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 Allocator> for GbmDevice { @@ -12,9 +16,9 @@ impl Allocator> for GbmDevice { fn create_buffer(&mut self, width: u32, height: u32, format: Format) -> std::io::Result> { 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 AsDmabuf for GbmBuffer { impl Dmabuf { /// Import a Dmabuf using libgbm, creating a gbm Buffer Object to the same underlying data. - pub fn import( + pub fn import_to( &self, gbm: &GbmDevice, - usage: BufferObjectFlags, + usage: GbmBufferFlags, ) -> std::io::Result> { 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 }, diff --git a/src/backend/allocator/swapchain.rs b/src/backend/allocator/swapchain.rs index 8fc52b3..c990668 100644 --- a/src/backend/allocator/swapchain.rs +++ b/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`), -/// you can store then in the buffer slots userdata, where it gets freed, if the buffer gets allocated, but -/// is still valid, if the buffer was just re-used. So instead of creating a framebuffer handle for each new -/// buffer, you can skip creation, if the userdata already contains a framebuffer handle. +/// 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 `Slot`s userdata field. If a buffer is re-used, its userdata is preserved for the next time +/// it is returned by `acquire()`. pub struct Swapchain, B: Buffer, U: 'static> { /// Allocator used by the swapchain pub allocator: A, @@ -44,7 +43,7 @@ pub struct Swapchain, B: Buffer, U: 'static> { height: u32, format: Format, - slots: [Slot; SLOT_CAP], + slots: [Arc>; SLOT_CAP], } /// Slot of a swapchain containing an allocated buffer and its userdata. @@ -52,45 +51,27 @@ pub struct Swapchain, 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 { - buffer: Arc>, - acquired: Arc, - userdata: Arc>>, +pub struct Slot(Arc>); + +struct InternalSlot { + buffer: Option, + acquired: AtomicBool, + userdata: Mutex>, } impl Slot { - /// Set userdata for this slot. - pub fn set_userdata(&self, data: U) -> Option { - self.userdata.lock().unwrap().replace(data) - } - /// Retrieve userdata for this slot. pub fn userdata(&self) -> MutexGuard<'_, Option> { - self.userdata.lock().unwrap() - } - - /// Clear userdata contained in this slot. - pub fn clear_userdata(&self) -> Option { - self.userdata.lock().unwrap().take() + self.0.userdata.lock().unwrap() } } -impl Default for Slot { +impl Default for InternalSlot { fn default() -> Self { - Slot { - buffer: Arc::new(None), - acquired: Arc::new(AtomicBool::new(false)), - userdata: Arc::new(Mutex::new(None)), - } - } -} - -impl Clone for Slot { - fn clone(&self) -> Self { - Slot { - buffer: self.buffer.clone(), - acquired: self.acquired.clone(), - userdata: self.userdata.clone(), + InternalSlot { + buffer: None, + acquired: AtomicBool::new(false), + userdata: Mutex::new(None), } } } @@ -98,13 +79,13 @@ impl Clone for Slot { impl Deref for Slot { type Target = B; fn deref(&self) -> &B { - Option::as_ref(&*self.buffer).unwrap() + Option::as_ref(&self.0.buffer).unwrap() } } impl Drop for Slot { 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>, 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()); - - if !free_slot.acquired.swap(true, Ordering::SeqCst) { - return Ok(Some(free_slot.clone())); - } + return Ok(Some(Slot(free_slot.clone()))); } // no free slots diff --git a/src/backend/drm/mod.rs b/src/backend/drm/mod.rs index 3527b0f..781160a 100644 --- a/src/backend/drm/mod.rs +++ b/src/backend/drm/mod.rs @@ -85,7 +85,7 @@ pub struct Planes { /// The cursor plane of the crtc, if available pub cursor: Option, /// Overlay planes supported by the crtc, if available - pub overlay: Option>, + pub overlay: Vec, } fn planes(dev: &impl ControlDevice, crtc: &crtc::Handle, has_universal_planes: bool) -> Result { @@ -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() { - Some(overlay) + overlay: if has_universal_planes { + overlay } else { - None + Vec::new() }, }) } diff --git a/src/backend/drm/render.rs b/src/backend/drm/render.rs index 81ca82b..a7c8c0b 100644 --- a/src/backend/drm/render.rs +++ b/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),