From 0f5b99f8aa9bb2deedc06b9e18b9492e5129d52b Mon Sep 17 00:00:00 2001 From: Hadrien G Date: Sun, 14 Oct 2018 13:53:10 +0200 Subject: [PATCH 1/4] Clean up direct framebuffer access --- src/proto/console/gop.rs | 73 +++++++++++++++++------ uefi-test-runner/src/proto/console/gop.rs | 30 +++++----- 2 files changed, 70 insertions(+), 33 deletions(-) diff --git a/src/proto/console/gop.rs b/src/proto/console/gop.rs index 56d84eea0..cd89f5400 100644 --- a/src/proto/console/gop.rs +++ b/src/proto/console/gop.rs @@ -23,6 +23,7 @@ //! In theory, a buffer with a width of 640 should have (640 * 4) bytes per row, //! but in practice there might be some extra padding used for efficiency. +use core::marker::PhantomData; use core::ptr; use crate::{Completion, Result, Status}; @@ -249,29 +250,20 @@ impl GraphicsOutput { *self.mode.info } - /// Returns the base pointer and size (in bytes) of the framebuffer - /// - /// To use this pointer safely, a caller must... - /// - Honor the pixel format specificed by the mode info - /// - Keep pointer accesses in bound - /// - Use volatile writes so that the compiler does not optimize out or - /// aggressively reorder framebuffer accesses - /// - Make sure that the pointer is not used beyond its validity limit - /// - /// Although the UEFI spec makes no clear statement about framebuffer - /// pointer validity, it seems reasonable to expect the framebuffer pointer - /// to be valid until the next mode change. In the future, a safer interface - /// may be introduced, which would enforce volatile writes and automatically - /// check for dangling pointers using Rust's borrow checker. - pub fn frame_buffer(&mut self) -> (*mut u8, usize) { + /// Access the frame buffer directly + pub fn frame_buffer(&mut self) -> FrameBuffer { assert!( self.mode.info.format != PixelFormat::BltOnly, "Cannot access the framebuffer in a Blt-only mode" ); - let data = self.mode.fb_address as *mut u8; - let len = self.mode.fb_size; + let base = self.mode.fb_address as *mut u8; + let size = self.mode.fb_size; - (data, len) + FrameBuffer { + base, + size, + _lifetime: PhantomData, + } } } @@ -519,3 +511,48 @@ pub enum BltOp<'a> { dims: (usize, usize), }, } + +/// Direct access to a memory-mapped frame buffer +pub struct FrameBuffer<'a> { + base: *mut u8, + size: usize, + _lifetime: PhantomData<&'a mut u8>, +} + +impl<'a> FrameBuffer<'a> { + /// Access the raw framebuffer pointer + /// + /// To use this pointer safely and correctly, you must... + /// - Honor the pixel format and stride specified by the mode info + /// - Keep memory accesses in bound + /// - Use volatile reads and writes + /// - Make sure that the pointer does not outlive the FrameBuffer + pub fn as_mut_ptr(&mut self) -> *mut u8 { + self.base + } + + /// Query the framebuffer size in bytes + pub fn len(&self) -> usize { + self.size + } + + /// Modify the i-th byte of the frame buffer + /// + /// This operation is unsafe because... + /// - You must honor the pixel format and stride specified by the mode info + /// - There is no bound checking on memory accesses in release mode + pub unsafe fn write_byte(&mut self, index: usize, value: u8) { + debug_assert!(index < self.size, "Frame buffer accessed out of bounds"); + self.base.add(index).write_volatile(value) + } + + /// Read the i-th byte of the frame buffer + /// + /// This operation is unsafe because... + /// - You must honor the pixel format and stride specified by the mode info + /// - There is no bound checking on memory accesses in release mode + pub unsafe fn read_byte(&mut self, index: usize) -> u8 { + debug_assert!(index < self.size, "Frame buffer accessed out of bounds"); + self.base.add(index).read_volatile() + } +} \ No newline at end of file diff --git a/uefi-test-runner/src/proto/console/gop.rs b/uefi-test-runner/src/proto/console/gop.rs index 21143ee76..aaaf9d3a4 100644 --- a/uefi-test-runner/src/proto/console/gop.rs +++ b/uefi-test-runner/src/proto/console/gop.rs @@ -1,5 +1,5 @@ use uefi::prelude::*; -use uefi::proto::console::gop::{BltOp, BltPixel, GraphicsOutput, PixelFormat}; +use uefi::proto::console::gop::{BltOp, BltPixel, FrameBuffer, GraphicsOutput, PixelFormat}; use uefi::table::boot::BootServices; use uefi_exts::BootServicesExt; @@ -54,20 +54,20 @@ fn draw_fb(gop: &mut GraphicsOutput) { let stride = mi.stride(); let (width, height) = mi.resolution(); - let (fb_base, _fb_size) = gop.frame_buffer(); + let mut fb = gop.frame_buffer(); - type PixelWriter = unsafe fn(*mut u8, [u8; 3]); - unsafe fn write_pixel_rgb(pixel_base: *mut u8, rgb: [u8; 3]) { + type PixelWriter = unsafe fn(&mut FrameBuffer, usize, [u8; 3]); + unsafe fn write_pixel_rgb(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - pixel_base.add(0).write_volatile(r); - pixel_base.add(1).write_volatile(g); - pixel_base.add(2).write_volatile(b); + fb.write_byte(pixel_base + 0, r); + fb.write_byte(pixel_base + 1, g); + fb.write_byte(pixel_base + 2, b); }; - unsafe fn write_pixel_bgr(pixel_base: *mut u8, rgb: [u8; 3]) { + unsafe fn write_pixel_bgr(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - pixel_base.add(0).write_volatile(b); - pixel_base.add(1).write_volatile(g); - pixel_base.add(2).write_volatile(r); + fb.write_byte(pixel_base + 0, b); + fb.write_byte(pixel_base + 1, g); + fb.write_byte(pixel_base + 2, r); }; let write_pixel: PixelWriter = match mi.pixel_format() { PixelFormat::RGB => write_pixel_rgb, @@ -78,15 +78,15 @@ fn draw_fb(gop: &mut GraphicsOutput) { } }; - let fill_rectangle = |(x1, y1), (x2, y2), color| { + let mut fill_rectangle = |(x1, y1), (x2, y2), color| { assert!((x1 < width) && (x2 < width), "Bad X coordinate"); assert!((y1 < height) && (y2 < height), "Bad Y coordinate"); for row in y1..y2 { for column in x1..x2 { unsafe { - let index = (row * stride) + column; - let pixel_base = fb_base.add(4 * index); - write_pixel(pixel_base, color); + let pixel_index = (row * stride) + column; + let pixel_base = 4 * pixel_index; + write_pixel(&mut fb, pixel_base, color); } } } From e732fc832d1d78a5d2e86d8c4a1f7f34e13097a2 Mon Sep 17 00:00:00 2001 From: Hadrien G Date: Sun, 14 Oct 2018 13:56:07 +0200 Subject: [PATCH 2/4] Make framebuffer more clippy-friendly --- src/proto/console/gop.rs | 2 +- uefi-test-runner/src/proto/console/gop.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/proto/console/gop.rs b/src/proto/console/gop.rs index cd89f5400..36e679316 100644 --- a/src/proto/console/gop.rs +++ b/src/proto/console/gop.rs @@ -532,7 +532,7 @@ impl<'a> FrameBuffer<'a> { } /// Query the framebuffer size in bytes - pub fn len(&self) -> usize { + pub fn size(&self) -> usize { self.size } diff --git a/uefi-test-runner/src/proto/console/gop.rs b/uefi-test-runner/src/proto/console/gop.rs index aaaf9d3a4..eae805a02 100644 --- a/uefi-test-runner/src/proto/console/gop.rs +++ b/uefi-test-runner/src/proto/console/gop.rs @@ -59,13 +59,13 @@ fn draw_fb(gop: &mut GraphicsOutput) { type PixelWriter = unsafe fn(&mut FrameBuffer, usize, [u8; 3]); unsafe fn write_pixel_rgb(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - fb.write_byte(pixel_base + 0, r); + fb.write_byte(pixel_base, r); fb.write_byte(pixel_base + 1, g); fb.write_byte(pixel_base + 2, b); }; unsafe fn write_pixel_bgr(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - fb.write_byte(pixel_base + 0, b); + fb.write_byte(pixel_base, b); fb.write_byte(pixel_base + 1, g); fb.write_byte(pixel_base + 2, r); }; From d5b72ea70c71aa6102468c6dcbe143834cf715a7 Mon Sep 17 00:00:00 2001 From: Hadrien G Date: Sun, 14 Oct 2018 13:56:34 +0200 Subject: [PATCH 3/4] Apply cargo fmt --- src/proto/console/gop.rs | 2 +- uefi-test-runner/src/proto/console/gop.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/proto/console/gop.rs b/src/proto/console/gop.rs index 36e679316..dc2b7eb25 100644 --- a/src/proto/console/gop.rs +++ b/src/proto/console/gop.rs @@ -555,4 +555,4 @@ impl<'a> FrameBuffer<'a> { debug_assert!(index < self.size, "Frame buffer accessed out of bounds"); self.base.add(index).read_volatile() } -} \ No newline at end of file +} diff --git a/uefi-test-runner/src/proto/console/gop.rs b/uefi-test-runner/src/proto/console/gop.rs index eae805a02..cb2cc6509 100644 --- a/uefi-test-runner/src/proto/console/gop.rs +++ b/uefi-test-runner/src/proto/console/gop.rs @@ -59,13 +59,13 @@ fn draw_fb(gop: &mut GraphicsOutput) { type PixelWriter = unsafe fn(&mut FrameBuffer, usize, [u8; 3]); unsafe fn write_pixel_rgb(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - fb.write_byte(pixel_base, r); + fb.write_byte(pixel_base, r); fb.write_byte(pixel_base + 1, g); fb.write_byte(pixel_base + 2, b); }; unsafe fn write_pixel_bgr(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { let [r, g, b] = rgb; - fb.write_byte(pixel_base, b); + fb.write_byte(pixel_base, b); fb.write_byte(pixel_base + 1, g); fb.write_byte(pixel_base + 2, r); }; From 74ed83578bb81653f0e3d4fdeba07a5bb3b6ec84 Mon Sep 17 00:00:00 2001 From: Hadrien G Date: Sun, 14 Oct 2018 14:18:24 +0200 Subject: [PATCH 4/4] Enable multi-byte writes --- src/proto/console/gop.rs | 43 ++++++++++++++++++++++- uefi-test-runner/src/proto/console/gop.rs | 10 ++---- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/proto/console/gop.rs b/src/proto/console/gop.rs index dc2b7eb25..f2c78ff72 100644 --- a/src/proto/console/gop.rs +++ b/src/proto/console/gop.rs @@ -24,6 +24,7 @@ //! but in practice there might be some extra padding used for efficiency. use core::marker::PhantomData; +use core::mem; use core::ptr; use crate::{Completion, Result, Status}; @@ -541,6 +542,7 @@ impl<'a> FrameBuffer<'a> { /// This operation is unsafe because... /// - You must honor the pixel format and stride specified by the mode info /// - There is no bound checking on memory accesses in release mode + #[inline] pub unsafe fn write_byte(&mut self, index: usize, value: u8) { debug_assert!(index < self.size, "Frame buffer accessed out of bounds"); self.base.add(index).write_volatile(value) @@ -551,8 +553,47 @@ impl<'a> FrameBuffer<'a> { /// This operation is unsafe because... /// - You must honor the pixel format and stride specified by the mode info /// - There is no bound checking on memory accesses in release mode - pub unsafe fn read_byte(&mut self, index: usize) -> u8 { + #[inline] + pub unsafe fn read_byte(&self, index: usize) -> u8 { debug_assert!(index < self.size, "Frame buffer accessed out of bounds"); self.base.add(index).read_volatile() } + + /// Write a value in the frame buffer, starting at the i-th byte + /// + /// We only recommend using this method with [u8; N] arrays. Once Rust has + /// const generics, it will be deprecated and replaced with a write_bytes() + /// method that only accepts [u8; N] input. + /// + /// This operation is unsafe because... + /// - It is your reponsibility to make sure that the value type makes sense + /// - You must honor the pixel format and stride specified by the mode info + /// - There is no bound checking on memory accesses in release mode + #[inline] + pub unsafe fn write_value(&mut self, index: usize, value: T) { + debug_assert!( + index.saturating_add(mem::size_of::()) <= self.size, + "Frame buffer accessed out of bounds" + ); + (self.base.add(index) as *mut T).write_volatile(value) + } + + /// Read a value from the frame buffer, starting at the i-th byte + /// + /// We only recommend using this method with [u8; N] arrays. Once Rust has + /// const generics, it will be deprecated and replaced with a read_bytes() + /// method that only accepts [u8; N] input. + /// + /// This operation is unsafe because... + /// - It is your reponsibility to make sure that the value type makes sense + /// - You must honor the pixel format and stride specified by the mode info + /// - There is no bound checking on memory accesses in release mode + #[inline] + pub unsafe fn read_value(&self, index: usize) -> T { + debug_assert!( + index.saturating_add(mem::size_of::()) <= self.size, + "Frame buffer accessed out of bounds" + ); + (self.base.add(index) as *const T).read_volatile() + } } diff --git a/uefi-test-runner/src/proto/console/gop.rs b/uefi-test-runner/src/proto/console/gop.rs index cb2cc6509..f07536698 100644 --- a/uefi-test-runner/src/proto/console/gop.rs +++ b/uefi-test-runner/src/proto/console/gop.rs @@ -58,16 +58,10 @@ fn draw_fb(gop: &mut GraphicsOutput) { type PixelWriter = unsafe fn(&mut FrameBuffer, usize, [u8; 3]); unsafe fn write_pixel_rgb(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { - let [r, g, b] = rgb; - fb.write_byte(pixel_base, r); - fb.write_byte(pixel_base + 1, g); - fb.write_byte(pixel_base + 2, b); + fb.write_value(pixel_base, rgb); }; unsafe fn write_pixel_bgr(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) { - let [r, g, b] = rgb; - fb.write_byte(pixel_base, b); - fb.write_byte(pixel_base + 1, g); - fb.write_byte(pixel_base + 2, r); + fb.write_value(pixel_base, [rgb[2], rgb[1], rgb[0]]); }; let write_pixel: PixelWriter = match mi.pixel_format() { PixelFormat::RGB => write_pixel_rgb,