From 4c4ab2de442366ca14d313f52e36b50e0292e989 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sun, 22 Dec 2024 11:04:53 +0100 Subject: [PATCH 1/6] graphics: Implement handoff from vesad to virtio-gpud With this virtio-gpud no longer has to support getting deactivated at any time to hand back control to vesad. This makes it much easier to add many new features that a proper graphics driver is supposed to support. Be aware that virtio-gpud waits for the vsync on every flush request. Fbcond and orbital are not really happy about this right now and when something changes multiple times within a single frame, flush requests queue up, causing the ui to hang until all flush requests are finished. It is still possible to switch to another VT though which won't hang. --- graphics/fbcond/src/display.rs | 38 +++++++++++ graphics/fbcond/src/main.rs | 9 ++- graphics/fbcond/src/text.rs | 4 ++ graphics/vesad/src/scheme.rs | 2 +- graphics/virtio-gpud/src/main.rs | 15 ----- graphics/virtio-gpud/src/scheme.rs | 93 ++++---------------------- inputd/src/lib.rs | 5 ++ inputd/src/main.rs | 103 +++++++++++++++++++++++++++-- 8 files changed, 167 insertions(+), 102 deletions(-) diff --git a/graphics/fbcond/src/display.rs b/graphics/fbcond/src/display.rs index 39b615e5a9..ceaf48da44 100644 --- a/graphics/fbcond/src/display.rs +++ b/graphics/fbcond/src/display.rs @@ -60,6 +60,7 @@ impl Drop for DisplayMap { enum DisplayCommand { Resize { width: usize, height: usize }, + ReopenForHandoff { display_path: String }, SyncRects(Vec), } @@ -104,6 +105,29 @@ impl Display { } } } + DisplayCommand::ReopenForHandoff { display_path } => { + eprintln!("fbcond: Performing handoff for '{display_path}'"); + + let (mut new_display_file, width, height) = + Self::open_display(&display_path).unwrap(); + + eprintln!("fbcond: Opened new display '{display_path}'"); + + match display_fd_map(width, height, &mut new_display_file) { + Ok(ok) => { + *map_clone.lock().unwrap() = ok; + display_file = new_display_file; + + eprintln!("fbcond: Mapped new display '{display_path}'"); + } + Err(err) => { + eprintln!( + "failed to resize display to {}x{}: {}", + width, height, err + ); + } + } + } DisplayCommand::SyncRects(sync_rects) => { for sync_rect in sync_rects { unsafe { @@ -130,6 +154,20 @@ impl Display { }) } + /// Re-open the display after a handoff. + /// + /// Once re-opening is finished, you must call [`resize`] to map the new framebuffer. + /// + /// Warning: This must be called in a background thread to avoid a deadlock when the + /// graphics driver (indirectly) writes logs to fbcond. + pub fn reopen_for_handoff(&mut self) { + let display_path = Self::display_path(&mut self.input_handle).unwrap(); + + self.cmd_tx + .send(DisplayCommand::ReopenForHandoff { display_path }) + .unwrap(); + } + fn display_path(input_handle: &mut File) -> io::Result { let mut buffer = [0; 1024]; let fd = input_handle.as_raw_fd(); diff --git a/graphics/fbcond/src/main.rs b/graphics/fbcond/src/main.rs index 7449d5a4d7..f5d5b8d243 100644 --- a/graphics/fbcond/src/main.rs +++ b/graphics/fbcond/src/main.rs @@ -1,3 +1,5 @@ +#![feature(io_error_more)] + use event::EventQueue; use orbclient::Event; use std::fs::{File, OpenOptions}; @@ -55,7 +57,9 @@ fn inner(daemon: redox_daemon::Daemon, vt_ids: &[usize]) -> ! { let mut inputd_control_handle = inputd::ControlHandle::new().unwrap(); - libredox::call::setrens(0, 0).expect("fbcond: failed to enter null namespace"); + // This is not possible for now as fbcond needs to open new displays at runtime for graphics + // driver handoff. In the future inputd may directly pass a handle to the display instead. + //libredox::call::setrens(0, 0).expect("fbcond: failed to enter null namespace"); daemon.ready().expect("failed to notify parent"); @@ -127,6 +131,9 @@ fn handle_event( Err(err) if err.kind() == ErrorKind::WouldBlock => { break; } + Err(err) if err.kind() == ErrorKind::StaleNetworkFileHandle => { + vt.handle_handoff(); + } Ok(count) => { let events = &mut events[..count]; diff --git a/graphics/fbcond/src/text.rs b/graphics/fbcond/src/text.rs index 46afcc738c..d62b15f502 100644 --- a/graphics/fbcond/src/text.rs +++ b/graphics/fbcond/src/text.rs @@ -119,6 +119,10 @@ impl TextScreen { } } + pub fn handle_handoff(&mut self) { + self.display.reopen_for_handoff(); + } + pub fn resize(&mut self, width: usize, height: usize) { self.display .resize(width.try_into().unwrap(), height.try_into().unwrap()); diff --git a/graphics/vesad/src/scheme.rs b/graphics/vesad/src/scheme.rs index 3dd2b3a9ef..84dcb3ec39 100644 --- a/graphics/vesad/src/scheme.rs +++ b/graphics/vesad/src/scheme.rs @@ -34,7 +34,7 @@ pub struct DisplayScheme { impl DisplayScheme { pub fn new(framebuffers: Vec, spec: &[()]) -> DisplayScheme { - let mut inputd_handle = inputd::DisplayHandle::new("vesa").unwrap(); + let mut inputd_handle = inputd::DisplayHandle::new_early("vesa").unwrap(); let mut vts = BTreeMap::>::new(); diff --git a/graphics/virtio-gpud/src/main.rs b/graphics/virtio-gpud/src/main.rs index 8a40452427..ab36a1048e 100644 --- a/graphics/virtio-gpud/src/main.rs +++ b/graphics/virtio-gpud/src/main.rs @@ -25,14 +25,12 @@ use std::cell::UnsafeCell; use std::os::fd::AsRawFd; use std::sync::atomic::{AtomicU32, Ordering}; -use std::sync::Arc; use event::{user_data, EventQueue}; use libredox::errno::EAGAIN; use pcid_interface::PciFunctionHandle; use redox_scheme::{RequestKind, SignalBehavior, Socket, V2}; -use virtio_core::transport::{self, Queue}; use virtio_core::utils::VolatileCell; use virtio_core::MSIX_PRIMARY_VECTOR; @@ -411,19 +409,6 @@ impl XferToHost2d { static DEVICE: spin::Once = spin::Once::new(); -fn reinit(control_queue: Arc, cursor_queue: Arc) -> Result<(), transport::Error> { - let device = DEVICE.get().unwrap(); - - virtio_core::reinit(device)?; - device.transport.finalize_features(); - - device.transport.reinit_queue(control_queue.clone()); - device.transport.reinit_queue(cursor_queue.clone()); - - device.transport.run_device(); - Ok(()) -} - fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { let mut pcid_handle = PciFunctionHandle::connect_default()?; diff --git a/graphics/virtio-gpud/src/scheme.rs b/graphics/virtio-gpud/src/scheme.rs index 0d8dc80414..97c4a0f031 100644 --- a/graphics/virtio-gpud/src/scheme.rs +++ b/graphics/virtio-gpud/src/scheme.rs @@ -1,7 +1,7 @@ use std::cell::RefCell; use std::collections::{BTreeMap, HashMap}; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use common::{dma::Dma, sgl}; @@ -40,8 +40,6 @@ pub struct Display<'a> { height: u32, id: usize, - - is_reseted: AtomicBool, } impl<'a> Display<'a> { @@ -64,28 +62,9 @@ impl<'a> Display<'a> { transport, id, - - is_reseted: AtomicBool::new(false), } } - async fn init(&self, vt: usize) -> Result<(), Error> { - if !self.is_reseted.load(Ordering::SeqCst) { - // The device is already initialized. - self.set_scanout(vt).await?; - return Ok(()); - } - - self.is_reseted.store(false, Ordering::SeqCst); - - log::info!("virtio-gpu: initializing GPU after a reset"); - - crate::reinit(self.control_queue.clone(), self.cursor_queue.clone())?; - self.set_scanout(vt).await?; - - Ok(()) - } - async fn get_fpath(&self, buffer: &mut [u8]) -> Result { let path = format!("display.virtio-gpu:3.0/{}/{}", self.width, self.height); @@ -199,9 +178,12 @@ impl<'a> Display<'a> { /// If `damage` is `None`, the entire screen is flushed. async fn flush(&self, vt: usize, damage: Option<&Damage>) -> Result<(), Error> { - if vt != *self.active_vt.borrow() { + let Some(&res_id) = self.vts_res.borrow().get(&vt) else { + // The resource is not yet created. Ignore the damage. We will write the entire backing + // storage to the resource once we create the resource, which is equivalent to damaging + // the entire resource. return Ok(()); - } + }; let damage = if let Some(damage) = damage { damage.into() @@ -215,7 +197,7 @@ impl<'a> Display<'a> { }; let req = Dma::new(XferToHost2d::new( - self.vts_res.borrow()[&vt], + res_id, GpuRect { x: 0, y: 0, @@ -226,31 +208,8 @@ impl<'a> Display<'a> { let header = self.send_request(req).await?; assert_eq!(header.ty.get(), CommandTy::RespOkNodata); - self.flush_resource(ResourceFlush::new( - self.vts_res.borrow()[&vt], - damage.clone(), - )) - .await?; - Ok(()) - } - - /// This detaches any backing pages from the display and unrefs the resource. Also resets the - /// device, which is required to go back to legacy mode. - async fn detach(&self) -> Result<(), Error> { - for (_vt, res_id) in self.vts_res.borrow_mut().drain() { - let request = Dma::new(DetachBacking::new(res_id))?; - let header = self.send_request(request).await?; - assert_eq!(header.ty.get(), CommandTy::RespOkNodata); - - let request = Dma::new(ResourceUnref::new(res_id))?; - let header = self.send_request(request).await?; - assert_eq!(header.ty.get(), CommandTy::RespOkNodata); - } - - // Go back to legacy mode. - self.transport.reset(); - self.is_reseted.store(true, Ordering::SeqCst); - + self.flush_resource(ResourceFlush::new(res_id, damage.clone())) + .await?; Ok(()) } } @@ -283,9 +242,7 @@ impl<'a> Scheme<'a> { ) .await?; - let mut inputd_handle = inputd::DisplayHandle::new("virtio-gpu").unwrap(); - // FIXME make vesad handoff control over all it's VT's instead - inputd_handle.register_vt().unwrap(); + let inputd_handle = inputd::DisplayHandle::new("virtio-gpu").unwrap(); Ok(Self { handles: BTreeMap::new(), @@ -319,11 +276,6 @@ impl<'a> Scheme<'a> { transport.clone(), id, ); - // FIXME this is a hack to avoid breaking things while we need to co-exist with vesad - // Somehow necessary to ensure that creating a resource on the first reinitialization - // after this detach doesn't fail. - display.init(1).await?; - display.detach().await?; result.push(Arc::new(display)); } @@ -357,7 +309,7 @@ impl<'a> Scheme<'a> { for display in &self.displays { log::warn!("virtio-gpu: activating"); - futures::executor::block_on(display.init(vt_event.vt)).unwrap(); + futures::executor::block_on(display.set_scanout(vt_event.vt)).unwrap(); *display.active_vt.borrow_mut() = vt_event.vt; } @@ -365,21 +317,7 @@ impl<'a> Scheme<'a> { VtEventKind::Deactivate => { log::info!("deactivate {}", vt_event.vt); - - for handle in self.handles.values() { - if handle.vt != vt_event.vt { - continue; - } - - log::warn!("virtio-gpu: deactivating"); - - futures::executor::block_on(handle.display.detach()).unwrap(); - break; - } - - // for display in self.displays.iter() { - // futures::executor::block_on(display.detach()).unwrap(); - // } + // nothing to do :) } VtEventKind::Resize => { @@ -449,13 +387,6 @@ impl<'a> SchemeMut for Scheme<'a> { ) -> syscall::Result { let handle = self.handles.get(&id).ok_or(SysError::new(EINVAL))?; - // The VT is not active and the device is reseted. Ignore the damage. We will recreate the - // backing storage from scratch next time we initialize, which is equivalent to damaging the - // entire buffer. - if handle.display.is_reseted.load(Ordering::SeqCst) { - return Ok(buf.len()); - } - let damages = unsafe { core::slice::from_raw_parts( buf.as_ptr() as *const Damage, diff --git a/inputd/src/lib.rs b/inputd/src/lib.rs index 907ea53a9b..ca8b25750f 100644 --- a/inputd/src/lib.rs +++ b/inputd/src/lib.rs @@ -27,6 +27,11 @@ impl DisplayHandle { Ok(Self(File::open(path)?)) } + pub fn new_early>(device_name: S) -> Result { + let path = format!("/scheme/input/handle_early/display/{}", device_name.into()); + Ok(Self(File::open(path)?)) + } + // The return value is the display identifier. It will be used to uniquely // identify the display on activation events. pub fn register_vt(&mut self) -> Result { diff --git a/inputd/src/main.rs b/inputd/src/main.rs index 569e1d80e4..a79ba703df 100644 --- a/inputd/src/main.rs +++ b/inputd/src/main.rs @@ -18,6 +18,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use inputd::{VtActivate, VtEvent, VtEventKind}; +use libredox::errno::ESTALE; use redox_scheme::{RequestKind, SchemeMut, SignalBehavior, Socket, V2}; use orbclient::{Event, EventOption}; @@ -28,6 +29,9 @@ enum Handle { Consumer { events: EventFlags, pending: Vec, + /// We return an ESTALE error once to indicate that a handoff to a different graphics driver + /// is necessary. + needs_handoff: bool, notified: bool, vt: usize, }, @@ -36,6 +40,8 @@ enum Handle { pending: Vec, notified: bool, device: String, + /// Control of all VT's gets handed over from earlyfb devices to the first non-earlyfb device. + is_earlyfb: bool, }, Control, } @@ -46,6 +52,7 @@ impl Handle { } } +#[derive(Debug)] struct Vt { display: String, } @@ -69,6 +76,7 @@ struct InputScheme { active_vt: Option, has_new_events: bool, + maybe_perform_handoff_to: Option, } impl InputScheme { @@ -83,6 +91,7 @@ impl InputScheme { active_vt: None, has_new_events: false, + maybe_perform_handoff_to: None, } } @@ -163,17 +172,40 @@ impl SchemeMut for InputScheme { Handle::Consumer { events: EventFlags::empty(), pending: Vec::new(), + needs_handoff: false, notified: false, vt: target, } } - "handle" => { + "handle_early" => { let display = path_parts.collect::>().join("."); Handle::Display { events: EventFlags::empty(), pending: Vec::new(), notified: false, device: display, + is_earlyfb: true, + } + } + "handle" => { + let display = path_parts.collect::>().join("."); + self.maybe_perform_handoff_to = Some(display.clone()); + Handle::Display { + events: EventFlags::empty(), + pending: if let Some(active_vt) = self.active_vt { + vec![VtEvent { + kind: VtEventKind::Activate, + vt: active_vt, + width: 0, + height: 0, + stride: 0, + }] + } else { + vec![] + }, + notified: false, + device: display, + is_earlyfb: false, } } "control" => Handle::Control, @@ -216,7 +248,17 @@ impl SchemeMut for InputScheme { let handle = self.handles.get_mut(&id).ok_or(SysError::new(EINVAL))?; match handle { - Handle::Consumer { pending, .. } => { + Handle::Consumer { + pending, + needs_handoff, + .. + } => { + if *needs_handoff { + *needs_handoff = false; + // Indicates that handoff to a new graphics driver is necessary. + return Err(SysError::new(ESTALE)); + } + let copy = core::cmp::min(pending.len(), buf.len()); for (i, byte) in pending.drain(..copy).enumerate() { @@ -464,6 +506,55 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { RequestKind::MsyncMsg | RequestKind::MunmapMsg | RequestKind::MmapMsg => unreachable!(), } + if let Some(display) = scheme.maybe_perform_handoff_to.take() { + let early_displays = scheme + .handles + .values() + .filter_map(|handle| match handle { + Handle::Display { + device, + is_earlyfb: true, + .. + } => Some(&**device), + _ => None, + }) + .collect::>(); + let vts = scheme + .vts + .iter_mut() + .filter_map(|(&i, vt)| { + if early_displays.contains(&&*vt.display) { + vt.display = display.clone(); + + scheme.has_new_events = true; + + Some(i) + } else { + None + } + }) + .collect::>(); + + for handle in scheme.handles.values_mut() { + match handle { + Handle::Consumer { + needs_handoff, + notified, + vt, + .. + } => { + if !vts.contains(vt) { + continue; + } + + *needs_handoff = true; + *notified = false; + } + _ => continue, + } + } + } + if !scheme.has_new_events { continue; } @@ -473,10 +564,14 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { Handle::Consumer { events, pending, + needs_handoff, ref mut notified, vt, } => { - if pending.is_empty() || *notified || !events.contains(EventFlags::EVENT_READ) { + if (!*needs_handoff && pending.is_empty()) + || *notified + || !events.contains(EventFlags::EVENT_READ) + { continue; } @@ -484,7 +579,7 @@ fn deamon(deamon: redox_daemon::Daemon) -> anyhow::Result<()> { // The activate VT is not the same as the VT that the consumer is listening to // for events. - if active_vt != *vt { + if !*needs_handoff && active_vt != *vt { continue; } From f2156d4654b9c598c331f7d641e2647bbeb6fb18 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 23 Dec 2024 16:22:41 +0100 Subject: [PATCH 2/6] graphics: Optimize the damage buffer api It is now possible to write multiple damage locations in a single write call. Vesad also no longer processes damage for background vt's. The entire framebuffer will be redrawn once the vt switches to the foreground anyway. And finally fsync now consistently redraws the entire screen rather than having different behavior between vesad and virtio-gpud. --- graphics/fbcond/src/display.rs | 21 ++++++++--------- graphics/vesad/src/scheme.rs | 10 +++++---- graphics/vesad/src/screen.rs | 28 ++++++++++++----------- graphics/virtio-gpud/src/scheme.rs | 36 ++++++++++++++++-------------- 4 files changed, 49 insertions(+), 46 deletions(-) diff --git a/graphics/fbcond/src/display.rs b/graphics/fbcond/src/display.rs index ceaf48da44..bf6c435182 100644 --- a/graphics/fbcond/src/display.rs +++ b/graphics/fbcond/src/display.rs @@ -129,19 +129,16 @@ impl Display { } } DisplayCommand::SyncRects(sync_rects) => { - for sync_rect in sync_rects { - unsafe { - libredox::call::write( - display_file.as_raw_fd() as usize, - slice::from_raw_parts( - &sync_rect as *const Damage as *const u8, - mem::size_of::(), - ), - ) - .unwrap(); - } + unsafe { + libredox::call::write( + display_file.as_raw_fd() as usize, + slice::from_raw_parts( + sync_rects.as_ptr() as *const u8, + sync_rects.len() * mem::size_of::(), + ), + ) + .unwrap(); } - libredox::call::fsync(display_file.as_raw_fd() as usize).unwrap(); } } } diff --git a/graphics/vesad/src/scheme.rs b/graphics/vesad/src/scheme.rs index 84dcb3ec39..d2127e3a68 100644 --- a/graphics/vesad/src/scheme.rs +++ b/graphics/vesad/src/scheme.rs @@ -213,7 +213,7 @@ impl SchemeBlockMut for DisplayScheme { if let Some(screens) = self.vts.get_mut(&handle.vt) { if let Some(screen) = screens.get_mut(&handle.screen) { if handle.vt == self.active { - screen.sync(&mut self.framebuffers[handle.screen.0]); + screen.redraw(&mut self.framebuffers[handle.screen.0]); } return Ok(Some(0)); } @@ -260,11 +260,13 @@ impl SchemeBlockMut for DisplayScheme { if let Some(screens) = self.vts.get_mut(&handle.vt) { if let Some(screen) = screens.get_mut(&handle.screen) { - let count = screen.write(buf)?; if handle.vt == self.active { - screen.sync(&mut self.framebuffers[handle.screen.0]); + screen + .write(buf, Some(&mut self.framebuffers[handle.screen.0])) + .map(|count| Some(count)) + } else { + screen.write(buf, None).map(|count| Some(count)) } - Ok(Some(count)) } else { Err(Error::new(EBADF)) } diff --git a/graphics/vesad/src/screen.rs b/graphics/vesad/src/screen.rs index 2b86e83d28..d4a26c07c2 100644 --- a/graphics/vesad/src/screen.rs +++ b/graphics/vesad/src/screen.rs @@ -14,7 +14,6 @@ pub struct GraphicScreen { pub height: usize, pub offscreen: OffscreenBuffer, pub input: VecDeque, - pub sync_rects: Vec, } impl GraphicScreen { @@ -24,7 +23,6 @@ impl GraphicScreen { height, offscreen: OffscreenBuffer::new(width * height), input: VecDeque::new(), - sync_rects: Vec::new(), } } } @@ -105,7 +103,7 @@ impl GraphicScreen { !self.input.is_empty() } - pub fn write(&mut self, buf: &[u8]) -> Result { + pub fn write(&mut self, buf: &[u8], framebuffer: Option<&mut FrameBuffer>) -> Result { let sync_rects = unsafe { slice::from_raw_parts( buf.as_ptr() as *const Damage, @@ -113,13 +111,15 @@ impl GraphicScreen { ) }; - self.sync_rects.extend_from_slice(sync_rects); + if let Some(framebuffer) = framebuffer { + self.sync(framebuffer, sync_rects); + } Ok(sync_rects.len() * mem::size_of::()) } - pub fn sync(&mut self, framebuffer: &mut FrameBuffer) { - for sync_rect in self.sync_rects.drain(..) { + pub fn sync(&mut self, framebuffer: &mut FrameBuffer, sync_rects: &[Damage]) { + for sync_rect in sync_rects { let x = sync_rect.x.try_into().unwrap_or(0); let y = sync_rect.y.try_into().unwrap_or(0); let w = sync_rect.width.try_into().unwrap_or(0); @@ -152,12 +152,14 @@ impl GraphicScreen { pub fn redraw(&mut self, framebuffer: &mut FrameBuffer) { let width = self.width.try_into().unwrap(); let height = self.height.try_into().unwrap(); - self.sync_rects.push(Damage { - x: 0, - y: 0, - width, - height, - }); - self.sync(framebuffer); + self.sync( + framebuffer, + &[Damage { + x: 0, + y: 0, + width, + height, + }], + ); } } diff --git a/graphics/virtio-gpud/src/scheme.rs b/graphics/virtio-gpud/src/scheme.rs index 97c4a0f031..d34c20cda1 100644 --- a/graphics/virtio-gpud/src/scheme.rs +++ b/graphics/virtio-gpud/src/scheme.rs @@ -177,7 +177,7 @@ impl<'a> Display<'a> { } /// If `damage` is `None`, the entire screen is flushed. - async fn flush(&self, vt: usize, damage: Option<&Damage>) -> Result<(), Error> { + async fn flush(&self, vt: usize, damage: Option<&[Damage]>) -> Result<(), Error> { let Some(&res_id) = self.vts_res.borrow().get(&vt) else { // The resource is not yet created. Ignore the damage. We will write the entire backing // storage to the resource once we create the resource, which is equivalent to damaging @@ -185,17 +185,6 @@ impl<'a> Display<'a> { return Ok(()); }; - let damage = if let Some(damage) = damage { - damage.into() - } else { - GpuRect { - x: 0, - y: 0, - width: self.width, - height: self.height, - } - }; - let req = Dma::new(XferToHost2d::new( res_id, GpuRect { @@ -208,8 +197,23 @@ impl<'a> Display<'a> { let header = self.send_request(req).await?; assert_eq!(header.ty.get(), CommandTy::RespOkNodata); - self.flush_resource(ResourceFlush::new(res_id, damage.clone())) + if let Some(damage) = damage { + for damage in damage { + self.flush_resource(ResourceFlush::new(res_id, damage.into())) + .await?; + } + } else { + self.flush_resource(ResourceFlush::new( + res_id, + GpuRect { + x: 0, + y: 0, + width: self.width, + height: self.height, + }, + )) .await?; + } Ok(()) } } @@ -387,16 +391,14 @@ impl<'a> SchemeMut for Scheme<'a> { ) -> syscall::Result { let handle = self.handles.get(&id).ok_or(SysError::new(EINVAL))?; - let damages = unsafe { + let damage = unsafe { core::slice::from_raw_parts( buf.as_ptr() as *const Damage, buf.len() / core::mem::size_of::(), ) }; - for damage in damages { - futures::executor::block_on(handle.display.flush(handle.vt, Some(damage))).unwrap(); - } + futures::executor::block_on(handle.display.flush(handle.vt, Some(damage))).unwrap(); Ok(buf.len()) } From f6cc7d25dfc9afd7021ac2b94bcd9ca396aabd01 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 23 Dec 2024 16:27:57 +0100 Subject: [PATCH 3/6] graphics: Unify damage clipping --- graphics/vesad/src/screen.rs | 23 +++++++++++++---------- graphics/virtio-gpud/src/main.rs | 2 +- graphics/virtio-gpud/src/scheme.rs | 9 ++++++--- inputd/src/lib.rs | 17 +++++++++++++++++ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/graphics/vesad/src/screen.rs b/graphics/vesad/src/screen.rs index d4a26c07c2..765de4ff3d 100644 --- a/graphics/vesad/src/screen.rs +++ b/graphics/vesad/src/screen.rs @@ -120,23 +120,26 @@ impl GraphicScreen { pub fn sync(&mut self, framebuffer: &mut FrameBuffer, sync_rects: &[Damage]) { for sync_rect in sync_rects { - let x = sync_rect.x.try_into().unwrap_or(0); - let y = sync_rect.y.try_into().unwrap_or(0); - let w = sync_rect.width.try_into().unwrap_or(0); - let h = sync_rect.height.try_into().unwrap_or(0); + let sync_rect = sync_rect.clip( + self.height.try_into().unwrap(), + self.width.try_into().unwrap(), + ); - let start_y = cmp::min(self.height, y); - let end_y = cmp::min(self.height, y + h); + let start_x: usize = sync_rect.x.try_into().unwrap_or(0); + let start_y: usize = sync_rect.y.try_into().unwrap_or(0); + let w: usize = sync_rect.width.try_into().unwrap_or(0); + let h: usize = sync_rect.height.try_into().unwrap_or(0); - let start_x = cmp::min(self.width, x); - let row_pixel_count = cmp::min(self.width, x + w) - start_x; + let end_y = start_y + h; + + let row_pixel_count = w; let mut offscreen_ptr = self.offscreen.as_mut_ptr(); let mut onscreen_ptr = framebuffer.onscreen as *mut u32; // FIXME use as_mut_ptr once stable unsafe { - offscreen_ptr = offscreen_ptr.add(y * self.width + start_x); - onscreen_ptr = onscreen_ptr.add(y * framebuffer.stride + start_x); + offscreen_ptr = offscreen_ptr.add(start_y * self.width + start_x); + onscreen_ptr = onscreen_ptr.add(start_y * framebuffer.stride + start_x); let mut rows = end_y - start_y; while rows > 0 { diff --git a/graphics/virtio-gpud/src/main.rs b/graphics/virtio-gpud/src/main.rs index ab36a1048e..dcb33958f1 100644 --- a/graphics/virtio-gpud/src/main.rs +++ b/graphics/virtio-gpud/src/main.rs @@ -169,7 +169,7 @@ impl Default for ControlHeader { } } -#[derive(Debug, Clone)] +#[derive(Debug, Copy, Clone)] #[repr(C)] pub struct GpuRect { pub x: u32, diff --git a/graphics/virtio-gpud/src/scheme.rs b/graphics/virtio-gpud/src/scheme.rs index d34c20cda1..7654291251 100644 --- a/graphics/virtio-gpud/src/scheme.rs +++ b/graphics/virtio-gpud/src/scheme.rs @@ -16,7 +16,7 @@ use virtio_core::utils::VolatileCell; use crate::*; -impl Into for &Damage { +impl Into for Damage { fn into(self) -> GpuRect { GpuRect { x: self.x as u32, @@ -199,8 +199,11 @@ impl<'a> Display<'a> { if let Some(damage) = damage { for damage in damage { - self.flush_resource(ResourceFlush::new(res_id, damage.into())) - .await?; + self.flush_resource(ResourceFlush::new( + res_id, + damage.clip(self.width as i32, self.height as i32).into(), + )) + .await?; } } else { self.flush_resource(ResourceFlush::new( diff --git a/inputd/src/lib.rs b/inputd/src/lib.rs index ca8b25750f..d73cc62d28 100644 --- a/inputd/src/lib.rs +++ b/inputd/src/lib.rs @@ -1,5 +1,6 @@ #![feature(iter_next_chunk)] +use std::cmp; use std::fs::File; use std::io::{Error, Read, Write}; use std::mem::size_of; @@ -104,3 +105,19 @@ pub struct Damage { pub width: i32, pub height: i32, } + +impl Damage { + #[must_use] + pub fn clip(mut self, width: i32, height: i32) -> Self { + // Clip damage + self.x = cmp::min(self.x, width); + if self.x + self.width > width { + self.width -= cmp::min(self.width, width - self.x); + } + self.y = cmp::min(self.y, height); + if self.y + self.height > height { + self.height = cmp::min(self.height, height - self.y); + } + self + } +} From 025086acd10d533e9d6e12c96e991aba7e14859e Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 23 Dec 2024 18:10:32 +0100 Subject: [PATCH 4/6] virtio-gpud: Add offset method to XferToHost2d This will hopefully avoid confusion if someone in the future changes the XferToHost2d in flush to pass a smaller GpuRect. Without adjusting offset this would cause glitches. --- graphics/virtio-gpud/src/main.rs | 4 ++-- graphics/virtio-gpud/src/scheme.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/graphics/virtio-gpud/src/main.rs b/graphics/virtio-gpud/src/main.rs index dcb33958f1..66f41b64dd 100644 --- a/graphics/virtio-gpud/src/main.rs +++ b/graphics/virtio-gpud/src/main.rs @@ -392,7 +392,7 @@ pub struct XferToHost2d { } impl XferToHost2d { - pub fn new(resource_id: ResourceId, rect: GpuRect) -> Self { + pub fn new(resource_id: ResourceId, rect: GpuRect, offset: u64) -> Self { Self { header: ControlHeader { ty: VolatileCell::new(CommandTy::TransferToHost2d), @@ -400,8 +400,8 @@ impl XferToHost2d { }, rect, + offset, resource_id, - offset: 0, padding: 0, } } diff --git a/graphics/virtio-gpud/src/scheme.rs b/graphics/virtio-gpud/src/scheme.rs index 7654291251..a874d782cd 100644 --- a/graphics/virtio-gpud/src/scheme.rs +++ b/graphics/virtio-gpud/src/scheme.rs @@ -193,6 +193,7 @@ impl<'a> Display<'a> { width: self.width, height: self.height, }, + 0, ))?; let header = self.send_request(req).await?; assert_eq!(header.ty.get(), CommandTy::RespOkNodata); From 0019eecadde1b6e0a1fd51eeb0da86576e496dab Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 23 Dec 2024 18:39:23 +0100 Subject: [PATCH 5/6] virtio-gpud: Don't process flush requests for background VT's This ensures that the foreground VT doesn't lockup if a background VT spams us with flush requests. In the future we may want to add a scheduler or collapse redundant flush requests. --- graphics/virtio-gpud/src/scheme.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/graphics/virtio-gpud/src/scheme.rs b/graphics/virtio-gpud/src/scheme.rs index a874d782cd..c7b3bed07a 100644 --- a/graphics/virtio-gpud/src/scheme.rs +++ b/graphics/virtio-gpud/src/scheme.rs @@ -370,6 +370,11 @@ impl<'a> SchemeMut for Scheme<'a> { fn fsync(&mut self, id: usize) -> syscall::Result { let handle = self.handles.get(&id).ok_or(SysError::new(EINVAL))?; + if handle.vt != *handle.display.active_vt.borrow() { + // This is a protection against background VT's spamming us with flush requests. We will + // flush the resource on the next scanout anyway + return Ok(0); + } futures::executor::block_on(handle.display.flush(handle.vt, None)).unwrap(); Ok(0) } @@ -395,6 +400,12 @@ impl<'a> SchemeMut for Scheme<'a> { ) -> syscall::Result { let handle = self.handles.get(&id).ok_or(SysError::new(EINVAL))?; + if handle.vt != *handle.display.active_vt.borrow() { + // This is a protection against background VT's spamming us with flush requests. We will + // flush the resource on the next scanout anyway + return Ok(buf.len()); + } + let damage = unsafe { core::slice::from_raw_parts( buf.as_ptr() as *const Damage, From 05a6058131ebe0268594d5965228b2828300ea33 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 23 Dec 2024 19:05:00 +0100 Subject: [PATCH 6/6] Rustfmt --- graphics/fbcond/src/display.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/graphics/fbcond/src/display.rs b/graphics/fbcond/src/display.rs index bf6c435182..d7af9f1ab6 100644 --- a/graphics/fbcond/src/display.rs +++ b/graphics/fbcond/src/display.rs @@ -128,18 +128,16 @@ impl Display { } } } - DisplayCommand::SyncRects(sync_rects) => { - unsafe { - libredox::call::write( - display_file.as_raw_fd() as usize, - slice::from_raw_parts( - sync_rects.as_ptr() as *const u8, - sync_rects.len() * mem::size_of::(), - ), - ) - .unwrap(); - } - } + DisplayCommand::SyncRects(sync_rects) => unsafe { + libredox::call::write( + display_file.as_raw_fd() as usize, + slice::from_raw_parts( + sync_rects.as_ptr() as *const u8, + sync_rects.len() * mem::size_of::(), + ), + ) + .unwrap(); + }, } } });