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

[Tom Rini]

On Fri, Aug 04, 2023 at 01:10:10PM +0530, Devarsh Thakkar wrote:
> Hi Simon, Tom, Bin,
> 
> Thanks for the quick feedback.
> 
> On 04/08/23 04:58, Simon Glass wrote:
> > 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...
> > 
> 
> Thanks Simon, I understand your point here but I think this problem is still
> there even with other approaches like separate Kconfig or SPL blob passing to
> indicate relocaddr update, as though even in that case SPL will need some base
> to take a decision on whether u-boot proper will require to move relocaddr or not.
> 
> For e.g. in this case there is a gap of ~1Gb between relocaddr and framebuffer
> and so we don't want to move relocaddr here since we assume that most likely
> ~1Gb is enough for u-boot to carry out and complete it's reservations without
> touching the framebuffer area, but this assumption may not hold true if the
> gap is in Mb's or Kb's.
> 
> To resolve this, the maximum value required for u-boot to carry out
> reservations need to be estimated and then we can make this condition more
> intelligent :
> 
>  if (gd->relocaddr - gd->video_top < UBOOT_MAX_RESERVE_SIZE)
>      gd->relocaddr = ho->fb;
> 
> 
> If above looks good then, is it possible to estimate worst case common value
> for UBOOT_MAX_RESERVE_SIZE ? Or we want vendors to choose it say using a
> separate Kconfig say CONFIG_UBOOT_MAX_RESERVE_SIZE ?
> 
> Kindly let me know your feedback on above approach, I can send a V2 based on it.
> 
> Also to add, I think different vendors may choose to use different addresses
> for reserving framebuffer at SPL stage and I am not sure if one approach be
> labelled as more general above other. For e.g. Some vendors may follow same
> scheme as u-boot proper by calling reserve_video at SPL too and keep it near
> ram_top, some may keep it little away from it and some may keep it even farther.

My main comment right now is that by "vendors" I hope you mean "SoC
vendors" and not "platform vendors".  Within a given SoC family the
address picked should be right, and if it needs adjusting (say larger
display resolution) then it must be well documented how to calculate and
in what direction it is safe to change it towards.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230804/5e917539/attachment.sig>