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

[Devarsh Thakkar]

Hi,

On 04/08/23 13:10, 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;
> 

Also need to add and consider the relocation aspect here since u-boot
relocates to gd->relocaddr after all reservations so need to make sure it
doesn't touch the framebuffer area after relocation.

To summarize there are different approaches possible here :

Approach 1 :
Have a paradigm that relocaddr should always be below framebuffer so that any
further reservations and also the relocation "never" overlaps with framebuffer
area. If it is not below then it will be moved below framebufer irrespective
of the gap between the two.

This is essentially
https://lore.kernel.org/all/20230801140414.76216-1-devarsht@ti.com/ i.e. below
condition :

+		/* Relocate after framebuffer area to avoid overlap */
+		if (gd->relocaddr > (unsigned long)ho->fb)
+			gd->relocaddr = ho->fb;

The advantage is simplicity here but disadvantage of-course is scenario where
although framebuffer is below relocaddr but gap between framebuffer and
relocaddr is already large and relocaddr will be moved below the framebuffer
here too.

Approach 2:
To get over above disadvantage, don't move relocaddr if there is "enough" gap
between framebuffer and relocaddr. The challenge here is how to estimate or
find on what is "enough" gap as it depends on number and size of reserved
areas and also the size of relocation components. For e.g what to keep the
values of UBOOT_MAX_RESERVE_SIZE, UBOOT_MAX_RELOC_SIZE in below condition
do they need to be Kconfig filled by platform or u-boot need to estimate it.

  if (gd->relocaddr - gd->video_top < UBOOT_MAX_RESERVE_SIZE +
UBOOT_MAX_RELOC_SIZE)
      gd->relocaddr = ho->fb;


Approach 3:
Dynamically check before every reservation and relocation whether we overlap
with any pre-reserved area from blob :

For e.g. have an API called check_spl_reserve_overlap which basically checks
against any pre-reserved components passed of hand-off from previous stage
(right now only video)

This looks more efficient than approaches discussed above but it is more
complex and further more each existing and new reserve/reloc func will need to
call this API before reserving/relocating.

Also it could be further be extended to use
lmb regions in u-boot to mark the reserved areas.

Approach 4:
Base the relocaddr movement decision based on a kconfig.

This is similar to:
https://lore.kernel.org/all/20230730171619.915576-5-sjg@chromium.org/

Approach 5:
Base the relocaddr movement decision based on bloblist for e.g. have a bool
flag passed inside say video blob to indicate the same.

In the last two approaches we are passing the complexity (and power to
dictate) to vendor to make the decision of relocaddr movement in u-boot proper.

Kindly let me know if any preferences on above approaches.

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