* [PATCH v3 5/9] board_f: Fix corruption of relocaddr
@ 2023-07-31 15:59 Simon Glass
2023-07-31 19:32 ` Tom Rini
2023-08-03 10:37 ` Bin Meng
0 siblings, 2 replies; 26+ messages in thread
From: Simon Glass @ 2023-07-31 15:59 UTC (permalink / raw)
To: U-Boot Mailing List
Cc: Bin Meng, Simon Glass, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 15:59 [PATCH v3 5/9] board_f: Fix corruption of relocaddr Simon Glass
@ 2023-07-31 19:32 ` Tom Rini
2023-07-31 20:37 ` Simon Glass
2023-08-03 10:37 ` Bin Meng
1 sibling, 1 reply; 26+ messages in thread
From: Tom Rini @ 2023-07-31 19:32 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]
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?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 19:32 ` Tom Rini
@ 2023-07-31 20:37 ` Simon Glass
2023-07-31 20:44 ` Tom Rini
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-07-31 20:37 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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?
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 20:37 ` Simon Glass
@ 2023-07-31 20:44 ` Tom Rini
2023-07-31 20:49 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Tom Rini @ 2023-07-31 20:44 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]
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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 20:44 ` Tom Rini
@ 2023-07-31 20:49 ` Simon Glass
2023-07-31 21:06 ` Tom Rini
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-07-31 20:49 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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@chromium.org/
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 20:49 ` Simon Glass
@ 2023-07-31 21:06 ` Tom Rini
2023-07-31 21:09 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Tom Rini @ 2023-07-31 21:06 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
[-- Attachment #1: Type: text/plain, Size: 3867 bytes --]
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@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?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 21:06 ` Tom Rini
@ 2023-07-31 21:09 ` Simon Glass
2023-08-01 14:29 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-07-31 21:09 UTC (permalink / raw)
To: Tom Rini
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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@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?
Yes I think that would be best. Also the video info needs to be fully
filled out to fix the x86 problem.
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 21:09 ` Simon Glass
@ 2023-08-01 14:29 ` Devarsh Thakkar
2023-08-03 14:03 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Devarsh Thakkar @ 2023-08-01 14:29 UTC (permalink / raw)
To: Simon Glass, Tom Rini
Cc: U-Boot Mailing List, Bin Meng, Nikhil M Jain, Anatolij Gustschin,
Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese, Luthra, Jai, Bhatia, Aradhya,
Raghavendra, Vignesh, Bajjuri, Praneeth
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@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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-07-31 15:59 [PATCH v3 5/9] board_f: Fix corruption of relocaddr Simon Glass
2023-07-31 19:32 ` Tom Rini
@ 2023-08-03 10:37 ` Bin Meng
2023-08-03 11:03 ` Bin Meng
1 sibling, 1 reply; 26+ messages in thread
From: Bin Meng @ 2023-08-03 10:37 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-03 10:37 ` Bin Meng
@ 2023-08-03 11:03 ` Bin Meng
2023-08-03 13:13 ` Tom Rini
2023-08-09 15:29 ` Bin Meng
0 siblings, 2 replies; 26+ messages in thread
From: Bin Meng @ 2023-08-03 11:03 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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!
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-03 11:03 ` Bin Meng
@ 2023-08-03 13:13 ` Tom Rini
2023-08-03 14:19 ` Bin Meng
2023-08-09 15:29 ` Bin Meng
1 sibling, 1 reply; 26+ messages in thread
From: Tom Rini @ 2023-08-03 13:13 UTC (permalink / raw)
To: Bin Meng
Cc: Simon Glass, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Devarsh Thakkar, Michal Suchanek,
Ovidiu Panait, Pali Rohár, Rasmus Villemoes, Stefan Roese
[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]
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-devarsht@ti.com/
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-01 14:29 ` Devarsh Thakkar
@ 2023-08-03 14:03 ` Simon Glass
0 siblings, 0 replies; 26+ messages in thread
From: Simon Glass @ 2023-08-03 14:03 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Tom Rini, U-Boot Mailing List, Bin Meng, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese, Luthra, Jai,
Bhatia, Aradhya, Raghavendra, Vignesh, Bajjuri, Praneeth
Hi Devarsh,
On Tue, 1 Aug 2023 at 08:30, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> 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@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 replied to the patch. It is a strange heuristic, IMO, and we
should avoid this sort of thing. But if that is the only acceptable
solution, we can go with it.
>
> > 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.
Right, so you need to fill these in when the video is probed in SPL,
not before. By leaving them out, you are breaking U-Boot proper which
is expecting these values.
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-03 13:13 ` Tom Rini
@ 2023-08-03 14:19 ` Bin Meng
2023-08-03 23:28 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Bin Meng @ 2023-08-03 14:19 UTC (permalink / raw)
To: Tom Rini
Cc: Simon Glass, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Devarsh Thakkar, Michal Suchanek,
Ovidiu Panait, Pali Rohár, Rasmus Villemoes, Stefan Roese
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-devarsht@ti.com/
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.
Regards,
Bin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-03 14:19 ` Bin Meng
@ 2023-08-03 23:28 ` Simon Glass
0 siblings, 0 replies; 26+ messages in thread
From: Simon Glass @ 2023-08-03 23:28 UTC (permalink / raw)
To: Bin Meng
Cc: Tom Rini, U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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-devarsht@ti.com/
>
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-03 11:03 ` Bin Meng
2023-08-03 13:13 ` Tom Rini
@ 2023-08-09 15:29 ` Bin Meng
2023-08-14 22:43 ` Simon Glass
1 sibling, 1 reply; 26+ messages in thread
From: Bin Meng @ 2023-08-09 15:29 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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.
Regards,
Bin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-09 15:29 ` Bin Meng
@ 2023-08-14 22:43 ` Simon Glass
2023-08-15 9:23 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-08-14 22:43 UTC (permalink / raw)
To: Bin Meng
Cc: U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Devarsh Thakkar, Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-14 22:43 ` Simon Glass
@ 2023-08-15 9:23 ` Devarsh Thakkar
2023-08-15 14:44 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Devarsh Thakkar @ 2023-08-15 9:23 UTC (permalink / raw)
To: Simon Glass, Bin Meng, trini
Cc: U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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.
Regards
Devarsh
> Regards,
> Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-15 9:23 ` Devarsh Thakkar
@ 2023-08-15 14:44 ` Simon Glass
2023-08-16 15:46 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-08-15 14:44 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Bin Meng, trini, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-15 14:44 ` Simon Glass
@ 2023-08-16 15:46 ` Devarsh Thakkar
2023-08-17 15:10 ` Tom Rini
0 siblings, 1 reply; 26+ messages in thread
From: Devarsh Thakkar @ 2023-08-16 15:46 UTC (permalink / raw)
To: Simon Glass
Cc: Bin Meng, trini, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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,
2) Also per current reserve flow in u-boot using the reserve_video helper it
reserve page table first and then framebuffer, so we would possibly have to
change that order to do framebuffer reservation first at the top and that's
why we required this condition patch at first place.
3) Also If we are thinking on lines of putting constraints then, I think
current constraint i.e. "Relocation will always happen below the framebuffer
area so that there is no overlap" as we put in the patch
https://lore.kernel.org/all/20230801140414.76216-1-devarsht@ti.com/ seems
little better to me as it gives little bit of flexiblity and avoids problem
mentinoed in 2)
4) Also as per your feedback on this patch at
https://lore.kernel.org/all/CAPnjgZ03iQ6iu3gz=Xkp1vHS43=1XXEk2TKdYgj7v4FhxVbj9g@mail.gmail.com/
you mentioned a scenario where although framebuffer region is below current
relocation pointer but the gap between the two is 1Gb so we don't require to
move relocation pointer and instead continue reservations without worrying
about overlap due to huge gap, so to address such scenarios I thought to go
with this Kconfig approach.
5) From my POV, Ideal solution could be to have some function that estimates
memory requirements for reservation and relocation well in advance and then we
can take decision on whether to continue reservation from current relocaddr or
move it after receiving the framebuffer address from videoblob but I am not
sure how much feasible is it at this point and hence I suggested this Kconfig
based approach.
Regards
Devarsh
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-16 15:46 ` Devarsh Thakkar
@ 2023-08-17 15:10 ` Tom Rini
2023-09-10 23:14 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Tom Rini @ 2023-08-17 15:10 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Simon Glass, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
[-- Attachment #1: Type: text/plain, Size: 6138 bytes --]
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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-08-17 15:10 ` Tom Rini
@ 2023-09-10 23:14 ` Simon Glass
2023-09-12 14:35 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-09-10 23:14 UTC (permalink / raw)
To: Tom Rini
Cc: Devarsh Thakkar, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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?
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-09-10 23:14 ` Simon Glass
@ 2023-09-12 14:35 ` Devarsh Thakkar
2023-09-22 18:27 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Devarsh Thakkar @ 2023-09-12 14:35 UTC (permalink / raw)
To: Simon Glass, Tom Rini
Cc: Bin Meng, U-Boot Mailing List, Nikhil M Jain, Anatolij Gustschin,
Michal Suchanek, Ovidiu Panait, Pali Rohár,
Rasmus Villemoes, Stefan Roese
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-09-12 14:35 ` Devarsh Thakkar
@ 2023-09-22 18:27 ` Simon Glass
2023-09-22 21:49 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-09-22 18:27 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Tom Rini, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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.
>
> 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.
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-09-22 18:27 ` Simon Glass
@ 2023-09-22 21:49 ` Devarsh Thakkar
2023-10-10 14:57 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Devarsh Thakkar @ 2023-09-22 21:49 UTC (permalink / raw)
To: Simon Glass
Cc: Tom Rini, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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?ref_type=tags#L943
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?ref_type=tags#L943
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-09-22 21:49 ` Devarsh Thakkar
@ 2023-10-10 14:57 ` Simon Glass
2023-10-16 16:15 ` Devarsh Thakkar
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2023-10-10 14:57 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Tom Rini, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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?ref_type=tags#L943
> 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?ref_type=tags#L943
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
2023-10-10 14:57 ` Simon Glass
@ 2023-10-16 16:15 ` Devarsh Thakkar
0 siblings, 0 replies; 26+ messages in thread
From: Devarsh Thakkar @ 2023-10-16 16:15 UTC (permalink / raw)
To: Simon Glass
Cc: Tom Rini, Bin Meng, U-Boot Mailing List, Nikhil M Jain,
Anatolij Gustschin, Michal Suchanek, Ovidiu Panait,
Pali Rohár, Rasmus Villemoes, Stefan Roese
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?ref_type=tags#L943
>> 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?ref_type=tags#L943
>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-10-16 16:15 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-31 15:59 [PATCH v3 5/9] board_f: Fix corruption of relocaddr Simon Glass
2023-07-31 19:32 ` Tom Rini
2023-07-31 20:37 ` Simon Glass
2023-07-31 20:44 ` Tom Rini
2023-07-31 20:49 ` Simon Glass
2023-07-31 21:06 ` Tom Rini
2023-07-31 21:09 ` Simon Glass
2023-08-01 14:29 ` Devarsh Thakkar
2023-08-03 14:03 ` Simon Glass
2023-08-03 10:37 ` Bin Meng
2023-08-03 11:03 ` Bin Meng
2023-08-03 13:13 ` Tom Rini
2023-08-03 14:19 ` Bin Meng
2023-08-03 23:28 ` Simon Glass
2023-08-09 15:29 ` Bin Meng
2023-08-14 22:43 ` Simon Glass
2023-08-15 9:23 ` Devarsh Thakkar
2023-08-15 14:44 ` Simon Glass
2023-08-16 15:46 ` Devarsh Thakkar
2023-08-17 15:10 ` Tom Rini
2023-09-10 23:14 ` Simon Glass
2023-09-12 14:35 ` Devarsh Thakkar
2023-09-22 18:27 ` Simon Glass
2023-09-22 21:49 ` Devarsh Thakkar
2023-10-10 14:57 ` Simon Glass
2023-10-16 16:15 ` Devarsh Thakkar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.