[PATCH] common: board_f: Move relocation address after framebuffer

[Simon Glass]

Hi Devarsh,

On Thu, 3 Aug 2023 at 08:28, Devarsh Thakkar <devarsht at 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 <devarsht at 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 <devarsht at 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 <sjg at chromium.org>
> >
> > Regards,
> > Simon