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 > >> 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. Regards, Simon