On 28/07/23 14:05, Nikhil M Jain wrote:
Hi Simon,
On 27/07/23 23:31, Simon Glass wrote:
Hi Nikhil,
On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-ja...@ti.com> wrote:
Hi Simon,
On 27/07/23 06:23, Simon Glass wrote:
Hi Devarsh,
On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devar...@ti.com> wrote:
Hi Simon,
On 26/07/23 02:58, Simon Glass wrote:
Hi Devarsh,
On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devar...@ti.com>
wrote:
Hi Simon,
On 24/07/23 20:22, Simon Glass wrote:
When the video framebuffer comes from the bloblist, we should
not change
relocaddr to this address, since it interfers with the normal
memory
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")
---
common/board_f.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c
index 7d2c380e91e2..5c8646b22283 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -419,7 +419,6 @@ static int reserve_video(void)
if (!ho)
return log_msg_ret("blf", -ENOENT);
video_reserve_from_bloblist(ho);
- gd->relocaddr = ho->fb;
I think this change was done as relocaddr pointer was required to
be updated
to move after frame-buffer reserved area to ensure that any
further memory
reservations done using gd->relocaddr (for e.g. in
reserve_trace/uboot/malloc)
don't overlap with frame-buffer reserved area passed from blob,
so I think
removing this line may cause further memory reservations to
overlap with
reserved framebuffer.
Could you please confirm?
SPL and U-Boot have different memory layouts. The current code is
interrupting U-Boot's careful building up of chunks of memory used
for
different purposes.
But it is possible they could be using similar memory layout for some
components like framebuffer.
For e.g. in our case we are using same video_reserve func in A53
SPL too
and which allocates framebuffer from gd->relocaddr as seen here:
https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427
Even if it is similar, the point is that U-Boot proper needs to do its
own allocation stuff.
Of course, if SPL sets up the video, it will provide the framebuffer
address, but only that. The other addresses need to be done using the
normal mechanism.
The video memory has already been allocated by SPL, so we don't need
to insert a new one here, as your code demonstrates.
Agreed.
But also we have no way of knowing if it is legal to relocate U-Boot
(and various other things) just below the frame buffer chosen by SPL.
Yes, so i suppose your case is that framebuffer address which is
being passed
by SPL is totally disjoint and too far away from gd->relocaddr, for
e.g. it is
at the start (or bottom of DDR) and doesn't interfere with
gd->relocaddr in
any manner since relocaddr points to ramtop (i.e. near to end
address of DDR).
In that case I agree it doesn't make sense to move relocaddr to
ho->fb.
But for the scenario where gd->relocaddr and ho->fb are nearby
there is every
possibility that gd->relocaddr may overlap with framebuffer, also
the code in
reserve_trace, reserve_uboot doesn't have any intelligence or check
that it is
overlapping with framebuffer area or not.
I think one thing that can probably be done here is to have a check
that if
passed framebuffer area falls within current relocaddr region, then
update the
relocaddr else don't touch relocaddr :
if (ho->fb <= gd->relocaddr - ho->size)
Just a small correction here
if (ho->fb >= gd->relocaddr - ho->size)
//It means framebuffer are is overlapping with current
relocaddr so update
relocaddr
gd->relocaddr = ho->fb
We should go ahead with this check because it won't disrupt u-boot's
allocation of memory and will allow both the cases if a platform is
using same memory layout or different memory layout across SPL and
u-boot proper. Below are the logs for both scenarios.
https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df
else
//don't update gd->relocaddr since ho->fb is disjoint to
gd->relocaddr
Could you please share your opinion on this and if above logic
suffice your
case too ?
I don't think this line is needed at all, which is why this patch
removes it. What problem are you seeing?
Across SPL stage and U-boot we are keeping same memory layout and
ensuring that same memory regions are used, this way it doesn't
interfere in the way of u-boot while allocating memory regions for
various purposes. This allowed us to display splash screen without any
flicker across the stages.
Now if you remove the line gd->relocaddr = ho->fb, the frame buffer
region will be used for reserving memory for other purposes which
corrupts the frame buffer.
One solution which we are planning to implement is move the ram_top to a
lower address leaving out a region for video buffer and u-boot can do
the allocation from the new ram_top address without spl video handoff
interfering in the u-boot's allocation of memory.The region above the
ram_top can be used for video.
Present Scenario
+---------------------+ram_top
| |
| page_table |
| |
| |
+---------------------+
| |
| |
| |
| |
| |
| video frame buffer |
| |
| |
| |
| |
| |
| |
+---------------------+
| |
| |
| reserve_trace |
| |
| |
| |
+---------------------+
Proposed Solution
+---------------------+
| |
| |
| |
| |
| |
| video frame buffer |
| |
| |
| |
| |
| |
+---------------------+ram_top
| |
| |
| page_table |
| |
| |
| |
+---------------------+
| |
| |
| reserve_trace |
| |
| |
+---------------------+
Sure, whatever you need to do is fine. You could pass the ram top from
SPL to U-Boot perhaps.
In this solution problem arises when user doesn't want's to hand-off
buffer, the frame buffer region will be reallocated by u-boot or will
require us to hard code buffer address, which we don't want to do.
Regards,
Simon
Thanks,
Nikhil
