[PATCH v5 01/12] efi: Correct handling of frame buffer

[Heinrich Schuchardt]

On 11/19/23 20:11, 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.
>
> We could provide a function to read this, but it seems better to just
> document how it works. The original change ignored an explicit comment
> in the video.h file ("Things that are private to the uclass: don't use
> these in the driver") which is why this was missed when the VIDEO_COPY
> feature was added.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> Fixes: 8f661a5b662 ("efi_loader: gop: Expose fb when 32bpp")
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>#

Since this patch
a75cf70d23ac ("efi: Correct handling of frame buffer") the EFI block
image transfer is broken on the sandbox.

Build sandbox_defconfig with CONFIG_EFI_SELFTEST=y.

setenv efi_selftest block image transfer
bootefi selftest

A moving submarine should be shown.

Best regards

Heinrich

> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Rebase to -next
> - Add some more comments to the header file
> - Add fixes tag
>
>   include/video.h          |  9 ++++++---
>   lib/efi_loader/efi_gop.c | 12 +++++++-----
>   2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/include/video.h b/include/video.h
> index 5048116a3d57..4d8df9baaada 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -21,9 +21,12 @@ struct udevice;
>    * @align: Frame-buffer alignment, indicating the memory boundary the frame
>    *	buffer should start on. If 0, 1MB is assumed
>    * @size: Frame-buffer size, in bytes
> - * @base: Base address of frame buffer, 0 if not yet known
> - * @copy_base: Base address of a hardware copy of the frame buffer. See
> - *	CONFIG_VIDEO_COPY.
> + * @base: Base address of frame buffer, 0 if not yet known. If CONFIG_VIDEO_COPY
> + *	is enabled, this is the software copy, so writes to this will not be
> + *	visible until vidconsole_sync_copy() is called. If CONFIG_VIDEO_COPY is
> + *	disabled, this is the hardware framebuffer.
> + * @copy_base: Base address of a hardware copy of the frame buffer. If
> + *	CONFIG_VIDEO_COPY is disabled, this is not used.
>    * @copy_size: Size of copy framebuffer, used if @size is 0
>    * @hide_logo: Hide the logo (used for testing)
>    */
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 778b693f983a..a09db31eb465 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;
>
>   	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);
>
>   	return EFI_SUCCESS;
>   }