Hi Devarsh, On Thu, 17 Aug 2023 at 09:10, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Aug 16, 2023 at 09:16:05PM +0530, Devarsh Thakkar wrote: > > Hi Simon, > > > > On 15/08/23 20:14, Simon Glass wrote: > > > 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? > > > > 1) As per my personal opinion, I don't like putting such constraints and > > would > > instead like to give some flexibility to end user for choosing > > framebuffer area as I earlier mentioned, as for that matter if we are using > > a > > predefined address then there is no need of using framebuffer address on > > videoblob, > > I think this is the wrong direction. We need to offer strong defaults > that shouldn't be deviated without good reason, rather than "pick what > you want". Very few cases will deviate from the defaults, and of those > it's hard to know if they're being changed for the best, or because > someone didn't fully understand the implications and breaks something > else.
So what is next with this? I would like to clean it up...I feel that having SPL pass the top of usable RAM (below the framebuffer) is a reasonable solution. Does constraining things in that way cause any problems for TI? Regards, Simon