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