allocator: Let the allocator select the best modifier out of a given set.

This change fixes modifier selection by delegating this to the allocators and thus to libgbm, which can ask the driver for an appropriate modifier for scanout, that results in the best possible performance. We do not have this information, the order in which modifiers are returned by EGL has no meaning and this is far better then testing modifiers non-deterministically at random and choosing the first one, that does not error out...
2021-05-29 21:09:38 +02:00 · 2021-05-29 21:09:38 +02:00 · e008360bde
parent 4f0161658f
commit e008360bde
6 changed files with 123 additions and 95 deletions
--- a/examples/raw_drm.rs
+++ b/examples/raw_drm.rs
@ -6,7 +6,7 @@ extern crate slog;
 use slog::Drain;
 use smithay::{
    backend::{
-        allocator::{dumb::DumbBuffer, Format, Fourcc, Modifier, Slot, Swapchain},
+        allocator::{dumb::DumbBuffer, Fourcc, Slot, Swapchain},
        drm::{device_bind, DeviceHandler, DrmDevice, DrmError, DrmSurface},
    },
    reexports::{
@ -93,14 +93,18 @@ fn main() {
     */
    let (w, h) = mode.size();
    let allocator = DrmDevice::new(fd, false, log.clone()).unwrap();
+    let mods = surface
+        .supported_formats(surface.plane())
+        .expect("Unable to readout formats for surface")
+        .iter()
+        .filter_map(|format| if format.code == Fourcc::Argb8888 { Some(format.modifier) } else { None })
+        .collect::<Vec<_>>();
    let mut swapchain = Swapchain::new(
        allocator,
        w.into(),
        h.into(),
-        Format {
-            code: Fourcc::Argb8888,
-            modifier: Modifier::Invalid,
-        },
+        Fourcc::Argb8888,
+        mods,
    );
    let first_buffer: Slot<DumbBuffer<FdWrapper>, _> = swapchain.acquire().unwrap().unwrap();
    let framebuffer = surface.add_framebuffer(first_buffer.handle(), 32, 32).unwrap();
--- a/src/backend/allocator/dumb.rs
+++ b/src/backend/allocator/dumb.rs
@ -7,7 +7,7 @@ use std::sync::Arc;
 use drm::buffer::Buffer as DrmBuffer;
 use drm::control::{dumbbuffer::DumbBuffer as Handle, Device as ControlDevice};

-use super::{Allocator, Buffer, Format};
+use super::{Allocator, Buffer, Format, Fourcc, Modifier};
 use crate::backend::drm::device::{DrmDevice, DrmDeviceInternal, FdWrapper};

 /// Wrapper around raw DumbBuffer handles.
@ -33,9 +33,19 @@ impl<A: AsRawFd + 'static> Allocator<DumbBuffer<A>> for DrmDevice<A> {
        &mut self,
        width: u32,
        height: u32,
-        format: Format,
+        fourcc: Fourcc,
+        modifiers: &[Modifier],
    ) -> Result<DumbBuffer<A>, Self::Error> {
-        let handle = self.create_dumb_buffer((width, height), format.code, 32 /* TODO */)?;
+        // dumb buffers are always linear
+        if modifiers
+            .iter()
+            .find(|x| **x == Modifier::Invalid || **x == Modifier::Linear)
+            .is_none()
+        {
+            return Err(drm::SystemError::InvalidArgument);
+        }
+
+        let handle = self.create_dumb_buffer((width, height), fourcc, 32 /* TODO */)?;

        Ok(DumbBuffer {
            fd: match &*self.internal {
@ -43,7 +53,10 @@ impl<A: AsRawFd + 'static> Allocator<DumbBuffer<A>> for DrmDevice<A> {
                DrmDeviceInternal::Legacy(dev) => dev.fd.clone(),
            },
            handle,
-            format,
+            format: Format {
+                code: fourcc,
+                modifier: Modifier::Linear,
+            },
        })
    }
 }
--- a/src/backend/allocator/gbm.rs
+++ b/src/backend/allocator/gbm.rs
@ -14,20 +14,26 @@ use std::os::unix::io::AsRawFd;
 impl<A: AsRawFd + 'static, T> Allocator<GbmBuffer<T>> for GbmDevice<A> {
    type Error = std::io::Error;

-    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 {
+    fn create_buffer(
+        &mut self,
+        width: u32,
+        height: u32,
+        fourcc: Fourcc,
+        modifiers: &[Modifier],
+    ) -> Result<GbmBuffer<T>, Self::Error> {
+        match self.create_buffer_object_with_modifiers(width, height, fourcc, modifiers.iter().copied()) {
+            Ok(bo) => Ok(bo),
+            Err(err) => {
+                if modifiers.contains(&Modifier::Invalid) || modifiers.contains(&Modifier::Linear) {
                    let mut usage = GbmBufferFlags::SCANOUT | GbmBufferFlags::RENDERING;
-            if format.modifier == Modifier::Linear {
+                    if !modifiers.contains(&Modifier::Invalid) {
                        usage |= GbmBufferFlags::LINEAR;
                    }
-            self.create_buffer_object(width, height, format.code, usage)
+                    self.create_buffer_object(width, height, fourcc, usage)
                } else {
-            self.create_buffer_object_with_modifiers(
-                width,
-                height,
-                format.code,
-                Some(format.modifier).into_iter(),
-            )
+                    Err(err)
+                }
+            }
        }
    }
 }
--- a/src/backend/allocator/mod.rs
+++ b/src/backend/allocator/mod.rs
@ -49,5 +49,11 @@ pub trait Allocator<B: Buffer> {
    type Error: std::error::Error;

    /// Try to create a buffer with the given dimensions and pixel format
-    fn create_buffer(&mut self, width: u32, height: u32, format: Format) -> Result<B, Self::Error>;
+    fn create_buffer(
+        &mut self,
+        width: u32,
+        height: u32,
+        fourcc: Fourcc,
+        modifiers: &[Modifier],
+    ) -> Result<B, Self::Error>;
 }
--- a/src/backend/allocator/swapchain.rs
+++ b/src/backend/allocator/swapchain.rs
@ -4,7 +4,7 @@ use std::sync::{
    Arc, Mutex, MutexGuard,
 };

-use crate::backend::allocator::{Allocator, Buffer, Format};
+use crate::backend::allocator::{Allocator, Buffer, Fourcc, Modifier};

 pub const SLOT_CAP: usize = 4;

@ -41,7 +41,8 @@ pub struct Swapchain<A: Allocator<B>, B: Buffer, U: 'static> {

    width: u32,
    height: u32,
-    format: Format,
+    fourcc: Fourcc,
+    modifiers: Vec<Modifier>,

    slots: [Arc<InternalSlot<B, U>>; SLOT_CAP],
 }
@ -96,12 +97,19 @@ where
    U: 'static,
 {
    /// Create a new swapchain with the desired allocator and dimensions and pixel format for the created buffers.
-    pub fn new(allocator: A, width: u32, height: u32, format: Format) -> Swapchain<A, B, U> {
+    pub fn new(
+        allocator: A,
+        width: u32,
+        height: u32,
+        fourcc: Fourcc,
+        modifiers: Vec<Modifier>,
+    ) -> Swapchain<A, B, U> {
        Swapchain {
            allocator,
            width,
            height,
-            format,
+            fourcc,
+            modifiers,
            slots: Default::default(),
        }
    }
@ -122,7 +130,8 @@ where
                free_slot.buffer = Some(self.allocator.create_buffer(
                    self.width,
                    self.height,
-                    self.format,
+                    self.fourcc,
+                    &self.modifiers,
                )?);
            }
            assert!(free_slot.buffer.is_some());
--- a/src/backend/drm/render.rs
+++ b/src/backend/drm/render.rs
@ -22,7 +22,6 @@ use crate::backend::SwapBuffersError;
 /// of a single renderer. In these cases `DrmRenderSurface` provides a way to quickly
 /// get up and running without manually handling and binding buffers.
 pub struct DrmRenderSurface<D: AsRawFd + 'static, A: Allocator<B>, R: Bind<Dmabuf>, B: Buffer> {
-    _format: Format,
    buffers: Buffers<D, B>,
    swapchain: Swapchain<A, B, (Dmabuf, BufferObject<FbHandle<D>>)>,
    renderer: R,
@ -52,7 +51,7 @@ where
    pub fn new<L: Into<Option<::slog::Logger>>>(
        drm: DrmSurface<D>,
        allocator: A,
-        renderer: R,
+        mut renderer: R,
        log: L,
    ) -> Result<DrmRenderSurface<D, A, R, B>, Error<E1, E2, E3>> {
        // we cannot simply pick the first supported format of the intersection of *all* formats, because:
@ -123,37 +122,31 @@ where
        };
        debug!(logger, "Testing Formats: {:?}", formats);

-        // Test explicit formats first
        let drm = Arc::new(drm);
-        let iter = formats
-            .iter()
-            .filter(|x| x.modifier != Modifier::Invalid && x.modifier != Modifier::Linear)
-            .chain(formats.iter().find(|x| x.modifier == Modifier::Linear))
-            .chain(formats.iter().find(|x| x.modifier == Modifier::Invalid))
-            .cloned();
+        let modifiers = formats.iter().map(|x| x.modifier).collect::<Vec<_>>();

-        DrmRenderSurface::new_internal(drm, allocator, renderer, iter, logger)
-    }
-
-    #[allow(clippy::type_complexity)]
-    fn new_internal(
-        drm: Arc<DrmSurface<D>>,
-        allocator: A,
-        mut renderer: R,
-        mut formats: impl Iterator<Item = Format>,
-        logger: ::slog::Logger,
-    ) -> Result<DrmRenderSurface<D, A, R, B>, Error<E1, E2, E3>> {
-        let format = formats.next().ok_or(Error::NoSupportedPlaneFormat)?;
        let mode = drm.pending_mode();

        let gbm = unsafe { GbmDevice::new_from_fd(drm.as_raw_fd())? };
-        let mut swapchain = Swapchain::new(allocator, mode.size().0 as u32, mode.size().1 as u32, format);
+        let mut swapchain = Swapchain::new(
+            allocator,
+            mode.size().0 as u32,
+            mode.size().1 as u32,
+            code,
+            modifiers,
+        );

        // Test format
        let buffer = swapchain.acquire().map_err(Error::SwapchainError)?.unwrap();
        let dmabuf = buffer.export().map_err(Error::AsDmabufError)?;
+        let format = Format {
+            code,
+            modifier: buffer.format().modifier, // no guarantee
+                                                // that this is stable across allocations, but
+                                                // we want to print that here for debugging proposes.
+                                                // It has no further use.
+        };

-        {
        match renderer
            .bind(dmabuf.clone())
            .map_err(Error::<E1, E2, E3>::RenderError)
@ -169,20 +162,7 @@ where
            })
            .and_then(|_| renderer.unbind().map_err(Error::RenderError))
        {
-                Ok(_) => {}
-                Err(err) => {
-                    warn!(logger, "Rendering failed with format {:?}: {}", format, err);
-                    return DrmRenderSurface::new_internal(
-                        drm,
-                        swapchain.allocator,
-                        renderer,
-                        formats,
-                        logger,
-                    );
-                }
-            }
-        }
-
+            Ok(_) => {
                let bo = import_dmabuf(&drm, &gbm, &dmabuf)?;
                let fb = bo.userdata().unwrap().unwrap().fb;
                *buffer.userdata() = Some((dmabuf, bo));
@ -193,7 +173,6 @@ where
                        let buffers = Buffers::new(drm.clone(), gbm, buffer);
                        Ok(DrmRenderSurface {
                            drm,
-                    _format: format,
                            renderer,
                            swapchain,
                            buffers,
@ -202,9 +181,20 @@ where
                    Err(err) => {
                        warn!(
                            logger,
-                    "Mode-setting failed with buffer format {:?}: {}", format, err
+                            "Mode-setting failed with automatically selected buffer format {:?}: {}",
+                            format,
+                            err
                        );
-                DrmRenderSurface::new_internal(drm, swapchain.allocator, renderer, formats, logger)
+                        Err(err).map_err(Into::into)
+                    }
+                }
+            }
+            Err(err) => {
+                warn!(
+                    logger,
+                    "Rendering failed with automatically selected format {:?}: {}", format, err
+                );
+                Err(err)
            }
        }
    }