[PATCH] efi: Correct handling of frame buffer

[Heinrich Schuchardt]

On 8/25/23 21:28, Simon Glass wrote:
> The efi_gop driver uses private fields from the video uclass to obtain a
> pointer to the frame buffer. Use the platform data instead.
>
> Check the VIDEO_COPY setting to determine which frame buffer to use. Once
> the next stage is running (and making use of U-Boot's EFI boot services)
> U-Boot does not handle copying from priv->fb to the hardware framebuffer,
> so we must allow EFI to write directly to the hardware framebuffer.

Let's think of a EFI application running:

With your patch GOP API calls will use the hardware buffer. But text
output by calling OutputString() and messages generated in the U-Boot
code would write to the copy buffer. This will lead to unexpected output.

What triggers copying from the copy buffer to the hardware buffer?

Where is the copy function implemented?

>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>   lib/efi_loader/efi_gop.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 778b693f983..a09db31eb46 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -10,6 +10,7 @@
>   #include <efi_loader.h>
>   #include <log.h>
>   #include <malloc.h>
> +#include <mapmem.h>
>   #include <video.h>
>   #include <asm/global_data.h>
>
> @@ -467,10 +468,10 @@ efi_status_t efi_gop_register(void)
>   	struct efi_gop_obj *gopobj;
>   	u32 bpix, format, col, row;
>   	u64 fb_base, fb_size;
> -	void *fb;
>   	efi_status_t ret;
>   	struct udevice *vdev;
>   	struct video_priv *priv;
> +	struct video_uc_plat *plat;
>
>   	/* We only support a single video output device for now */
>   	if (uclass_first_device_err(UCLASS_VIDEO, &vdev)) {
> @@ -483,9 +484,10 @@ efi_status_t efi_gop_register(void)
>   	format = priv->format;
>   	col = video_get_xsize(vdev);
>   	row = video_get_ysize(vdev);
> -	fb_base = (uintptr_t)priv->fb;
> -	fb_size = priv->fb_size;
> -	fb = priv->fb;
> +
> +	plat = dev_get_uclass_plat(vdev);
> +	fb_base = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base;
> +	fb_size = plat->size;

This logic and the map_sysmem() conversion would better be placed in
video-uclass.c or video.h as a function video_get_hw_fb(void *fb_base,
size_t *fb_size).
>
>   	switch (bpix) {
>   	case VIDEO_BPP16:
> @@ -547,7 +549,7 @@ efi_status_t efi_gop_register(void)
>   	}
>   	gopobj->info.pixels_per_scanline = col;
>   	gopobj->bpix = bpix;
> -	gopobj->fb = fb;
> +	gopobj->fb = map_sysmem(fb_base, fb_size);

Could you, please, add a description of the side effects of the 'len'
parameter of map_sysmem() in doc/arch/sandbox/sandbox.rst. It seems to
be ignored, see

include/mapmem.h:16
arch/sandbox/include/asm/io.h:40

So maybe just remove it?

Best regards

Heinrich

>
>   	return EFI_SUCCESS;
>   }