Clean up framebuffer abstraction (#64)

Author: HadrienG2

src/proto/console/gop.rs

@@ -23,6 +23,8 @@
 //! 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::mem;
 use core::ptr;
 use crate::{Completion, Result, Status};
 
@@ -249,29 +251,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 +512,88 @@ 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 size(&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
+    #[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)
+    }
+
+    /// 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
+    #[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<T>(&mut self, index: usize, value: T) {
+        debug_assert!(
+            index.saturating_add(mem::size_of::<T>()) <= 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<T>(&self, index: usize) -> T {
+        debug_assert!(
+            index.saturating_add(mem::size_of::<T>()) <= self.size,
+            "Frame buffer accessed out of bounds"
+        );
+        (self.base.add(index) as *const T).read_volatile()
+    }
+}

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,14 @@ 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]) {
-        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);
+    type PixelWriter = unsafe fn(&mut FrameBuffer, usize, [u8; 3]);
+    unsafe fn write_pixel_rgb(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) {
+        fb.write_value(pixel_base, rgb);
     };
-    unsafe fn write_pixel_bgr(pixel_base: *mut u8, 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);
+    unsafe fn write_pixel_bgr(fb: &mut FrameBuffer, pixel_base: usize, rgb: [u8; 3]) {
+        fb.write_value(pixel_base, [rgb[2], rgb[1], rgb[0]]);
     };
     let write_pixel: PixelWriter = match mi.pixel_format() {
         PixelFormat::RGB => write_pixel_rgb,
@@ -78,15 +72,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);
                 }
             }
         }