Hi Simon,
On 11/09/23 04:44, Simon Glass wrote:
> Hi Devarsh,
>
> On Thu, 17 Aug 2023 at 09:10, Tom Rini <tr...@konsulko.com> wrote:
>>
>> On Wed, Aug 16, 2023 at 09:16:05PM +0530, Devarsh Thakkar wrote:
>>> Hi Simon,
>>>
>>> On 15/08/23 20:14, Simon Glass wrote:
>>>> Hi Devarsh,
>>>>
>>>> On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar <devar...@ti.com> wrote:
>>>>>
>>>>> Hi Simon, Tom,
>>>>>
>>>>> On 15/08/23 04:13, Simon Glass wrote:
>>>>>> Hi Devarsh, Nikhil, Tom,
>>>>>>
>>>>>> On Wed, 9 Aug 2023 at 09:29, Bin Meng <bmeng...@gmail.com> wrote:
>>>>>>>
>>>>>>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng <bmeng...@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng <bmeng...@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass <s...@chromium.org> wrote:
>>>>>>>>>>
>>>>>>>>>> When the video framebuffer comes from the bloblist, we should not
>>>>>>>>>> change
>>>>>>>>>> relocaddr to this address, since it interfers with the normal memory
>>>>>>>>>
>>>>>>>>> typo: interferes
>>>>>>>>>
>>>>>>>>>> allocation.
>>>>>>>>>>
>>>>>>>>>> This fixes a boot loop in qemu-x86_64
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Simon Glass <s...@chromium.org>
>>>>>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from
>>>>>>>>>> SPL to u-boot")
>>>>>>>>>> Suggested-by: Nikhil M Jain <n-ja...@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
>>>>>>>>>
>>>>>>>>> Reviewed-by: Bin Meng <bmeng...@gmail.com>
>>>>>>>>
>>>>>>>> applied to u-boot-x86, thanks!
>>>>>>>
>>>>>>> Dropped this one from the x86 queue per the discussion.
>>>>>>
>>>>>> I just wanted to come back to this discussion.
>>>>>>
>>>>>> Do we have an agreed way forward? Who is waiting for who?
>>>>>>
>>>>>
>>>>> I was waiting on feedback on
>>>>> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
>>>>> but per my opinion, I would prefer to go with "Approach 2" with a
>>>>> Kconfig as it looks simpler to me. It would look something like below :
>>>>>
>>>>> if (gd->relocaddr > (unsigned long)ho->fb) {
>>>>> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>>>>>
>>>>> /* Relocate after framebuffer area if nearing too close to it */
>>>>> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
>>>>> gd->relocaddr = ho->fb;
>>>>> }
>>>>>
>>>>> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
>>>>> -> This describes minimum gap to keep between framebuffer address and
>>>>> relocation address to avoid overlap when framebuffer address used by
>>>>> blob is below the current relocation address
>>>>>
>>>>> -> It would be selected as default when CONFIG_BLOBLIST is selected with
>>>>> default value set to 100Mb
>>>>>
>>>>> -> SoC specific Vendors can override this in their defconfigs to a
>>>>> custom value if they feel 100Mb is not enough
>>>>>
>>>>> Also probably we can have some debug/error prints in the code to show
>>>>> overlap happened (or is going happen) so that users can fine tune this
>>>>> Kconfig if they got it wrong at first place.
>>>>>
>>>>> I can re-spin updated patch if we are aligned on this,
>>>>> Kindly let me know your opinion.
>>>>
>>>> I'm just nervous about the whole idea, TBH. Perhaps I am missing some
>>>> documentation on how people are supposed to lay out memory in SPL and
>>>> U-Boot properr, but I'm not really aware of any guidance we give.
>>>>
>>>> Perhaps we should say that the SPL frame buffer should be at the top
>>>> of memory, and U-Boot's reserve area should start below that?
>>>
>>> 1) As per my personal opinion, I don't like putting such constraints and
>>> would
>>> instead like to give some flexibility to end user for choosing
>>> framebuffer area as I earlier mentioned, as for that matter if we are using
>>> a
>>> predefined address then there is no need of using framebuffer address on
>>> videoblob,
>>
>> I think this is the wrong direction. We need to offer strong defaults
>> that shouldn't be deviated without good reason, rather than "pick what
>> you want". Very few cases will deviate from the defaults, and of those
>> it's hard to know if they're being changed for the best, or because
>> someone didn't fully understand the implications and breaks something
>> else.
>
> So what is next with this? I would like to clean it up...I feel that
> having SPL pass the top of usable RAM (below the framebuffer) is a
> reasonable solution. Does constraining things in that way cause any
> problems for TI?
TBH, I am not fully able to visualize how this fits current arch :
So instead of blob address will we be passing ram_top inside the video blob ?
.Or we will be using separate ram_top blob passing from SPL to u-boot ?
Are we planning to enforce some restriction/hardcoding for framebuffer to be
reserved at specific address (near top of RAM) or it would be just a general
guideline to keep framebuffer near the RAM top ?
Currently don't see video_reserve API put such constraint and user is free to
call it anywhere and it just reserve after previously reserved areas. Now,
with this approach I guess we would deviate from the agnostic behavior of
video_reserve API then if we plan to update the same API ?
Also u-boot proper starts reserving regions for MMU and few other stuff much
before reserving framebuffers so by the time we receive the blob containing
updated ram_top, we would have already reserved those regions from old ram_top
so moving the ram_top here seems little counter-intuitive to me. In such
scenario, as per my opinion better option seems to be moving he gd->relocaddr
instead.
Lastly, I think as much the user keep framebuffer way from ram_top that much
memory will be lost even with this approach (as ram_top will be moved after
framebuffer for u-boot proper) and same behavior will be observed with
https://lore.kernel.org/all/20230801140414.76216-1-devar...@ti.com/ too
but if we are planning to put just a general guideline to user to keep
framebuffer near the RAM top then to me above patch looks much simpler than
moving the ram_top.
Regards
Devarsh
>
> Regards,
> Simon
