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@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@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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.

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@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@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@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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...

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?

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@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@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@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@chromium.org > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") > Suggested-by: Nikhil M Jain n-jain1@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...

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

On Thu, Aug 03, 2023 at 07:03:47PM +0800, Bin Meng wrote:

On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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.cn@gmail.com

applied to u-boot-x86, thanks!

This isn't the right path, we need to test: https://patchwork.ozlabs.org/project/uboot/patch/20230801140414.76216-1-deva...

Hi Bin,

On Thu, 3 Aug 2023 at 08:19, Bin Meng bmeng.cn@gmail.com wrote:

Hi Tom, Simon,

On Thu, Aug 3, 2023 at 9:13 PM Tom Rini trini@konsulko.com wrote:

...

On Thu, Aug 03, 2023 at 07:03:47PM +0800, Bin Meng wrote:

...

On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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.cn@gmail.com

applied to u-boot-x86, thanks!

This isn't the right path, we need to test: https://patchwork.ozlabs.org/project/uboot/patch/20230801140414.76216-1-deva...

I can drop this commit and apply Devarsh's one, but looks there are some arguments.

Let me know which one is the preferred approach.

We can take that one at least for now...I have spent enough time trying to explain the problem.

Regards, Simon

Hi Devarsh, Nikhil, Tom,

On Wed, 9 Aug 2023 at 09:29, Bin Meng bmeng.cn@gmail.com wrote:

On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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.cn@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?

Regards, Simon

Hi Devarsh,

On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar devarsht@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.cn@gmail.com wrote:

...

On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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?

Regards, Simon

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 devarsht@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.cn@gmail.com wrote:

...

On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote:

...

On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote: > > On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org >> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") >> Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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.

Hi Simon,

On 11/09/23 04:44, Simon Glass wrote:

Hi Devarsh,

On Thu, 17 Aug 2023 at 09:10, Tom Rini trini@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 devarsht@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.cn@gmail.com wrote: > > On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote: >> >> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote: >>> >>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org >>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") >>>> Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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-devarsht@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

Hi Simon,

On 22/09/23 23:57, Simon Glass wrote:

Hi Devarsh,

On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar devarsht@ti.com wrote:

...

Hi Simon,

On 11/09/23 04:44, Simon Glass wrote:

...

Hi Devarsh,

On Thu, 17 Aug 2023 at 09:10, Tom Rini trini@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 devarsht@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.cn@gmail.com wrote: >>> >>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com wrote: >>>> >>>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com wrote: >>>>> >>>>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org >>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") >>>>>> Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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 ?

Yes, a separate ram_top in the bloblist, not in the video blob.

Ok, but we still need to have video blob too for conveying frame-buffer information, right ?

Also, just wanted to check if we really require a ram_top blob to be passed b/w SPL to u-boot, I thought ram_top addr is know by both stages before-hand if so then, why pass the ram_top blob separately?

...

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 ?

Well it makes sense to put it at the top, since U-Boot relocations itself to the 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-devarsht@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.

I don't really have any more to say here. This is all just confusing and I don't think it needs to be. If SPL allocs stuff, I believe it should do so at the top of memory, then tell U-Boot about it, so U-Boot's reservations can happen below that address.

Ok, thanks for explaining your design, I understand your suggestion here and the design you are proposing i.e. of putting framebuffer at the ram_top, I am just thinking on that if we are to go with this then what is the simplest way to fit it with current u-boot architecture where we are using same API i.e. video_reserve at SPL stage and u-boot proper for both reserving the memory, passing the blob and catching the blob for simplicity.

I think we can probably achieve the same thing what you are proposing, if at u-boot proper also we follow the same paradigm i.e. reserve the video memory first (or for that matter any region which is reserve-able by SPL), this way if u-boot call reserve_video function first in this sequence https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?re... then it will also trigger a call to video_reserve function which can read the blob and move the relocaddr as it is already doing (which is actually the ram_top since reserve_video is getting called at the start with this) after the reserved video area thus avoiding any gaps between reservations. This way we don't require to pass ram_top via blob.

Is that possible to update sequence at https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?re... to reserve video memory first since it is also shared/reserve-able with previous stage ? If so then I think we probably don't require much change there-after as blob will be read first and further reservation will only start below the frame-buffer area even with current video_reserve API.

Kindly let me know your opinion.

Regards Devarsh

Regards, Simon

Hi Simon, Tom,

On 10/10/23 20:27, Simon Glass wrote:

Hi Devarsh,

On Fri, 22 Sept 2023 at 15:49, Devarsh Thakkar devarsht@ti.com wrote:

...

Hi Simon,

On 22/09/23 23:57, Simon Glass wrote:

...

Hi Devarsh,

On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar devarsht@ti.com wrote:

...

Hi Simon,

On 11/09/23 04:44, Simon Glass wrote:

...

Hi Devarsh,

On Thu, 17 Aug 2023 at 09:10, Tom Rini trini@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 devarsht@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.cn@gmail.com wrote: >>>>> >>>>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng bmeng.cn@gmail.com

wrote:

...

...

...

...

...

>>>>>> >>>>>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng bmeng.cn@gmail.com

wrote:

...

...

...

...

...

>>>>>>> >>>>>>> On Tue, Aug 1, 2023 at 12:00 AM Simon Glass sjg@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 sjg@chromium.org >>>>>>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info

from SPL to u-boot")

...

...

...

...

...

>>>>>>>> Suggested-by: Nikhil M Jain n-jain1@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.cn@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-3de2316aac34@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 ?

Yes, a separate ram_top in the bloblist, not in the video blob.

Ok, but we still need to have video blob too for conveying frame-buffer information, right ?

Yes

...

Also, just wanted to check if we really require a ram_top blob to be passed b/w SPL to u-boot, I thought ram_top addr is know by both stages before-hand if so then, why pass the ram_top blob separately?

I don't believe it can be known at build time, if that is what you mean. At least not in general.

...

...

...

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 ?

Well it makes sense to put it at the top, since U-Boot relocations itself to the 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-devarsht@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.

I don't really have any more to say here. This is all just confusing and I don't think it needs to be. If SPL allocs stuff, I believe it should do so at the top of memory, then tell U-Boot about it, so U-Boot's reservations can happen below that address.

Ok, thanks for explaining your design, I understand your suggestion here and the design you are proposing i.e. of putting framebuffer at the ram_top, I am just thinking on that if we are to go with this then what is the simplest way to fit it with current u-boot architecture where we are using same API i.e. video_reserve at SPL stage and u-boot proper for both reserving the memory, passing the blob and catching the blob for simplicity.

I think we can probably achieve the same thing what you are proposing, if at u-boot proper also we follow the same paradigm i.e. reserve the video memory first (or for that matter any region which is reserve-able by SPL), this way if u-boot call reserve_video function first in this sequence

https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?re...

...

then it will also trigger a call to video_reserve function which can read the blob and move the relocaddr as it is already doing (which is actually the ram_top since reserve_video is getting called at the start with this) after the reserved video area thus avoiding any gaps between reservations. This way we don't require to pass ram_top via blob.

Is that possible to update sequence at

https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc4/common/board_f.c?re...

...

to reserve video memory first since it is also shared/reserve-able with previous stage ? If so then I think we probably don't require much change there-after as blob will be read first and further reservation will only start below the frame-buffer area even with current video_reserve API.

Kindly let me know your opinion.

Sure, it is worth a try. I don't see any problem with rearranging the memory reservations.

Regards, Simon

Thanks for the feedback, as suggested and discussed, I moved the fb allocation at the end of RAM for SPL and catching bloblist at the start in u-boot proper so that it doesn't impact u-boot's reservation sequence and posted https://lore.kernel.org/all/20231016160611.1353458-1-devarsht@ti.com/

Could you please have a look at these series and share your opinion ?

P.S I decided against changing reservation sequence at u-boot proper though as thought that this might potentially break backward compatibility with various device-trees in linux kernel which are reserving hard-coded framebuffer addresses based on current sequence of reservation (i.e. video memory being reserved after page table)

If the approach looks good, then I will do some more testing before sending out again.

Regards Devarsh