Hi, +Tom Rini for comment
On Fri, 28 Jul 2023 at 02:35, Nikhil M Jain <n-ja...@ti.com> 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) > >>>> //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. How about using a Kconfig instead? It seems to me that the idea of SPL and U-Boot happening to place the video buffer at the same place is more the exception than the rule. I've tried to explain that the sequence of reserve...() calls that U-Boot proper does really should not be molested by some data arriving from SPL. If you don't want to do a Kconfig, then please send a patch that works for you and I'll test it, then update my series. Regards, Simon