Hi Simon,
On 15/08/23 20:14, Simon Glass wrote:
Hi Devarsh,
On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar devarsht@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.cn@gmail.com wrote:
On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote:
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote:
On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") > Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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,
2) Also per current reserve flow in u-boot using the reserve_video helper it reserve page table first and then framebuffer, so we would possibly have to change that order to do framebuffer reservation first at the top and that's why we required this condition patch at first place.
3) Also If we are thinking on lines of putting constraints then, I think current constraint i.e. "Relocation will always happen below the framebuffer area so that there is no overlap" as we put in the patch https://lore.kernel.org/all/20230801140414.76216-1-devarsht@ti.com/ seems little better to me as it gives little bit of flexiblity and avoids problem mentinoed in 2)
4) Also as per your feedback on this patch at https://lore.kernel.org/all/CAPnjgZ03iQ6iu3gz=Xkp1vHS43=1XXEk2TKdYgj7v4FhxVb... you mentioned a scenario where although framebuffer region is below current relocation pointer but the gap between the two is 1Gb so we don't require to move relocation pointer and instead continue reservations without worrying about overlap due to huge gap, so to address such scenarios I thought to go with this Kconfig approach.
5) From my POV, Ideal solution could be to have some function that estimates memory requirements for reservation and relocation well in advance and then we can take decision on whether to continue reservation from current relocaddr or move it after receiving the framebuffer address from videoblob but I am not sure how much feasible is it at this point and hence I suggested this Kconfig based approach.
Regards Devarsh
Regards, Simon