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) //It means framebuffer are is overlapping with current relocaddr so update relocaddr gd->relocaddr = ho->fb 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?
Regards, Simon
Regards Devarsh
The SPL frame buffer needs to be considered pre-allocated. It makes no sense to set relocaddr to this value. It will break all sorts of things. E.g. qemu-x86_64 crashes with this.
Regards Devarsh
} else if (CONFIG_IS_ENABLED(VIDEO)) { ulong addr; int ret;Regards, Simon