Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uefi(gop): add a embedded_graphics_core::DrawTarget for (a wrapper of) GraphicsOutput #820

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion uefi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,22 @@ license = "MPL-2.0"
rust-version = "1.68"

[features]
default = ["panic-on-logger-errors"]
default = ["panic-on-logger-errors", "draw_target"]
alloc = []
global_allocator = []
logger = []
# Ignore text output errors in logger as a workaround for firmware issues that
# were observed on the VirtualBox UEFI implementation (see uefi-rs#121).
# In those cases, this feature can be excluded by removing the default features.
panic-on-logger-errors = []
# DrawTarget for GOP protocol
draw_target = ["embedded-graphics-core"]
# Generic gate to code that uses unstable features of Rust. You usually need a nightly toolchain.
unstable = []

[dependencies]
bitflags = "2.0.0"
embedded-graphics-core = { version = "0.4.0", optional = true }
Copy link
Contributor

@phip1611 phip1611 May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we do not need a feature flag for this. What do you @nicholasbishop think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would imagine that the embedded-graphics-core closure would bloat a bit things?

Copy link
Contributor

@phip1611 phip1611 May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this will end up in a binary, if the GOP is not used. Probalby, at least with LTO, this stuff should be garbage-collected?! What do you think? I'm not sure here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something something rust zero cost abstraction yada yada? :D

more seriously: I was reasoning on a purely "developer" side, bringing more dependency is more annoying usually, but this might be a fair tradeoff here given it's tailored for embedded systems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the feature flag is good. Over time we'll likely add interop with more crates like this (one example I've thought about before: conversion between our Time and the time crate), and I wouldn't want to make all such crates required.

I'm uncertain about the feature name, draw_target seems a little generic. Maybe embedded_graphics to reflect crate name?

Also, I think we can drop it from the default features, I'd like to keep that set minimal.

log = { version = "0.4.5", default-features = false }
ptr_meta = { version = "0.2.0", default-features = false }
ucs2 = "0.3.2"
Expand Down
54 changes: 54 additions & 0 deletions uefi/src/proto/console/draw_target.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
use embedded_graphics_core::prelude::{DrawTarget, OriginDimensions, Size, PixelColor, Pixel, IntoStorage};

use super::gop::GraphicsOutput;

// FIXME: offer conversions from C to current pixel color format?
struct GraphicsDisplay<C: PixelColor> {
color: C,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Although this is not consistent across the code-base, personally I prefer Box<dyn PixelColor for all non-performance critical stuff. Having a generic parameter has the consequence that all wrapping structs (that library users may create in higher levels) are also forced to be generic. But this is just a nit and you don't have to address it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'm open to Box<dyn PixelColor> really :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would imply a dependency on the alloc feature, tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, correct

gop: GraphicsOutput
}

impl OriginDimensions for GraphicsOutput {
fn size(&self) -> embedded_graphics_core::prelude::Size {
let (width, height) = self.current_mode_info().resolution();

Size::from((width as u32, height as u32))
}
}

impl<C: PixelColor> OriginDimensions for GraphicsDisplay<C> {
fn size(&self) -> Size {
self.gop.size()
}
}

impl<C: PixelColor + IntoStorage> DrawTarget for GraphicsDisplay<C> {
type Color = C;
type Error = uefi::Error;

fn draw_iter<I>(&mut self, pixels: I) -> Result<(), Self::Error>
where
I: IntoIterator<Item = embedded_graphics_core::Pixel<Self::Color>> {
let stride = self.gop.current_mode_info().stride() as u64;
for Pixel(point, color) in pixels.into_iter() {
let bytes = color.into_storage();
let (x, y) = (point.x as u64, point.y as u64);
let index: usize = (((y * stride) + x) * 4)
.try_into()
.map_err(|_|
uefi::Error::from(
uefi::Status::UNSUPPORTED
)
)?;

unsafe {
self.gop.frame_buffer().write_value(index, bytes);
}
}

Ok(())
}

// FIXME: provide a blt technique for fill_solid
// FIXME: fallback to blt when pixelformat is blt-only.
}
2 changes: 2 additions & 0 deletions uefi/src/proto/console/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ pub mod gop;
pub mod pointer;
pub mod serial;
pub mod text;
#[cfg(feature = "draw_target")]
pub mod draw_target;