Hi Heinrich,
On Fri, 25 Aug 2023 at 16:06, Heinrich Schuchardt <xypron.g...@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 <s...@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;
> > }
>
