Hi Devarsh, On Thu, 3 Aug 2023 at 08:28, Devarsh Thakkar <devar...@ti.com> wrote: > > Hi Simon, > > On 03/08/23 19:32, Simon Glass wrote: > > +Bin Meng > > > > Hi Devarsh, > > > > On Tue, 1 Aug 2023 at 08:04, Devarsh Thakkar <devar...@ti.com> wrote: > >> > >> When passing framebuffer address using bloblist, check > >> that passed address is overlapping with current relocation > >> address, if so move the relocation address after the framebuffer > >> region to avoid overlap. > >> > >> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from > >> SPL to u-boot") > >> Signed-off-by: Devarsh Thakkar <devar...@ti.com> > >> --- > >> common/board_f.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/common/board_f.c b/common/board_f.c > >> index 7d2c380e91..20fa17207a 100644 > >> --- a/common/board_f.c > >> +++ b/common/board_f.c > >> @@ -419,7 +419,10 @@ static int reserve_video(void) > >> if (!ho) > >> return log_msg_ret("blf", -ENOENT); > >> video_reserve_from_bloblist(ho); > >> - gd->relocaddr = ho->fb; > >> + /* Relocate after framebuffer area to avoid overlap */ > >> + if (gd->relocaddr > (unsigned long)ho->fb) > >> + gd->relocaddr = ho->fb; > >> + > >> } else if (CONFIG_IS_ENABLED(VIDEO)) { > >> ulong addr; > >> int ret; > >> -- > >> 2.34.1 > >> > > > > Yes this happens to work with qemu-x86_64. However it depends on the > > SPL frame buffer being below the current allocation area. Why would it > > be below, in general? This seems like a land mine for others to trip > > up on. > > > > Basically to avoid overlap, the thing we are enforcing here is that > relocaddr should always be below framebuffer so that further areas > reserved do not overlap with framebuffer since we decrement relocaddr to > resereve them. If relocaddr is already below framebuffer then there is no > need to update relocaddr as in your case otherwise we have to do it, hence > working for both scenarios.
That's not true in general. Say the frame buffer is at 1GB and U-Boot's top of memory is at 2GB, then this will create a gap in the reservations, so some things are just below 2GB and some others are just below 1GB. There will be no indication of this... > > The way I look at it is at u-boot proper is getting a blob from SPL > with video handoff structure and before carrying out further reservations > u-boot proper being agnostic to the framebuffer address set by SPL should have > some check to make sure there is no overlap with current relocaddr. > > > I think this is similar to suggestion from Tom and you regarding moving the > ram_top if framebuffer is reserved near the ram_top, albeit instead of moving > ram_top I am moving relocaddr which is derived from ram_top. > > Kindly let me know if any queries. It just doesn't make sense to me...if you want to influence U-Boot's reservation, add a handoff blob to tell U-Boot that. Regards, Simon > > Regards > Devarsh > > > Anyway I am OK with this for now since it fixes my problems. I would > > prefer something positive like the Kconfig I provided, since I still > > think it is very weird to interrupt the U-Boot reservation process > > like this.> > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > Regards, > > Simon