[PATCH v3 5/9] board_f: Fix corruption of relocaddr

[Devarsh Thakkar]

Hi Tom, Simon,

Thanks for sharing all the information.

On 01/08/23 02:39, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 31 Jul 2023 at 15:06, Tom Rini <trini at konsulko.com> wrote:
>>
>> On Mon, Jul 31, 2023 at 02:49:06PM -0600, Simon Glass wrote:
>>> Hi Tom,
>>>
>>> On Mon, 31 Jul 2023 at 14:45, Tom Rini <trini at konsulko.com> wrote:
>>>>
>>>> On Mon, Jul 31, 2023 at 02:37:04PM -0600, Simon Glass wrote:
>>>>> Hi Tom,
>>>>>
>>>>> On Mon, 31 Jul 2023 at 13:32, Tom Rini <trini at konsulko.com> wrote:
>>>>>>
>>>>>> On Mon, Jul 31, 2023 at 09:59:56AM -0600, Simon Glass wrote:
>>>>>>
>>>>>>> When the video framebuffer comes from the bloblist, we should not change
>>>>>>> relocaddr to this address, since it interfers with the normal memory
>>>>>>> allocation.
>>>>>>>
>>>>>>> This fixes a boot loop in qemu-x86_64
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
>>>>>>> Suggested-by: Nikhil M Jain <n-jain1 at ti.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Reword the Kconfig help as suggested
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Add a Kconfig as the suggested conditional did not work
>>>>>>>
>>>>>>>  common/board_f.c      | 3 ++-
>>>>>>>  drivers/video/Kconfig | 9 +++++++++
>>>>>>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>>>> index 7d2c380e91e..5173d0a0c2d 100644
>>>>>>> --- a/common/board_f.c
>>>>>>> +++ b/common/board_f.c
>>>>>>> @@ -419,7 +419,8 @@ static int reserve_video(void)
>>>>>>>               if (!ho)
>>>>>>>                       return log_msg_ret("blf", -ENOENT);
>>>>>>>               video_reserve_from_bloblist(ho);
>>>>>>> -             gd->relocaddr = ho->fb;
>>>>>>> +             if (IS_ENABLED(CONFIG_VIDEO_RESERVE_SPL))
>>>>>>> +                     gd->relocaddr = ho->fb;
>>>>>>>       } else if (CONFIG_IS_ENABLED(VIDEO)) {
>>>>>>>               ulong addr;
>>>>>>>               int ret;
>>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>>>>>> index b41dc60cec5..f2e56204d52 100644
>>>>>>> --- a/drivers/video/Kconfig
>>>>>>> +++ b/drivers/video/Kconfig
>>>>>>> @@ -1106,6 +1106,15 @@ config SPL_VIDEO_REMOVE
>>>>>>>         if this  option is enabled video driver will be removed at the end of
>>>>>>>         SPL stage, beforeloading the next stage.
>>>>>>>
>>>>>>> +config VIDEO_RESERVE_SPL
>>>>>>> +     bool
>>>>>>> +     help
>>>>>>> +       This adjusts reserve_video() to redirect memory reservation when it
>>>>>>> +       sees a video handoff blob (BLOBLISTT_U_BOOT_VIDEO). This avoids the
>>>>>>> +       memory used for framebuffer from being allocated by U-Boot proper,
>>>>>>> +       thus preventing any further memory reservations done by U-Boot proper
>>>>>>> +       from overwriting the framebuffer.
>>>>>>> +
>>>>>>>  if SPL_SPLASH_SCREEN
>>>>>>>
>>>>>>>  config SPL_SPLASH_SCREEN_ALIGN
>>>>>>
>>>>>> Nothing selects this and it's not offered as a choice, so now we've just
>>>>>> broken the original case?
>>>>>
>>>>> Yes, I should have mentioned that. I'm not sure which board(s) need
>>>>> this option selected.
>>>>>
>>>>> Devarsh, do you know?
>>>>
>>>> And shouldn't this just be part of the normal flow when we have
>>>> SPL_BLOBLIST && BLOBLIST, and not need a new symbol? I feel like I'm
>>>> missing something here.
>>>
>>> Most of the discussion was on the v1 patch.
>>>
>>> https://patchwork.ozlabs.org/project/uboot/patch/20230724145210.304917-5-sjg@chromium.org/
>>
>> OK, yeah.  It seems like something more needs to be done then since
>> "don't flicker the screen" is pretty much always the case if we have
>> splash screen / video set up in SPL.  Perhaps the case where that's not
>> true should just be treated as the uncommon one, so we can do the
>> ram_top suggestion normally?
> 

The gd->relocaddr points to ram_top at the start and framebuffer is not the
first region, I think TLB table is reserved first and then the framebuffer, so
I am not sure if it is good idea to move ram_top since old ram_top is already
used for reserving page table.

As per my opinion
https://patchwork.ozlabs.org/project/uboot/patch/20230801140414.76216-1-devarsht@ti.com/
should suffice to fix this issue ?

Simon,
Could you please try with above patch once? ?
In your case ,
gd->relocaddr=c0000000, ho->fb=d0000000

so the relocaddr is already below the framebuffer so condition won't hold true
and relocaddr won't get updated.

> Yes I think that would be best. Also the video info needs to be fully
> filled out to fix the x86 problem.
> 

Sorry I didn't get this, As i understand by the time video_reserve is called
only driver is bound but not yet probed and I think below fields are only
filled after driver is probed, this for most video drivers as I see.

        u32 size;
        u16 xsize;
        u16 ysize;
        u32 line_length;

So all these fields are zero in the handoff structure.

Kindly let me know if any queries or issues faced.

Regards
Devarsh

> Regards,
> Simon