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
