[PATCH] efi: Correct handling of frame buffer

[Simon Glass]

Hi Heinrich,

On Fri, 25 Aug 2023 at 16:06, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

I haven't seen an EFI app writing text output and graphics output at
the same time. Probably with good reason!

>
> What triggers copying from the copy buffer to the hardware buffer?
>
> Where is the copy function implemented?

It is in vidconsole_sync_copy() at present, but Alper's series is
going to expand things a bit.

Regards,
Simon


>
> >
> > 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;
> >   }
>