From b33ea2b53a6111687cde0b2f93c3cbaf3a5200ef Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jul 2024 21:50:15 +0200 Subject: [PATCH 1/5] Fix a FIXME in display_fd_unmap The len method on raw pointers has been stabilized. --- graphics/fbcond/src/display.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/graphics/fbcond/src/display.rs b/graphics/fbcond/src/display.rs index 4efedf8263..96ea7a144a 100644 --- a/graphics/fbcond/src/display.rs +++ b/graphics/fbcond/src/display.rs @@ -36,9 +36,8 @@ fn display_fd_map(width: usize, height: usize, display_fd: usize) -> syscall::Re } } -unsafe fn display_fd_unmap(image: *mut [u32], size: usize) { - // FIXME use image.len() instead of size once it is stable. - let _ = libredox::call::munmap(image as *mut (), size * 4); +unsafe fn display_fd_unmap(image: *mut [u32]) { + let _ = libredox::call::munmap(image as *mut (), image.len()); } pub struct Display { @@ -125,7 +124,7 @@ impl Display { match display_fd_map(width, height, self.display_file.as_raw_fd() as usize) { Ok(ok) => { unsafe { - display_fd_unmap(self.offscreen, (self.width * self.height) as usize); + display_fd_unmap(self.offscreen); } self.offscreen = ok; self.width = width; @@ -154,7 +153,7 @@ impl Display { impl Drop for Display { fn drop(&mut self) { unsafe { - display_fd_unmap(self.offscreen, self.width * self.height); + display_fd_unmap(self.offscreen); } } } From ef29520cf567d53c2b8cc1bc8336fa55c7406a99 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jul 2024 21:51:51 +0200 Subject: [PATCH 2/5] Avoid ptr2int2ptr casts in GraphicScreen::sync Also use pointer arithmetic on *mut u32 over manual multiplication by 4. The ptr2int2ptr casts pessimize optimizations. --- graphics/vesad/src/screen.rs | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/graphics/vesad/src/screen.rs b/graphics/vesad/src/screen.rs index 636fdfec42..2369aafae8 100644 --- a/graphics/vesad/src/screen.rs +++ b/graphics/vesad/src/screen.rs @@ -52,11 +52,7 @@ impl GraphicScreen { for _y in 0..cmp::min(height, self.height) { unsafe { - ptr::copy( - old_ptr as *const u8, - new_ptr as *mut u8, - cmp::min(width, self.width) * 4, - ); + ptr::copy(old_ptr, new_ptr, cmp::min(width, self.width)); if width > self.width { ptr::write_bytes( new_ptr.offset(self.width as isize), @@ -145,22 +141,22 @@ impl GraphicScreen { let end_y = cmp::min(self.height, y + h); let start_x = cmp::min(self.width, x); - let len = (cmp::min(self.width, x + w) - start_x) * 4; + let row_pixel_count = cmp::min(self.width, x + w) - start_x; - let mut offscreen_ptr = self.offscreen.as_mut_ptr() as usize; - let mut onscreen_ptr = onscreen.as_mut_ptr() as usize; + let mut offscreen_ptr = self.offscreen.as_mut_ptr(); + let mut onscreen_ptr = onscreen.as_mut_ptr(); - offscreen_ptr += (y * self.width + start_x) * 4; - onscreen_ptr += (y * stride + start_x) * 4; + unsafe { + offscreen_ptr = offscreen_ptr.add(y * self.width + start_x); + onscreen_ptr = onscreen_ptr.add(y * stride + start_x); - let mut rows = end_y - start_y; - while rows > 0 { - unsafe { - ptr::copy(offscreen_ptr as *const u8, onscreen_ptr as *mut u8, len); + let mut rows = end_y - start_y; + while rows > 0 { + ptr::copy(offscreen_ptr, onscreen_ptr, row_pixel_count); + offscreen_ptr = offscreen_ptr.add(self.width); + onscreen_ptr = onscreen_ptr.add(stride); + rows -= 1; } - offscreen_ptr += self.width * 4; - onscreen_ptr += stride * 4; - rows -= 1; } } } From 857773393d7041a4fbd13ecaf0133f3d40bc03ab Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jul 2024 21:56:56 +0200 Subject: [PATCH 3/5] Ensure fields of SyncRect are not reordered Without repr(C), repr(packed) technically allows reordering fields. --- graphics/fbcond/src/display.rs | 2 +- graphics/vesad/src/screen.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/graphics/fbcond/src/display.rs b/graphics/fbcond/src/display.rs index 96ea7a144a..01f58ed2c8 100644 --- a/graphics/fbcond/src/display.rs +++ b/graphics/fbcond/src/display.rs @@ -13,7 +13,7 @@ use syscall::{O_CLOEXEC, O_NONBLOCK, O_RDWR}; // Keep synced with vesad #[derive(Clone, Copy)] -#[repr(packed)] +#[repr(C, packed)] pub struct SyncRect { pub x: i32, pub y: i32, diff --git a/graphics/vesad/src/screen.rs b/graphics/vesad/src/screen.rs index 2369aafae8..d77c095c9b 100644 --- a/graphics/vesad/src/screen.rs +++ b/graphics/vesad/src/screen.rs @@ -9,7 +9,7 @@ use crate::display::OffscreenBuffer; // Keep synced with orbital #[derive(Clone, Copy)] -#[repr(packed)] +#[repr(C, packed)] pub struct SyncRect { pub x: i32, pub y: i32, From 53c02b5322a2531cbc8bca10043a81e7cdd20ce7 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jul 2024 22:12:48 +0200 Subject: [PATCH 4/5] Move the framebuffer resizing code into a FrameBuffer method --- graphics/vesad/src/framebuffer.rs | 69 +++++++++++++++++++++++-------- graphics/vesad/src/main.rs | 4 +- graphics/vesad/src/scheme.rs | 40 +++--------------- 3 files changed, 59 insertions(+), 54 deletions(-) diff --git a/graphics/vesad/src/framebuffer.rs b/graphics/vesad/src/framebuffer.rs index a3ca0ba769..5e1da0d04b 100644 --- a/graphics/vesad/src/framebuffer.rs +++ b/graphics/vesad/src/framebuffer.rs @@ -1,6 +1,9 @@ -use std::{ptr, slice}; +use std::ptr; + +use syscall::PAGE_SIZE; pub struct FrameBuffer { + pub onscreen: *mut [u32], pub phys: usize, pub width: usize, pub height: usize, @@ -8,8 +11,25 @@ pub struct FrameBuffer { } impl FrameBuffer { - pub fn new(phys: usize, width: usize, height: usize, stride: usize) -> Self { + pub unsafe fn new(phys: usize, width: usize, height: usize, stride: usize) -> Self { + let size = stride * height; + let virt = common::physmap( + phys, + size * 4, + common::Prot { + read: true, + write: true, + }, + common::MemoryType::WriteCombining, + ) + .expect("vesad: failed to map framebuffer") as *mut u32; + //TODO: should we clear the framebuffer here? + ptr::write_bytes(virt, 0, size); + + let onscreen = ptr::slice_from_raw_parts_mut(virt, size); + Self { + onscreen, phys, width, height, @@ -17,7 +37,7 @@ impl FrameBuffer { } } - pub fn parse(var: &str) -> Option { + pub unsafe fn parse(var: &str) -> Option { fn parse_number(part: &str) -> Option { let (start, radix) = if part.starts_with("0x") { (2, 16) @@ -41,20 +61,35 @@ impl FrameBuffer { Some(Self::new(phys, width, height, stride)) } - pub unsafe fn map(&mut self) -> syscall::Result<&'static mut [u32]> { - let size = self.stride * self.height; - let virt = common::physmap( - self.phys, - size * 4, - common::Prot { - read: true, - write: true, - }, - common::MemoryType::WriteCombining, - )? as *mut u32; - //TODO: should we clear the framebuffer here? - ptr::write_bytes(virt, 0, size); + pub unsafe fn resize(&mut self, width: usize, height: usize, stride: usize) { + // Unmap old onscreen + unsafe { + let slice = self.onscreen; + libredox::call::munmap(slice.cast(), (slice.len() * 4).next_multiple_of(PAGE_SIZE)) + .expect("vesad: failed to unmap framebuffer"); + } - Ok(slice::from_raw_parts_mut(virt, size)) + // Map new onscreen + self.onscreen = unsafe { + let size = stride * height; + let onscreen_ptr = common::physmap( + self.phys, + size * 4, + common::Prot { + read: true, + write: true, + }, + common::MemoryType::WriteCombining, + ) + .expect("vesad: failed to map framebuffer") as *mut u32; + ptr::write_bytes(onscreen_ptr, 0, size); + + ptr::slice_from_raw_parts_mut(onscreen_ptr, size) + }; + + // Update size + self.width = width; + self.height = height; + self.stride = stride; } } diff --git a/graphics/vesad/src/main.rs b/graphics/vesad/src/main.rs index 54e257ebd5..bb2bcc6985 100644 --- a/graphics/vesad/src/main.rs +++ b/graphics/vesad/src/main.rs @@ -54,12 +54,12 @@ fn main() { return; } - let mut framebuffers = vec![FrameBuffer::new(phys, width, height, stride)]; + let mut framebuffers = vec![unsafe { FrameBuffer::new(phys, width, height, stride) }]; //TODO: ideal maximum number of outputs? for i in 1..1024 { match env::var(&format!("FRAMEBUFFER{}", i)) { - Ok(var) => match FrameBuffer::parse(&var) { + Ok(var) => match unsafe { FrameBuffer::parse(&var) } { Some(fb) => { println!( "vesad: framebuffer {}: {}x{} stride {} at 0x{:X}", diff --git a/graphics/vesad/src/scheme.rs b/graphics/vesad/src/scheme.rs index 1db03b05f9..c5221f95c3 100644 --- a/graphics/vesad/src/scheme.rs +++ b/graphics/vesad/src/scheme.rs @@ -1,9 +1,7 @@ use std::collections::BTreeMap; -use std::{mem, ptr, slice, str}; +use std::str; -use syscall::{ - Error, EventFlags, MapFlags, Result, SchemeMut, EBADF, EINVAL, ENOENT, O_NONBLOCK, PAGE_SIZE, -}; +use syscall::{Error, EventFlags, MapFlags, Result, SchemeMut, EBADF, EINVAL, ENOENT, O_NONBLOCK}; use crate::{framebuffer::FrameBuffer, screen::GraphicScreen}; @@ -43,7 +41,7 @@ impl DisplayScheme { let mut onscreens = Vec::new(); for fb in framebuffers.iter_mut() { - onscreens.push(unsafe { fb.map().expect("vesad: failed to map framebuffer") }); + onscreens.push(unsafe { &mut *fb.onscreen }); } let mut vts = BTreeMap::>::new(); @@ -96,39 +94,11 @@ impl DisplayScheme { fb_i, width, height, stride ); - // Unmap old onscreen unsafe { - let slice = mem::take(&mut self.onscreens[fb_i]); - libredox::call::munmap( - slice.as_mut_ptr().cast(), - (slice.len() * 4).next_multiple_of(PAGE_SIZE), - ) - .expect("vesad: failed to unmap framebuffer"); + self.framebuffers[fb_i].resize(width, height, stride); + self.onscreens[fb_i] = &mut *self.framebuffers[fb_i].onscreen; } - // Map new onscreen - self.onscreens[fb_i] = unsafe { - let size = stride * height; - let onscreen_ptr = common::physmap( - self.framebuffers[fb_i].phys, - size * 4, - common::Prot { - read: true, - write: true, - }, - common::MemoryType::WriteCombining, - ) - .expect("vesad: failed to map framebuffer") as *mut u32; - ptr::write_bytes(onscreen_ptr, 0, size); - - slice::from_raw_parts_mut(onscreen_ptr, size) - }; - - // Update size - self.framebuffers[fb_i].width = width; - self.framebuffers[fb_i].height = height; - self.framebuffers[fb_i].stride = stride; - // Resize screens for (vt_i, screens) in self.vts.iter_mut() { for (screen_i, screen) in screens.iter_mut() { From 2e341fa7426083820e16b2dfb6624d831afd15f2 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 19 Jul 2024 22:18:34 +0200 Subject: [PATCH 5/5] Directly pass FrameBuffer to redraw and sync This avoids creating an intermediate &'static mut [u32] of questionable soundness and simplifies the code a bit. --- graphics/vesad/src/scheme.rs | 27 +++++---------------------- graphics/vesad/src/screen.rs | 13 +++++++------ 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/graphics/vesad/src/scheme.rs b/graphics/vesad/src/scheme.rs index c5221f95c3..1d4f7cb238 100644 --- a/graphics/vesad/src/scheme.rs +++ b/graphics/vesad/src/scheme.rs @@ -27,7 +27,6 @@ pub struct Handle { pub struct DisplayScheme { framebuffers: Vec, - onscreens: Vec<&'static mut [u32]>, active: VtIndex, pub vts: BTreeMap>, next_id: usize, @@ -36,14 +35,9 @@ pub struct DisplayScheme { } impl DisplayScheme { - pub fn new(mut framebuffers: Vec, spec: &[()]) -> DisplayScheme { + pub fn new(framebuffers: Vec, spec: &[()]) -> DisplayScheme { let mut inputd_handle = inputd::Handle::new("vesa").unwrap(); - let mut onscreens = Vec::new(); - for fb in framebuffers.iter_mut() { - onscreens.push(unsafe { &mut *fb.onscreen }); - } - let mut vts = BTreeMap::>::new(); for &() in spec.iter() { @@ -57,7 +51,6 @@ impl DisplayScheme { DisplayScheme { framebuffers, - onscreens, active: VtIndex(1), vts, next_id: 0, @@ -96,7 +89,6 @@ impl DisplayScheme { unsafe { self.framebuffers[fb_i].resize(width, height, stride); - self.onscreens[fb_i] = &mut *self.framebuffers[fb_i].onscreen; } // Resize screens @@ -105,7 +97,7 @@ impl DisplayScheme { if screen_i.0 == fb_i { screen.resize(width, height); if *vt_i == self.active { - screen.redraw(self.onscreens[fb_i], self.framebuffers[fb_i].stride); + screen.redraw(&mut self.framebuffers[screen_i.0]); } } } @@ -235,10 +227,7 @@ impl SchemeMut for DisplayScheme { if let Some(screens) = self.vts.get_mut(&vt_i) { if let Some(screen) = screens.get_mut(&screen_i) { if vt_i == self.active { - screen.sync( - self.onscreens[screen_i.0], - self.framebuffers[screen_i.0].stride, - ); + screen.sync(&mut self.framebuffers[screen_i.0]); } return Ok(0); } @@ -277,10 +266,7 @@ impl SchemeMut for DisplayScheme { if let Some(screens) = self.vts.get_mut(&vt_i) { for (screen_i, screen) in screens.iter_mut() { - screen.redraw( - self.onscreens[screen_i.0], - self.framebuffers[screen_i.0].stride, - ); + screen.redraw(&mut self.framebuffers[screen_i.0]); } } @@ -308,10 +294,7 @@ impl SchemeMut for DisplayScheme { if let Some(screen) = screens.get_mut(&screen_i) { let count = screen.write(buf)?; if vt_i == self.active { - screen.sync( - self.onscreens[screen_i.0], - self.framebuffers[screen_i.0].stride, - ); + screen.sync(&mut self.framebuffers[screen_i.0]); } Ok(count) } else { diff --git a/graphics/vesad/src/screen.rs b/graphics/vesad/src/screen.rs index d77c095c9b..50f2f05af2 100644 --- a/graphics/vesad/src/screen.rs +++ b/graphics/vesad/src/screen.rs @@ -6,6 +6,7 @@ use orbclient::{Event, ResizeEvent}; use syscall::error::*; use crate::display::OffscreenBuffer; +use crate::framebuffer::FrameBuffer; // Keep synced with orbital #[derive(Clone, Copy)] @@ -130,7 +131,7 @@ impl GraphicScreen { Ok(sync_rects.len() * mem::size_of::()) } - pub fn sync(&mut self, onscreen: &mut [u32], stride: usize) { + pub fn sync(&mut self, framebuffer: &mut FrameBuffer) { for sync_rect in self.sync_rects.drain(..) { let x = sync_rect.x.try_into().unwrap_or(0); let y = sync_rect.y.try_into().unwrap_or(0); @@ -144,24 +145,24 @@ impl GraphicScreen { let row_pixel_count = cmp::min(self.width, x + w) - start_x; let mut offscreen_ptr = self.offscreen.as_mut_ptr(); - let mut onscreen_ptr = onscreen.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 * stride + start_x); + onscreen_ptr = onscreen_ptr.add(y * framebuffer.stride + start_x); let mut rows = end_y - start_y; while rows > 0 { ptr::copy(offscreen_ptr, onscreen_ptr, row_pixel_count); offscreen_ptr = offscreen_ptr.add(self.width); - onscreen_ptr = onscreen_ptr.add(stride); + onscreen_ptr = onscreen_ptr.add(framebuffer.stride); rows -= 1; } } } } - pub fn redraw(&mut self, onscreen: &mut [u32], stride: usize) { + 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(SyncRect { @@ -170,6 +171,6 @@ impl GraphicScreen { w: width, h: height, }); - self.sync(onscreen, stride); + self.sync(framebuffer); } }