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

[RaitoBezarius]

Given that PixelFormat for UEFI is dynamic and DrawTarget needs it to be static.

We (anyway) need to bridge the gap by asserting this dynamically and providing API to create the
best wrapper.

In some cases, we might even need to convert the pixel to the target format by relying on Blt or similar.

[RaitoBezarius]

This is only a WIP and address partially #781. This is open for suggestions and ideas before I go further in the design.

[phip1611]

I like the idea! 👍🏻

[phip1611]

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.

[RaitoBezarius]

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

[phip1611]

This would imply a dependency on the alloc feature, tho

[RaitoBezarius]

Hm, correct

[phip1611]

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

[RaitoBezarius]

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

[phip1611]

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

[RaitoBezarius]

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.

[nicholasbishop]

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.

[medhefgo]

Making this generic over pixel format pushes the burden of handling them (correctly) on the consumer, no? That doesn't sound very nice to me. A usage snippet would be nice…

[RaitoBezarius]

Making this generic over pixel format pushes the burden of handling them (correctly) on the consumer, no? That doesn't sound very nice to me. A usage snippet would be nice…

This is an open discussion, my idea regarding this is: there's no way around if we want it to be nice on the consumer.

(1) Have generic over PixelFormat
(2) If pixel format instantiated != pixel format available, perform the conversions ourselves (performance cost but 🤷 — we could implement is_efficient to make the user notice that it's doing bad things).
(3) Dynamic dispatching to the optimized instance (I'm not sure this design enable this though)

[medhefgo]

The way I see it: Just draw into a [BltPixel] (your backbuffer) and then blit it to the framebuffer when an explicit flush() call is made by the consumer:

1. embedded_graphics_core just hands you an iterator of pixels to draw (as opposed to a (sub)region of a backbuffer to memcpy). So performance is out of the window anyways.
2. Only BltPixel is guaranteed by the spec, so has to be supported anyways. (Although, direct access in rgb888 or bgr888 format should almost always be available).
3. Other pixel formats complicate things.
4. There is no support for vsync in the gop protocol (a really stupid oversight by the spec if you ask me). So drawing into a backbuffer with an explicit flush command like shown in one of the embedded_graphics_core examples would reduce potential flicker (but cannot eliminate it).
5. From making https://github.com/slint-ui/slint/tree/master/examples/uefi-demo I can say that you're not gonna run into any performance issues from drawing full HD content that changes a log (V-Sync torture test).

[RaitoBezarius]

embedded_graphics_core just hands you an iterator of pixels to draw (as opposed to a (sub)region of a backbuffer to memcpy). So performance is out of the window anyways.

fill_solid is a thing, we just didn't implement it

[RaitoBezarius]

The way I see it: Just draw into a [BltPixel] (your backbuffer) and then blit it to the framebuffer when an explicit flush() call is made by the consumer:

1. embedded_graphics_core just hands you an iterator of pixels to draw (as opposed to a (sub)region of a backbuffer to memcpy). So performance is out of the window anyways.
2. Only BltPixel is guaranteed by the spec, so has to be supported anyways. (Although, direct access in rgb888 or bgr888 format should almost always be available).
3. Other pixel formats complicate things.
4. There is no support for vsync in the gop protocol (a really stupid oversight by the spec if you ask me). So drawing into a backbuffer with an explicit flush command like shown in one of the embedded_graphics_core examples would reduce potential flicker (but cannot eliminate it).
5. From making https://github.com/slint-ui/slint/tree/master/examples/uefi-demo I can say that you're not gonna run into any performance issues from drawing full HD content that changes a log (V-Sync torture test).

I agree, it's good alternative design.
Will try to implement this this week.

[medhefgo]

fill_solid is a thing, we just didn't implement it

I know. You can use Efibltvideofill for that, though that would break explicit flushing.

[phip1611]

I think this wasn't too far from the finishing line. Any update, @RaitoBezarius ?