[PATCH v3 5/9] board_f: Fix corruption of relocaddr

[Simon Glass]

Hi Tom,

On Mon, 31 Jul 2023 at 15:06, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Jul 31, 2023 at 02:49:06PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 31 Jul 2023 at 14:45, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Mon, Jul 31, 2023 at 02:37:04PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 31 Jul 2023 at 13:32, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Jul 31, 2023 at 09:59:56AM -0600, Simon Glass wrote:
> > > > >
> > > > > > When the video framebuffer comes from the bloblist, we should not change
> > > > > > relocaddr to this address, since it interfers with the normal memory
> > > > > > allocation.
> > > > > >
> > > > > > This fixes a boot loop in qemu-x86_64
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> > > > > > Suggested-by: Nikhil M Jain <n-jain1 at ti.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v3:
> > > > > > - Reword the Kconfig help as suggested
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Add a Kconfig as the suggested conditional did not work
> > > > > >
> > > > > >  common/board_f.c      | 3 ++-
> > > > > >  drivers/video/Kconfig | 9 +++++++++
> > > > > >  2 files changed, 11 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/common/board_f.c b/common/board_f.c
> > > > > > index 7d2c380e91e..5173d0a0c2d 100644
> > > > > > --- a/common/board_f.c
> > > > > > +++ b/common/board_f.c
> > > > > > @@ -419,7 +419,8 @@ static int reserve_video(void)
> > > > > >               if (!ho)
> > > > > >                       return log_msg_ret("blf", -ENOENT);
> > > > > >               video_reserve_from_bloblist(ho);
> > > > > > -             gd->relocaddr = ho->fb;
> > > > > > +             if (IS_ENABLED(CONFIG_VIDEO_RESERVE_SPL))
> > > > > > +                     gd->relocaddr = ho->fb;
> > > > > >       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> > > > > >               ulong addr;
> > > > > >               int ret;
> > > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > > > > > index b41dc60cec5..f2e56204d52 100644
> > > > > > --- a/drivers/video/Kconfig
> > > > > > +++ b/drivers/video/Kconfig
> > > > > > @@ -1106,6 +1106,15 @@ config SPL_VIDEO_REMOVE
> > > > > >         if this  option is enabled video driver will be removed at the end of
> > > > > >         SPL stage, beforeloading the next stage.
> > > > > >
> > > > > > +config VIDEO_RESERVE_SPL
> > > > > > +     bool
> > > > > > +     help
> > > > > > +       This adjusts reserve_video() to redirect memory reservation when it
> > > > > > +       sees a video handoff blob (BLOBLISTT_U_BOOT_VIDEO). This avoids the
> > > > > > +       memory used for framebuffer from being allocated by U-Boot proper,
> > > > > > +       thus preventing any further memory reservations done by U-Boot proper
> > > > > > +       from overwriting the framebuffer.
> > > > > > +
> > > > > >  if SPL_SPLASH_SCREEN
> > > > > >
> > > > > >  config SPL_SPLASH_SCREEN_ALIGN
> > > > >
> > > > > Nothing selects this and it's not offered as a choice, so now we've just
> > > > > broken the original case?
> > > >
> > > > Yes, I should have mentioned that. I'm not sure which board(s) need
> > > > this option selected.
> > > >
> > > > Devarsh, do you know?
> > >
> > > And shouldn't this just be part of the normal flow when we have
> > > SPL_BLOBLIST && BLOBLIST, and not need a new symbol? I feel like I'm
> > > missing something here.
> >
> > Most of the discussion was on the v1 patch.
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20230724145210.304917-5-sjg@chromium.org/
>
> OK, yeah.  It seems like something more needs to be done then since
> "don't flicker the screen" is pretty much always the case if we have
> splash screen / video set up in SPL.  Perhaps the case where that's not
> true should just be treated as the uncommon one, so we can do the
> ram_top suggestion normally?

Yes I think that would be best. Also the video info needs to be fully
filled out to fix the x86 problem.

Regards,
Simon