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

[Simon Glass]

Hi Devarsh,

On Tue, 1 Aug 2023 at 08:30, Devarsh Thakkar <devarsht at ti.com> wrote:
>
> Hi Tom, Simon,
>
> Thanks for sharing all the information.
>
> On 01/08/23 02:39, Simon Glass wrote:
> > 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?
> >
>
> The gd->relocaddr points to ram_top at the start and framebuffer is not the
> first region, I think TLB table is reserved first and then the framebuffer, so
> I am not sure if it is good idea to move ram_top since old ram_top is already
> used for reserving page table.
>
> As per my opinion
> https://patchwork.ozlabs.org/project/uboot/patch/20230801140414.76216-1-devarsht@ti.com/
> should suffice to fix this issue ?
>
> Simon,
> Could you please try with above patch once? ?
> In your case ,
> gd->relocaddr=c0000000, ho->fb=d0000000
>
> so the relocaddr is already below the framebuffer so condition won't hold true
> and relocaddr won't get updated.

Yes I replied to the patch. It is a strange heuristic, IMO, and we
should avoid this sort of thing. But if that is the only acceptable
solution, we can go with it.

>
> > Yes I think that would be best. Also the video info needs to be fully
> > filled out to fix the x86 problem.
> >
>
> Sorry I didn't get this, As i understand by the time video_reserve is called
> only driver is bound but not yet probed and I think below fields are only
> filled after driver is probed, this for most video drivers as I see.
>
>         u32 size;
>         u16 xsize;
>         u16 ysize;
>         u32 line_length;
>
> So all these fields are zero in the handoff structure.
>
> Kindly let me know if any queries or issues faced.

Right, so you need to fill these in when the video is probed in SPL,
not before. By leaving them out, you are breaking U-Boot proper which
is expecting these values.

Regards,
Simon