Hi Simon,
On 22/09/23 23:57, Simon Glass wrote:
Hi Devarsh,
On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar <devar...@ti.com> wrote:
Hi Simon,
On 11/09/23 04:44, Simon Glass wrote:
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?
TBH, I am not fully able to visualize how this fits current arch :
So instead of blob address will we be passing ram_top inside the video blob ?
.Or we will be using separate ram_top blob passing from SPL to u-boot ?
Yes, a separate ram_top in the bloblist, not in the video blob.
Ok, but we still need to have video blob too for conveying frame-buffer
information, right ?
Also, just wanted to check if we really require a ram_top blob to be
passed b/w SPL to u-boot, I thought ram_top addr is know by both stages
before-hand if so then, why pass the ram_top blob separately?
Are we planning to enforce some restriction/hardcoding for framebuffer to be
reserved at specific address (near top of RAM) or it would be just a general
guideline to keep framebuffer near the RAM top ?
Well it makes sense to put it at the top, since U-Boot relocations
itself to the top. >
Currently don't see video_reserve API put such constraint and user is free to
call it anywhere and it just reserve after previously reserved areas. Now,
with this approach I guess we would deviate from the agnostic behavior of
video_reserve API then if we plan to update the same API ?
Also u-boot proper starts reserving regions for MMU and few other stuff much
before reserving framebuffers so by the time we receive the blob containing
updated ram_top, we would have already reserved those regions from old ram_top
so moving the ram_top here seems little counter-intuitive to me. In such
scenario, as per my opinion better option seems to be moving he gd->relocaddr
instead.
Lastly, I think as much the user keep framebuffer way from ram_top that much
memory will be lost even with this approach (as ram_top will be moved after
framebuffer for u-boot proper) and same behavior will be observed with
https://lore.kernel.org/all/20230801140414.76216-1-devar...@ti.com/ too
but if we are planning to put just a general guideline to user to keep
framebuffer near the RAM top then to me above patch looks much simpler than
moving the ram_top.
I don't really have any more to say here. This is all just confusing
and I don't think it needs to be. If SPL allocs stuff, I believe it
should do so at the top of memory, then tell U-Boot about it, so
U-Boot's reservations can happen below that address.
Ok, thanks for explaining your design, I understand your suggestion here
and the design you are proposing i.e. of putting framebuffer at the
ram_top, I am just thinking on that if we are to go with this then what
is the simplest way to fit it with current u-boot architecture where we
are using same API i.e. video_reserve at SPL stage and u-boot proper for
both reserving the memory, passing the blob and catching the blob for
simplicity.
I think we can probably achieve the same thing what you are proposing,
if at u-boot proper also we follow the same paradigm i.e. reserve the
video memory first (or for that matter any region which is reserve-able
by SPL), this way if u-boot call reserve_video function first in this
sequence
https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?ref_type=tags#L943
then it will also trigger a call to video_reserve function which can
read the blob and move the relocaddr as it is already doing (which is
actually the ram_top since reserve_video is getting called at the start
with this) after the reserved video area thus avoiding any gaps between
reservations. This way we don't require to pass ram_top via blob.
Is that possible to update sequence at
https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?ref_type=tags#L943
to reserve video memory first since it is also shared/reserve-able with
previous stage ?
If so then I think we probably don't require much change there-after as
blob will be read first and further reservation will only start below
the frame-buffer area even with current video_reserve API.
Kindly let me know your opinion.
Regards
Devarsh
Regards,
Simon
