From bf90204db6a7ca0bf0d44cadd6ab4d81df1d18b8 Mon Sep 17 00:00:00 2001 From: Julian Orth Date: Sat, 4 Nov 2023 14:06:18 +0100 Subject: [PATCH] video: always use correct modifiers --- src/backends/metal.rs | 8 ++++ src/backends/metal/video.rs | 67 +++++++++++++++++++++------ src/backends/x.rs | 21 ++++++++- src/cli/screenshot.rs | 3 +- src/drm_feedback.rs | 2 +- src/gfx_api.rs | 3 +- src/gfx_apis/gl/egl/display.rs | 13 ++++-- src/ifs/jay_compositor.rs | 1 + src/ifs/jay_screencast.rs | 43 ++++++++++++++--- src/ifs/jay_screenshot.rs | 3 ++ src/ifs/zwp_linux_buffer_params_v1.rs | 2 +- src/ifs/zwp_linux_dmabuf_v1.rs | 2 +- src/portal/ptr_gui.rs | 37 ++++++++++----- src/screenshoter.rs | 20 +++++++- src/video/drm.rs | 7 +-- src/video/gbm.rs | 17 +++++-- wire/jay_screenshot.txt | 2 + 17 files changed, 196 insertions(+), 55 deletions(-) diff --git a/src/backends/metal.rs b/src/backends/metal.rs index f827087c..b3c3a212 100644 --- a/src/backends/metal.rs +++ b/src/backends/metal.rs @@ -108,6 +108,14 @@ pub enum MetalError { DevicePauseSignalHandler(#[source] DbusError), #[error("Could not create device-resumed signal handler")] DeviceResumeSignalHandler(#[source] DbusError), + #[error("Device render context does not support required format {0}")] + MissingDevFormat(&'static str), + #[error("Render context does not support required format {0}")] + MissingRenderFormat(&'static str), + #[error("Device cannot scan out any buffers writable by its GFX API (format {0})")] + MissingDevModifier(&'static str), + #[error("Device GFX API cannot read any buffers writable by the render GFX API (format {0})")] + MissingRenderModifier(&'static str), } pub struct MetalBackend { diff --git a/src/backends/metal/video.rs b/src/backends/metal/video.rs index b068596b..08c62445 100644 --- a/src/backends/metal/video.rs +++ b/src/backends/metal/video.rs @@ -28,11 +28,12 @@ use { DRM_MODE_ATOMIC_NONBLOCK, DRM_MODE_PAGE_FLIP_EVENT, }, gbm::{GbmDevice, GBM_BO_USE_LINEAR, GBM_BO_USE_RENDERING, GBM_BO_USE_SCANOUT}, - Modifier, + Modifier, INVALID_MODIFIER, }, }, ahash::{AHashMap, AHashSet}, bstr::{BString, ByteSlice}, + indexmap::{indexset, IndexSet}, std::{ cell::{Cell, RefCell}, ffi::CString, @@ -508,7 +509,7 @@ pub enum PlaneType { #[derive(Debug)] pub struct PlaneFormat { _format: &'static Format, - modifiers: Vec, + modifiers: IndexSet, } #[derive(Debug)] @@ -785,7 +786,7 @@ fn create_plane(plane: DrmPlane, master: &Rc) -> Result, format: &Format, - modifiers: &[Modifier], + plane_modifiers: &IndexSet, width: i32, height: i32, ctx: &MetalRenderContext, cursor: bool, ) -> Result<[RenderBuffer; 2], MetalError> { let create = - || self.create_scanout_buffer(dev, format, modifiers, width, height, ctx, cursor); + || self.create_scanout_buffer(dev, format, plane_modifiers, width, height, ctx, cursor); Ok([create()?, create()?]) } @@ -1608,17 +1609,35 @@ impl MetalBackend { &self, dev: &Rc, format: &Format, - modifiers: &[Modifier], + plane_modifiers: &IndexSet, width: i32, height: i32, render_ctx: &MetalRenderContext, cursor: bool, ) -> Result { + let dev_gfx_formats = dev.ctx.gfx.formats(); + let dev_gfx_format = match dev_gfx_formats.get(&format.drm) { + None => return Err(MetalError::MissingDevFormat(format.name)), + Some(f) => f, + }; + let possible_modifiers: Vec<_> = dev_gfx_format + .write_modifiers + .iter() + .filter(|m| plane_modifiers.contains(*m)) + .copied() + .collect(); + if possible_modifiers.is_empty() { + log::warn!("Scanout modifiers: {:?}", plane_modifiers); + log::warn!("DEV GFX modifiers: {:?}", dev_gfx_format.write_modifiers); + return Err(MetalError::MissingDevModifier(format.name)); + } let mut usage = GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT; - if cursor && modifiers.is_empty() { + if cursor { usage |= GBM_BO_USE_LINEAR; }; - let dev_bo = dev.gbm.create_bo(width, height, format, modifiers, usage); + let dev_bo = dev + .gbm + .create_bo(width, height, format, &possible_modifiers, usage); let dev_bo = match dev_bo { Ok(b) => b, Err(e) => return Err(MetalError::ScanoutBuffer(e)), @@ -1644,11 +1663,31 @@ impl MetalBackend { (None, render_tex, None) } else { // Create a _bridge_ BO in the render device + let render_gfx_formats = render_ctx.gfx.formats(); + let render_gfx_format = match render_gfx_formats.get(&format.drm) { + None => return Err(MetalError::MissingRenderFormat(format.name)), + Some(f) => f, + }; + let possible_modifiers: Vec<_> = render_gfx_format + .write_modifiers + .iter() + .filter(|m| dev_gfx_format.read_modifiers.contains(*m)) + .copied() + .collect(); + if possible_modifiers.is_empty() { + log::warn!( + "Render GFX modifiers: {:?}", + render_gfx_format.write_modifiers + ); + log::warn!("DEV GFX modifiers: {:?}", dev_gfx_format.read_modifiers); + return Err(MetalError::MissingRenderModifier(format.name)); + } usage = GBM_BO_USE_RENDERING | GBM_BO_USE_LINEAR; - let render_bo = render_ctx - .gfx - .gbm() - .create_bo(width, height, format, &[], usage); + let render_bo = + render_ctx + .gfx + .gbm() + .create_bo(width, height, format, &possible_modifiers, usage); let render_bo = match render_bo { Ok(b) => b, Err(e) => return Err(MetalError::ScanoutBuffer(e)), @@ -1765,7 +1804,7 @@ impl MetalBackend { false, )?); let mut cursor_plane = None; - let mut cursor_modifiers = &[][..]; + let mut cursor_modifiers = &IndexSet::new(); for plane in crtc.possible_planes.values() { if plane.ty == PlaneType::Cursor && !plane.assigned.get() @@ -1773,7 +1812,7 @@ impl MetalBackend { { if let Some(format) = plane.formats.get(&ARGB8888.drm) { cursor_plane = Some(plane.clone()); - cursor_modifiers = &format.modifiers[..]; + cursor_modifiers = &format.modifiers; break; } } diff --git a/src/backends/x.rs b/src/backends/x.rs index 7d12aee3..eefde986 100644 --- a/src/backends/x.rs +++ b/src/backends/x.rs @@ -20,7 +20,8 @@ use { }, video::{ drm::{ConnectorType, Drm, DrmError, DrmVersion}, - gbm::{GbmDevice, GbmError, GBM_BO_USE_RENDERING}, + gbm::{GbmDevice, GbmError, GBM_BO_USE_LINEAR, GBM_BO_USE_RENDERING}, + INVALID_MODIFIER, LINEAR_MODIFIER, }, wire_xcon::{ ChangeProperty, ChangeWindowAttributes, ConfigureNotify, CreateCursor, CreatePixmap, @@ -117,6 +118,8 @@ pub enum XBackendError { QueryDevice(#[source] XconError), #[error("Could not fstat the drm device")] DrmDeviceFstat(#[source] Errno), + #[error("Render device does not support XRGB8888 format")] + XRGB8888, } pub async fn create(state: &Rc) -> Result, XBackendError> { @@ -376,10 +379,24 @@ impl XBackend { height: i32, ) -> Result<[XImage; 2], XBackendError> { let mut images = [None, None]; + let formats = self.ctx.formats(); + let format = match formats.get(&XRGB8888.drm) { + Some(f) => f, + None => return Err(XBackendError::XRGB8888), + }; + let mut usage = GBM_BO_USE_RENDERING; + let modifier = if format.write_modifiers.contains(&LINEAR_MODIFIER) { + &[LINEAR_MODIFIER] + } else if format.write_modifiers.contains(&INVALID_MODIFIER) { + usage |= GBM_BO_USE_LINEAR; + &[INVALID_MODIFIER] + } else { + panic!("Neither linear nor invalid modifier is supported"); + }; for image in &mut images { let bo = self .gbm - .create_bo(width, height, XRGB8888, &[], GBM_BO_USE_RENDERING)?; + .create_bo(width, height, XRGB8888, modifier, usage)?; let dma = bo.dmabuf(); assert!(dma.planes.len() == 1); let plane = dma.planes.first().unwrap(); diff --git a/src/cli/screenshot.rs b/src/cli/screenshot.rs index e775c4c0..91222eba 100644 --- a/src/cli/screenshot.rs +++ b/src/cli/screenshot.rs @@ -8,7 +8,6 @@ use { dmabuf::{DmaBuf, DmaBufPlane, PlaneVec}, drm::Drm, gbm::{GbmDevice, GBM_BO_USE_LINEAR, GBM_BO_USE_RENDERING}, - INVALID_MODIFIER, }, wire::{ jay_compositor::TakeScreenshot, @@ -91,7 +90,7 @@ pub fn buf_to_qoi(buf: &Dmabuf) -> Vec { width: buf.width as _, height: buf.height as _, format: XRGB8888, - modifier: INVALID_MODIFIER, + modifier: (buf.modifier_hi as u64) << 32 | (buf.modifier_lo as u64), planes, }; let bo = match gbm.import_dmabuf(&dmabuf, GBM_BO_USE_LINEAR | GBM_BO_USE_RENDERING) { diff --git a/src/drm_feedback.rs b/src/drm_feedback.rs index 7eb8bdb3..381ab2c0 100644 --- a/src/drm_feedback.rs +++ b/src/drm_feedback.rs @@ -42,7 +42,7 @@ impl DrmFeedback { fn create_fd_data(ctx: &dyn GfxContext) -> Vec { let mut vec = vec![]; for (format, info) in &*ctx.formats() { - for modifier in &info.modifiers { + for modifier in &info.read_modifiers { vec.write_u32::(*format).unwrap(); vec.write_u32::(0).unwrap(); vec.write_u64::(*modifier).unwrap(); diff --git a/src/gfx_api.rs b/src/gfx_api.rs index 2de46e62..9681ea6e 100644 --- a/src/gfx_api.rs +++ b/src/gfx_api.rs @@ -301,7 +301,8 @@ pub trait GfxContext: Debug { #[derive(Debug)] pub struct GfxFormat { pub format: &'static Format, - pub modifiers: IndexSet, + pub read_modifiers: IndexSet, + pub write_modifiers: IndexSet, } #[derive(Error)] diff --git a/src/gfx_apis/gl/egl/display.rs b/src/gfx_apis/gl/egl/display.rs index 04d01ced..d4353658 100644 --- a/src/gfx_apis/gl/egl/display.rs +++ b/src/gfx_apis/gl/egl/display.rs @@ -151,19 +151,24 @@ impl EglDisplay { if format.implicit_external_only && !supports_external_only { continue; } - let mut modifiers = IndexSet::new(); + let mut read_modifiers = IndexSet::new(); + let mut write_modifiers = IndexSet::new(); for modifier in format.modifiers.values() { if modifier.external_only && !supports_external_only { continue; } - modifiers.insert(modifier.modifier); + if !modifier.external_only { + write_modifiers.insert(modifier.modifier); + } + read_modifiers.insert(modifier.modifier); } - if !modifiers.is_empty() { + if !read_modifiers.is_empty() || !write_modifiers.is_empty() { formats.insert( drm, GfxFormat { format: format.format, - modifiers, + read_modifiers, + write_modifiers, }, ); } diff --git a/src/ifs/jay_compositor.rs b/src/ifs/jay_compositor.rs index 47da44bf..321bbecf 100644 --- a/src/ifs/jay_compositor.rs +++ b/src/ifs/jay_compositor.rs @@ -143,6 +143,7 @@ impl JayCompositor { dmabuf.height, plane.offset, plane.stride, + dmabuf.modifier, ); } Err(e) => { diff --git a/src/ifs/jay_screencast.rs b/src/ifs/jay_screencast.rs index 492c22c2..2964de25 100644 --- a/src/ifs/jay_screencast.rs +++ b/src/ifs/jay_screencast.rs @@ -17,10 +17,13 @@ use { video::{ dmabuf::DmaBuf, gbm::{GbmError, GBM_BO_USE_LINEAR, GBM_BO_USE_RENDERING}, + Modifier, INVALID_MODIFIER, LINEAR_MODIFIER, }, wire::{jay_screencast::*, JayScreencastId}, }, ahash::AHashSet, + indexmap::{indexset, IndexSet}, + once_cell::sync::Lazy, std::{ cell::{Cell, RefCell}, ops::{Deref, DerefMut}, @@ -194,17 +197,37 @@ impl JayScreencast { pub fn realloc(&self, ctx: &Rc) -> Result<(), JayScreencastError> { let mut buffers = vec![]; + let formats = ctx.formats(); + let format = match formats.get(&XRGB8888.drm) { + Some(f) => f, + _ => return Err(JayScreencastError::XRGB8888), + }; if let Some(output) = self.output.get() { let mode = output.global.mode.get(); let num = 3; for _ in 0..num { - let mut flags = GBM_BO_USE_RENDERING; - if self.linear.get() { - flags |= GBM_BO_USE_LINEAR; - } - let buffer = ctx - .gbm() - .create_bo(mode.width, mode.height, XRGB8888, &[], flags)?; + let mut usage = GBM_BO_USE_RENDERING; + let modifiers = match self.linear.get() { + true if format.write_modifiers.contains(&LINEAR_MODIFIER) => { + static MODS: Lazy> = + Lazy::new(|| indexset![LINEAR_MODIFIER]); + &MODS + } + true if format.write_modifiers.contains(&INVALID_MODIFIER) => { + usage |= GBM_BO_USE_LINEAR; + static MODS: Lazy> = + Lazy::new(|| indexset![INVALID_MODIFIER]); + &MODS + } + true => return Err(JayScreencastError::Modifier), + false if format.write_modifiers.is_empty() => { + return Err(JayScreencastError::XRGB8888Writing) + } + false => &format.write_modifiers, + }; + let buffer = + ctx.gbm() + .create_bo(mode.width, mode.height, XRGB8888, modifiers, usage)?; let fb = ctx.clone().dmabuf_img(buffer.dmabuf())?.to_framebuffer()?; buffers.push(ScreencastBuffer { dmabuf: buffer.dmabuf().clone(), @@ -438,6 +461,12 @@ pub enum JayScreencastError { GbmError(#[from] GbmError), #[error(transparent)] GfxError(#[from] GfxError), + #[error("Render context does not support XRGB8888 format")] + XRGB8888, + #[error("Render context does not support XRGB8888 format for rendering")] + XRGB8888Writing, + #[error("Render context supports neither linear or invalid modifier")] + Modifier, } efrom!(JayScreencastError, MsgParserError); efrom!(JayScreencastError, ClientError); diff --git a/src/ifs/jay_screenshot.rs b/src/ifs/jay_screenshot.rs index 6ef3c7fc..aff61452 100644 --- a/src/ifs/jay_screenshot.rs +++ b/src/ifs/jay_screenshot.rs @@ -24,6 +24,7 @@ impl JayScreenshot { height: i32, offset: u32, stride: u32, + modifier: u64, ) { self.client.event(Dmabuf { self_id: self.id, @@ -33,6 +34,8 @@ impl JayScreenshot { height: height as _, offset, stride, + modifier_lo: modifier as u32, + modifier_hi: (modifier >> 32) as u32, }); } diff --git a/src/ifs/zwp_linux_buffer_params_v1.rs b/src/ifs/zwp_linux_buffer_params_v1.rs index 35aecf09..fbfb38e5 100644 --- a/src/ifs/zwp_linux_buffer_params_v1.rs +++ b/src/ifs/zwp_linux_buffer_params_v1.rs @@ -110,7 +110,7 @@ impl ZwpLinuxBufferParamsV1 { Some(m) => m, _ => return Err(ZwpLinuxBufferParamsV1Error::NoPlanes), }; - if !format.modifiers.contains(&modifier) { + if !format.read_modifiers.contains(&modifier) { return Err(ZwpLinuxBufferParamsV1Error::InvalidModifier(modifier)); } let mut dmabuf = DmaBuf { diff --git a/src/ifs/zwp_linux_dmabuf_v1.rs b/src/ifs/zwp_linux_dmabuf_v1.rs index f76d4954..5c8370a4 100644 --- a/src/ifs/zwp_linux_dmabuf_v1.rs +++ b/src/ifs/zwp_linux_dmabuf_v1.rs @@ -44,7 +44,7 @@ impl ZwpLinuxDmabufV1Global { for format in formats.values() { obj.send_format(format.format.drm); if version >= MODIFIERS_SINCE_VERSION { - for &modifier in &format.modifiers { + for &modifier in &format.read_modifiers { obj.send_modifier(format.format.drm, modifier); } } diff --git a/src/portal/ptr_gui.rs b/src/portal/ptr_gui.rs index c46f7294..6a232818 100644 --- a/src/portal/ptr_gui.rs +++ b/src/portal/ptr_gui.rs @@ -693,19 +693,32 @@ impl WindowData { self.frame_missed.set(true); let width = (self.width.get() as f64 * self.scale.get().to_f64()).round() as i32; let height = (self.height.get() as f64 * self.scale.get().to_f64()).round() as i32; + let formats = ctx.ctx.formats(); + let format = match formats.get(&ARGB8888.drm) { + None => { + log::error!("Render context does not support ARGB8888 format"); + return; + } + Some(f) => f, + }; + if format.write_modifiers.is_empty() { + log::error!("Render context cannot render to ARGB8888 format"); + return; + } for _ in 0..NUM_BUFFERS { - let bo = - match ctx - .ctx - .gbm() - .create_bo(width, height, ARGB8888, &[], GBM_BO_USE_RENDERING) - { - Ok(b) => b, - Err(e) => { - log::error!("Could not allocate dmabuf: {}", ErrorFmt(e)); - return; - } - }; + let bo = match ctx.ctx.gbm().create_bo( + width, + height, + ARGB8888, + &format.write_modifiers, + GBM_BO_USE_RENDERING, + ) { + Ok(b) => b, + Err(e) => { + log::error!("Could not allocate dmabuf: {}", ErrorFmt(e)); + return; + } + }; let img = match ctx.ctx.clone().dmabuf_img(bo.dmabuf()) { Ok(b) => b, Err(e) => { diff --git a/src/screenshoter.rs b/src/screenshoter.rs index 25327861..4732abfe 100644 --- a/src/screenshoter.rs +++ b/src/screenshoter.rs @@ -7,6 +7,7 @@ use { video::{ drm::DrmError, gbm::{GbmBo, GbmError, GBM_BO_USE_LINEAR, GBM_BO_USE_RENDERING}, + INVALID_MODIFIER, LINEAR_MODIFIER, }, }, std::{ops::Deref, rc::Rc}, @@ -26,6 +27,10 @@ pub enum ScreenshooterError { RenderError(#[from] GfxError), #[error(transparent)] DrmError(#[from] DrmError), + #[error("Render context does not support XRGB8888")] + XRGB8888, + #[error("Render context supports neither linear nor invalid modifier for XRGB8888 rendering")] + Linear, } pub struct Screenshot { @@ -42,13 +47,24 @@ pub fn take_screenshot(state: &State) -> Result if extents.is_empty() { return Err(ScreenshooterError::EmptyDisplay); } + let formats = ctx.formats(); + let mut usage = GBM_BO_USE_RENDERING; + let modifiers = match formats.get(&XRGB8888.drm) { + None => return Err(ScreenshooterError::XRGB8888), + Some(f) if f.write_modifiers.contains(&LINEAR_MODIFIER) => &[LINEAR_MODIFIER], + Some(f) if f.write_modifiers.contains(&INVALID_MODIFIER) => { + usage |= GBM_BO_USE_LINEAR; + &[INVALID_MODIFIER] + } + Some(_) => return Err(ScreenshooterError::Linear), + }; let gbm = ctx.gbm(); let bo = gbm.create_bo( extents.width(), extents.height(), XRGB8888, - &[], - GBM_BO_USE_RENDERING | GBM_BO_USE_LINEAR, + modifiers, + usage, )?; let fb = ctx.clone().dmabuf_fb(bo.dmabuf())?; fb.render_node( diff --git a/src/video/drm.rs b/src/video/drm.rs index 4968fa52..ca53b2d7 100644 --- a/src/video/drm.rs +++ b/src/video/drm.rs @@ -17,6 +17,7 @@ use { }, ahash::AHashMap, bstr::{BString, ByteSlice}, + indexmap::IndexSet, std::{ cell::RefCell, ffi::CString, @@ -181,7 +182,7 @@ impl Drm { pub struct InFormat { pub format: u32, - pub modifiers: Vec, + pub modifiers: IndexSet, } pub struct DrmMaster { @@ -418,7 +419,7 @@ impl DrmMaster { .unwrap() .map(|f| InFormat { format: f, - modifiers: vec![], + modifiers: IndexSet::new(), }) .collect(); let modifiers = @@ -435,7 +436,7 @@ impl DrmMaster { log::error!("Modifier offset is out of bounds"); return Err(DrmError::InFormats); } - formats[idx].modifiers.push(modifier.modifier); + formats[idx].modifiers.insert(modifier.modifier); } } Ok(formats) diff --git a/src/video/gbm.rs b/src/video/gbm.rs index c0f5fc1a..904f9330 100644 --- a/src/video/gbm.rs +++ b/src/video/gbm.rs @@ -7,7 +7,7 @@ use { video::{ dmabuf::{DmaBuf, DmaBufPlane, PlaneVec}, drm::{Drm, DrmError}, - Modifier, + Modifier, INVALID_MODIFIER, }, }, std::{ @@ -34,6 +34,8 @@ pub enum GbmError { DrmFd, #[error("Could not map bo")] MapBo(#[source] OsError), + #[error("Tried to allocate a buffer with no modifier")] + NoModifier, } pub type Device = u8; @@ -195,18 +197,23 @@ impl GbmDevice { self.dev } - pub fn create_bo( + pub fn create_bo<'a>( &self, width: i32, height: i32, format: &Format, - modifiers: &[Modifier], - usage: u32, + modifiers: impl IntoIterator, + mut usage: u32, ) -> Result { unsafe { - let (modifiers, n_modifiers) = if modifiers.is_empty() { + let modifiers: Vec = modifiers.into_iter().copied().collect(); + if modifiers.is_empty() { + return Err(GbmError::NoModifier); + } + let (modifiers, n_modifiers) = if modifiers == [INVALID_MODIFIER] { (ptr::null(), 0) } else { + usage &= !GBM_BO_USE_LINEAR; (modifiers.as_ptr() as _, modifiers.len() as _) }; let bo = gbm_bo_create_with_modifiers2( diff --git a/wire/jay_screenshot.txt b/wire/jay_screenshot.txt index 726e9d53..41cc34b8 100644 --- a/wire/jay_screenshot.txt +++ b/wire/jay_screenshot.txt @@ -7,6 +7,8 @@ msg dmabuf = 0 { height: u32, offset: u32, stride: u32, + modifier_lo: u32, + modifier_hi: u32, } msg error = 1 {