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

[Devarsh Thakkar]

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.

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.

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