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

[Devarsh Thakkar]

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.

Regards
Devarsh

>>
>> 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