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-jain1@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 devarsht@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 devarsht@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 sjg@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?re...
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