Hi Devarsh,
On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar <devar...@ti.com> wrote:
>
> Hi Simon, Tom,
>
> On 15/08/23 04:13, Simon Glass wrote:
> > Hi Devarsh, Nikhil, Tom,
> >
> > On Wed, 9 Aug 2023 at 09:29, Bin Meng <bmeng...@gmail.com> wrote:
> >>
> >> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng <bmeng...@gmail.com> wrote:
> >>>
> >>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng <bmeng...@gmail.com> wrote:
> >>>>
> >>>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass <s...@chromium.org> wrote:
> >>>>>
> >>>>> When the video framebuffer comes from the bloblist, we should not change
> >>>>> relocaddr to this address, since it interfers with the normal memory
> >>>>
> >>>> typo: interferes
> >>>>
> >>>>> allocation.
> >>>>>
> >>>>> This fixes a boot loop in qemu-x86_64
> >>>>>
> >>>>> Signed-off-by: Simon Glass <s...@chromium.org>
> >>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> >>>>> to u-boot")
> >>>>> Suggested-by: Nikhil M Jain <n-ja...@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
> >>>>
> >>>> Reviewed-by: Bin Meng <bmeng...@gmail.com>
> >>>
> >>> applied to u-boot-x86, thanks!
> >>
> >> Dropped this one from the x86 queue per the discussion.
> >
> > I just wanted to come back to this discussion.
> >
> > Do we have an agreed way forward? Who is waiting for who?
> >
>
> I was waiting on feedback on
> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> but per my opinion, I would prefer to go with "Approach 2" with a
> Kconfig as it looks simpler to me. It would look something like below :
>
> if (gd->relocaddr > (unsigned long)ho->fb) {
> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>
> /* Relocate after framebuffer area if nearing too close to it */
> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> gd->relocaddr = ho->fb;
> }
>
> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> -> This describes minimum gap to keep between framebuffer address and
> relocation address to avoid overlap when framebuffer address used by
> blob is below the current relocation address
>
> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> default value set to 100Mb
>
> -> SoC specific Vendors can override this in their defconfigs to a
> custom value if they feel 100Mb is not enough
>
> Also probably we can have some debug/error prints in the code to show
> overlap happened (or is going happen) so that users can fine tune this
> Kconfig if they got it wrong at first place.
>
> I can re-spin updated patch if we are aligned on this,
> Kindly let me know your opinion.
I'm just nervous about the whole idea, TBH. Perhaps I am missing some
documentation on how people are supposed to lay out memory in SPL and
U-Boot properr, but I'm not really aware of any guidance we give.
Perhaps we should say that the SPL frame buffer should be at the top
of memory, and U-Boot's reserve area should start below that?
Regards,
Simon
