Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Simon, Tom,
On 10/10/23 20:27, Simon Glass wrote:
> Hi Devarsh,
>
> On Fri, 22 Sept 2023 at 15:49, Devarsh Thakkar wrote:
>>
>> Hi Simon,
>>
>> On 22/09/23 23:57, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar wrote:
Hi Simon,
On 11/09/23 04:44, Simon Glass wrote:
> Hi Devarsh,
>
> On Thu, 17 Aug 2023 at 09:10, Tom Rini 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
> 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 wrote:
>>>
>>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng
> wrote:
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng
> wrote:
>
> On Tue, Aug 1, 2023 at 12:00 AM 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
>
> typo: interferes
>
>> allocation.
>>
>> This fixes a boot loop in qemu-x86_64
>>
>> Signed-off-by: Simon Glass
>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info
> from SPL to u-boot")
>> Suggested-by: Nikhil M Jain
>> ---
>>
>> 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
applied to u-boot-x86, thanks!
>>>
>>> Dropped this one from the x86 queue per the discussion.
>>
>> I just wanted to come back to this discussion.
>>
>> Do we have an agreed way forward? Who is waiting for who?
>>
>
> I was waiting on feedback on
>
> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> but per my opinion, I would prefer to go with "Approach 2" with a
> Kconfig as it looks simpler to me. It would look something like
> below :
>
> if (gd->relocaddr > (unsigned long)ho->fb) {
>ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>
>/* Relocate after framebuffer area if nearing too close to
> it */
>if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> gd->relocaddr = ho->fb;
> }
>
> Regarding
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh,
On Fri, 22 Sept 2023 at 15:49, Devarsh Thakkar wrote:
>
> Hi Simon,
>
> On 22/09/23 23:57, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar wrote:
> >>
> >> Hi Simon,
> >>
> >> On 11/09/23 04:44, Simon Glass wrote:
> >>> Hi Devarsh,
> >>>
> >>> On Thu, 17 Aug 2023 at 09:10, Tom Rini 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
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 wrote:
> >
> > On Thu, Aug 3, 2023 at 7:03 PM Bin Meng
wrote:
> >>
> >> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng
wrote:
> >>>
> >>> On Tue, Aug 1, 2023 at 12:00 AM 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
> >>>
> >>> typo: interferes
> >>>
> allocation.
>
> This fixes a boot loop in qemu-x86_64
>
> Signed-off-by: Simon Glass
> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info
from SPL to u-boot")
> Suggested-by: Nikhil M Jain
> ---
>
> 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
> >>
> >> applied to u-boot-x86, thanks!
> >
> > Dropped this one from the x86 queue per the discussion.
>
> I just wanted to come back to this discussion.
>
> Do we have an agreed way forward? Who is waiting for who?
>
> >>>
> >>> I was waiting on feedback on
> >>>
https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> >>> but per my opinion, I would prefer to go with "Approach 2" with a
> >>> Kconfig as it looks simpler to me. It would look something like
below :
> >>>
> >>> if (gd->relocaddr > (unsigned long)ho->fb) {
> >>>ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
> >>>
> >>>/* Relocate after framebuffer area if nearing too close to
it */
> >>>if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> >>> gd->relocaddr = ho->fb;
> >>> }
> >>>
> >>> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> >>> -> This describes minimum gap to keep between
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Simon,
On 22/09/23 23:57, Simon Glass wrote:
Hi Devarsh,
On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar wrote:
Hi Simon,
On 11/09/23 04:44, Simon Glass wrote:
Hi Devarsh,
On Thu, 17 Aug 2023 at 09:10, Tom Rini 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 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 wrote:
On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
On Tue, Aug 1, 2023 at 12:00 AM 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
typo: interferes
allocation.
This fixes a boot loop in qemu-x86_64
Signed-off-by: Simon Glass
Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
u-boot")
Suggested-by: Nikhil M Jain
---
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
applied to u-boot-x86, thanks!
Dropped this one from the x86 queue per the discussion.
I just wanted to come back to this discussion.
Do we have an agreed way forward? Who is waiting for who?
I was waiting on feedback on
https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
but per my opinion, I would prefer to go with "Approach 2" with a
Kconfig as it looks simpler to me. It would look something like below :
if (gd->relocaddr > (unsigned long)ho->fb) {
ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
/* Relocate after framebuffer area if nearing too close to it */
if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
gd->relocaddr = ho->fb;
}
Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
-> This describes minimum gap to keep between framebuffer address and
relocation address to avoid overlap when framebuffer address used by
blob is below the current relocation address
-> It would be selected as default when CONFIG_BLOBLIST is selected with
default value set to 100Mb
-> SoC specific Vendors can override this in their defconfigs to a
custom value if they feel 100Mb is not enough
Also probably we can have some debug/error prints in the code to show
overlap happened (or is going happen) so that users can fine tune this
Kconfig if they got it wrong at first place.
I can re-spin updated patch if we are aligned on this,
Kindly let me know your opinion.
I'm just nervous about the whole idea, TBH. Perhaps I am missing some
documentation on how people are supposed to lay out memory in SPL and
U-Boot properr, but I'm not really aware of any guidance we give.
Perhaps we should say that the SPL frame buffer should be at the top
of memory, and U-Boot's reserve area should start below that?
1) As per my personal opinion, I don't like putting such constraints and would
instead like to give some flexibility to end user for choosing
framebuffer area as I earlier mentioned, as for that matter if we are using a
predefined address then there is no need of using framebuffer address on
videoblob,
I think this is the wrong direction. We need to offer strong defaults
that shouldn't be deviated without good reason, rather than "pick what
you want". Very few cases will deviate from
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh,
On Tue, 12 Sept 2023 at 08:35, Devarsh Thakkar wrote:
>
> Hi Simon,
>
> On 11/09/23 04:44, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Thu, 17 Aug 2023 at 09:10, Tom Rini 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 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 wrote:
> >>>
> >>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
>
> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> >
> > On Tue, Aug 1, 2023 at 12:00 AM 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
> >
> > typo: interferes
> >
> >> allocation.
> >>
> >> This fixes a boot loop in qemu-x86_64
> >>
> >> Signed-off-by: Simon Glass
> >> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from
> >> SPL to u-boot")
> >> Suggested-by: Nikhil M Jain
> >> ---
> >>
> >> 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
>
> applied to u-boot-x86, thanks!
> >>>
> >>> Dropped this one from the x86 queue per the discussion.
> >>
> >> I just wanted to come back to this discussion.
> >>
> >> Do we have an agreed way forward? Who is waiting for who?
> >>
> >
> > I was waiting on feedback on
> > https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> > but per my opinion, I would prefer to go with "Approach 2" with a
> > Kconfig as it looks simpler to me. It would look something like below :
> >
> > if (gd->relocaddr > (unsigned long)ho->fb) {
> > ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
> >
> > /* Relocate after framebuffer area if nearing too close to it */
> > if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> > gd->relocaddr = ho->fb;
> > }
> >
> > Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> > -> This describes minimum gap to keep between framebuffer address and
> > relocation address to avoid overlap when framebuffer address used by
> > blob is below the current relocation address
> >
> > -> It would be selected as default when CONFIG_BLOBLIST is selected with
> > default value
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Simon,
On 11/09/23 04:44, Simon Glass wrote:
> Hi Devarsh,
>
> On Thu, 17 Aug 2023 at 09:10, Tom Rini 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 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 wrote:
>>>
>>> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
>
> On Tue, Aug 1, 2023 at 12:00 AM 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
>
> typo: interferes
>
>> allocation.
>>
>> This fixes a boot loop in qemu-x86_64
>>
>> Signed-off-by: Simon Glass
>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from
>> SPL to u-boot")
>> Suggested-by: Nikhil M Jain
>> ---
>>
>> 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
applied to u-boot-x86, thanks!
>>>
>>> Dropped this one from the x86 queue per the discussion.
>>
>> I just wanted to come back to this discussion.
>>
>> Do we have an agreed way forward? Who is waiting for who?
>>
>
> I was waiting on feedback on
> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> but per my opinion, I would prefer to go with "Approach 2" with a
> Kconfig as it looks simpler to me. It would look something like below :
>
> if (gd->relocaddr > (unsigned long)ho->fb) {
> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>
> /* Relocate after framebuffer area if nearing too close to it */
> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> gd->relocaddr = ho->fb;
> }
>
> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> -> This describes minimum gap to keep between framebuffer address and
> relocation address to avoid overlap when framebuffer address used by
> blob is below the current relocation address
>
> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> default value set to 100Mb
>
> -> SoC specific Vendors can override this in their defconfigs to a
> custom value if they feel 100Mb is not enough
>
> Also probably we can have some debug/error prints in the code to show
> overlap happened (or is going happen) so that users can fine tune this
> Kconfig if they got it wrong at
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh,
On Thu, 17 Aug 2023 at 09:10, Tom Rini 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 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 wrote:
> >
> > On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
> > >
> > > On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> > >>
> > >> On Tue, Aug 1, 2023 at 12:00 AM 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
> > >>
> > >> typo: interferes
> > >>
> > >>> allocation.
> > >>>
> > >>> This fixes a boot loop in qemu-x86_64
> > >>>
> > >>> Signed-off-by: Simon Glass
> > >>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from
> > >>> SPL to u-boot")
> > >>> Suggested-by: Nikhil M Jain
> > >>> ---
> > >>>
> > >>> 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
> > >
> > > applied to u-boot-x86, thanks!
> >
> > Dropped this one from the x86 queue per the discussion.
> > >>>
> > >>> I just wanted to come back to this discussion.
> > >>>
> > >>> Do we have an agreed way forward? Who is waiting for who?
> > >>>
> > >>
> > >> I was waiting on feedback on
> > >> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> > >> but per my opinion, I would prefer to go with "Approach 2" with a
> > >> Kconfig as it looks simpler to me. It would look something like below :
> > >>
> > >> if (gd->relocaddr > (unsigned long)ho->fb) {
> > >> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
> > >>
> > >> /* Relocate after framebuffer area if nearing too close to it */
> > >> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> > >> gd->relocaddr = ho->fb;
> > >> }
> > >>
> > >> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> > >> -> This describes minimum gap to keep between framebuffer address and
> > >> relocation address to avoid overlap when framebuffer address used by
> > >> blob is below the current relocation address
> > >>
> > >> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> > >> default value set to 100Mb
> > >>
> > >> -> SoC specific Vendors can override this in their defconfigs to a
> > >> custom value if they feel 100Mb is not enough
> > >>
> > >> Also probably we can have some debug/error prints in the code to show
> > >> overlap happened (or is going happen)
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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 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 wrote:
>
> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
> >
> > On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> >>
> >> On Tue, Aug 1, 2023 at 12:00 AM 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
> >>
> >> typo: interferes
> >>
> >>> allocation.
> >>>
> >>> This fixes a boot loop in qemu-x86_64
> >>>
> >>> Signed-off-by: Simon Glass
> >>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> >>> to u-boot")
> >>> Suggested-by: Nikhil M Jain
> >>> ---
> >>>
> >>> 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
> >
> > applied to u-boot-x86, thanks!
>
> Dropped this one from the x86 queue per the discussion.
> >>>
> >>> I just wanted to come back to this discussion.
> >>>
> >>> Do we have an agreed way forward? Who is waiting for who?
> >>>
> >>
> >> I was waiting on feedback on
> >> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> >> but per my opinion, I would prefer to go with "Approach 2" with a
> >> Kconfig as it looks simpler to me. It would look something like below :
> >>
> >> if (gd->relocaddr > (unsigned long)ho->fb) {
> >> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
> >>
> >> /* Relocate after framebuffer area if nearing too close to it */
> >> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> >> gd->relocaddr = ho->fb;
> >> }
> >>
> >> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> >> -> This describes minimum gap to keep between framebuffer address and
> >> relocation address to avoid overlap when framebuffer address used by
> >> blob is below the current relocation address
> >>
> >> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> >> default value set to 100Mb
> >>
> >> -> SoC specific Vendors can override this in their defconfigs to a
> >> custom value if they feel 100Mb is not enough
> >>
> >> Also probably we can have some debug/error prints in the code to show
> >> overlap happened (or is going happen) so that users can fine tune this
> >> Kconfig if they got it wrong at first place.
> >>
> >> I can re-spin updated patch if we are aligned on this,
> >> Kindly let me know your opinion.
> >
> > I'm just nervous about the whole idea, TBH. Perhaps I am missing some
> > documentation on how people are supposed to lay out
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Simon,
On 15/08/23 20:14, Simon Glass wrote:
> Hi Devarsh,
>
> On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar 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 wrote:
On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
>
> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
>>
>> On Tue, Aug 1, 2023 at 12:00 AM 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
>>
>> typo: interferes
>>
>>> allocation.
>>>
>>> This fixes a boot loop in qemu-x86_64
>>>
>>> Signed-off-by: Simon Glass
>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
>>> to u-boot")
>>> Suggested-by: Nikhil M Jain
>>> ---
>>>
>>> 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
>
> applied to u-boot-x86, thanks!
Dropped this one from the x86 queue per the discussion.
>>>
>>> I just wanted to come back to this discussion.
>>>
>>> Do we have an agreed way forward? Who is waiting for who?
>>>
>>
>> I was waiting on feedback on
>> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
>> but per my opinion, I would prefer to go with "Approach 2" with a
>> Kconfig as it looks simpler to me. It would look something like below :
>>
>> if (gd->relocaddr > (unsigned long)ho->fb) {
>> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>>
>> /* Relocate after framebuffer area if nearing too close to it */
>> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
>> gd->relocaddr = ho->fb;
>> }
>>
>> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
>> -> This describes minimum gap to keep between framebuffer address and
>> relocation address to avoid overlap when framebuffer address used by
>> blob is below the current relocation address
>>
>> -> It would be selected as default when CONFIG_BLOBLIST is selected with
>> default value set to 100Mb
>>
>> -> SoC specific Vendors can override this in their defconfigs to a
>> custom value if they feel 100Mb is not enough
>>
>> Also probably we can have some debug/error prints in the code to show
>> overlap happened (or is going happen) so that users can fine tune this
>> Kconfig if they got it wrong at first place.
>>
>> I can re-spin updated patch if we are aligned on this,
>> Kindly let me know your opinion.
>
> I'm just nervous about the whole idea, TBH. Perhaps I am missing some
> documentation on how people are supposed to lay out memory in SPL and
> U-Boot properr, but I'm not really aware of any guidance we give.
>
> Perhaps we should say that the SPL frame buffer should be at the top
> of memory, and U-Boot's reserve area should start below that?
1) As per my personal opinion, I don't like putting such constraints and would
instead like to give some
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh,
On Tue, 15 Aug 2023 at 03:23, Devarsh Thakkar 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 wrote:
> >>
> >> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
> >>>
> >>> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
>
> On Tue, Aug 1, 2023 at 12:00 AM 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
>
> typo: interferes
>
> > allocation.
> >
> > This fixes a boot loop in qemu-x86_64
> >
> > Signed-off-by: Simon Glass
> > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > to u-boot")
> > Suggested-by: Nikhil M Jain
> > ---
> >
> > 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
> >>>
> >>> applied to u-boot-x86, thanks!
> >>
> >> Dropped this one from the x86 queue per the discussion.
> >
> > I just wanted to come back to this discussion.
> >
> > Do we have an agreed way forward? Who is waiting for who?
> >
>
> I was waiting on feedback on
> https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
> but per my opinion, I would prefer to go with "Approach 2" with a
> Kconfig as it looks simpler to me. It would look something like below :
>
> if (gd->relocaddr > (unsigned long)ho->fb) {
> ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
>
> /* Relocate after framebuffer area if nearing too close to it */
> if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
> gd->relocaddr = ho->fb;
> }
>
> Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
> -> This describes minimum gap to keep between framebuffer address and
> relocation address to avoid overlap when framebuffer address used by
> blob is below the current relocation address
>
> -> It would be selected as default when CONFIG_BLOBLIST is selected with
> default value set to 100Mb
>
> -> SoC specific Vendors can override this in their defconfigs to a
> custom value if they feel 100Mb is not enough
>
> Also probably we can have some debug/error prints in the code to show
> overlap happened (or is going happen) so that users can fine tune this
> Kconfig if they got it wrong at first place.
>
> I can re-spin updated patch if we are aligned on this,
> Kindly let me know your opinion.
I'm just nervous about the whole idea, TBH. Perhaps I am missing some
documentation on how people are supposed to lay out memory in SPL and
U-Boot properr, but I'm not really aware of any guidance we give.
Perhaps we should say that the SPL frame buffer should be at the top
of memory, and U-Boot's reserve area should start below that?
Regards,
Simon
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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 wrote:
On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
On Tue, Aug 1, 2023 at 12:00 AM 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
typo: interferes
allocation.
This fixes a boot loop in qemu-x86_64
Signed-off-by: Simon Glass
Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
u-boot")
Suggested-by: Nikhil M Jain
---
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
applied to u-boot-x86, thanks!
Dropped this one from the x86 queue per the discussion.
I just wanted to come back to this discussion.
Do we have an agreed way forward? Who is waiting for who?
I was waiting on feedback on
https://lore.kernel.org/all/3b1e8005-f161-8058-13e7-3de2316aa...@ti.com/
but per my opinion, I would prefer to go with "Approach 2" with a
Kconfig as it looks simpler to me. It would look something like below :
if (gd->relocaddr > (unsigned long)ho->fb) {
ulong fb_reloc_gap = gd->relocaddr - gd->ho->fb;
/* Relocate after framebuffer area if nearing too close to it */
if (fb_reloc_gap < CONFIG_BLOBLIST_FB_RELOC_MIN_GAP)
gd->relocaddr = ho->fb;
}
Regarding CONFIG_BLOBLIST_FB_RELOC_MIN_GAP
-> This describes minimum gap to keep between framebuffer address and
relocation address to avoid overlap when framebuffer address used by
blob is below the current relocation address
-> It would be selected as default when CONFIG_BLOBLIST is selected with
default value set to 100Mb
-> SoC specific Vendors can override this in their defconfigs to a
custom value if they feel 100Mb is not enough
Also probably we can have some debug/error prints in the code to show
overlap happened (or is going happen) so that users can fine tune this
Kconfig if they got it wrong at first place.
I can re-spin updated patch if we are aligned on this,
Kindly let me know your opinion.
Regards
Devarsh
Regards,
Simon
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh, Nikhil, Tom,
On Wed, 9 Aug 2023 at 09:29, Bin Meng wrote:
>
> On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
> >
> > On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> > >
> > > On Tue, Aug 1, 2023 at 12:00 AM 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
> > >
> > > typo: interferes
> > >
> > > > allocation.
> > > >
> > > > This fixes a boot loop in qemu-x86_64
> > > >
> > > > Signed-off-by: Simon Glass
> > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > > > to u-boot")
> > > > Suggested-by: Nikhil M Jain
> > > > ---
> > > >
> > > > 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
> >
> > 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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
On Thu, Aug 3, 2023 at 7:03 PM Bin Meng wrote:
>
> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> >
> > On Tue, Aug 1, 2023 at 12:00 AM 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
> >
> > typo: interferes
> >
> > > allocation.
> > >
> > > This fixes a boot loop in qemu-x86_64
> > >
> > > Signed-off-by: Simon Glass
> > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> > > u-boot")
> > > Suggested-by: Nikhil M Jain
> > > ---
> > >
> > > 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
>
> applied to u-boot-x86, thanks!
Dropped this one from the x86 queue per the discussion.
Regards,
Bin
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Bin,
On Thu, 3 Aug 2023 at 08:19, Bin Meng wrote:
>
> Hi Tom, Simon,
>
> On Thu, Aug 3, 2023 at 9:13 PM Tom Rini 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 wrote:
> > > >
> > > > On Tue, Aug 1, 2023 at 12:00 AM 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
> > > >
> > > > typo: interferes
> > > >
> > > > > allocation.
> > > > >
> > > > > This fixes a boot loop in qemu-x86_64
> > > > >
> > > > > Signed-off-by: Simon Glass
> > > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > > > > to u-boot")
> > > > > Suggested-by: Nikhil M Jain
> > > > > ---
> > > > >
> > > > > 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
> > >
> > > 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-devar...@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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Tom, Simon,
On Thu, Aug 3, 2023 at 9:13 PM Tom Rini 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 wrote:
> > >
> > > On Tue, Aug 1, 2023 at 12:00 AM 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
> > >
> > > typo: interferes
> > >
> > > > allocation.
> > > >
> > > > This fixes a boot loop in qemu-x86_64
> > > >
> > > > Signed-off-by: Simon Glass
> > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > > > to u-boot")
> > > > Suggested-by: Nikhil M Jain
> > > > ---
> > > >
> > > > 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
> >
> > 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-devar...@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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Devarsh,
On Tue, 1 Aug 2023 at 08:30, Devarsh Thakkar 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 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 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 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
> >>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> >>> to u-boot")
> >>> Suggested-by: Nikhil M Jain
> >>> ---
> >>>
> >>> 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-...@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-devar...@ti.com/
> should suffice to fix this issue ?
>
> Simon,
> Could you please try with above patch once? ?
> In your case ,
> gd->relocaddr=c000, ho->fb=d000
>
> 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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
On Thu, Aug 03, 2023 at 07:03:47PM +0800, Bin Meng wrote:
> On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
> >
> > On Tue, Aug 1, 2023 at 12:00 AM 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
> >
> > typo: interferes
> >
> > > allocation.
> > >
> > > This fixes a boot loop in qemu-x86_64
> > >
> > > Signed-off-by: Simon Glass
> > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> > > u-boot")
> > > Suggested-by: Nikhil M Jain
> > > ---
> > >
> > > 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
>
> 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-devar...@ti.com/
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
On Thu, Aug 3, 2023 at 6:37 PM Bin Meng wrote:
>
> On Tue, Aug 1, 2023 at 12:00 AM 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
>
> typo: interferes
>
> > allocation.
> >
> > This fixes a boot loop in qemu-x86_64
> >
> > Signed-off-by: Simon Glass
> > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> > u-boot")
> > Suggested-by: Nikhil M Jain
> > ---
> >
> > 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
applied to u-boot-x86, thanks!
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
On Tue, Aug 1, 2023 at 12:00 AM 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
typo: interferes
> allocation.
>
> This fixes a boot loop in qemu-x86_64
>
> Signed-off-by: Simon Glass
> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> u-boot")
> Suggested-by: Nikhil M Jain
> ---
>
> 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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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 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 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 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
>>> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
>>> to u-boot")
>>> Suggested-by: Nikhil M Jain
>>> ---
>>>
>>> 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-...@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-devar...@ti.com/
should suffice to fix this issue ?
Simon,
Could you please try with above patch once? ?
In your case ,
gd->relocaddr=c000, ho->fb=d000
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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Tom,
On Mon, 31 Jul 2023 at 15:06, Tom Rini 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 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 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
> > > > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from
> > > > > > SPL to u-boot")
> > > > > > Suggested-by: Nikhil M Jain
> > > > > > ---
> > > > > >
> > > > > > 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-...@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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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 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 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
> > > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > > > > to u-boot")
> > > > > Suggested-by: Nikhil M Jain
> > > > > ---
> > > > >
> > > > > 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-...@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
signature.asc
Description: PGP signature
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Tom,
On Mon, 31 Jul 2023 at 14:45, Tom Rini 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 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
> > > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL
> > > > to u-boot")
> > > > Suggested-by: Nikhil M Jain
> > > > ---
> > > >
> > > > 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-...@chromium.org/
Regards,
Simon
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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 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
> > > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> > > u-boot")
> > > Suggested-by: Nikhil M Jain
> > > ---
> > >
> > > 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
signature.asc
Description: PGP signature
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
Hi Tom,
On Mon, 31 Jul 2023 at 13:32, Tom Rini 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
> > Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> > u-boot")
> > Suggested-by: Nikhil M Jain
> > ---
> >
> > 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
Re: [PATCH v3 5/9] board_f: Fix corruption of relocaddr
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
> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
> u-boot")
> Suggested-by: Nikhil M Jain
> ---
>
> 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
signature.asc
Description: PGP signature
[PATCH v3 5/9] board_f: Fix corruption of relocaddr
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
Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to
u-boot")
Suggested-by: Nikhil M Jain
---
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
