| Message ID | 20230724145210.304917-5-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Fixes for distro booting | expand |
Hi Simon, On 24/07/23 20:22, 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") > --- > > common/board_f.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/common/board_f.c b/common/board_f.c > index 7d2c380e91e2..5c8646b22283 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -419,7 +419,6 @@ static int reserve_video(void) > if (!ho) > return log_msg_ret("blf", -ENOENT); > video_reserve_from_bloblist(ho); > - gd->relocaddr = ho->fb; I think this change was done as relocaddr pointer was required to be updated to move after frame-buffer reserved area to ensure that any further memory reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) don't overlap with frame-buffer reserved area passed from blob, so I think removing this line may cause further memory reservations to overlap with reserved framebuffer. Could you please confirm? Regards Devarsh > } else if (CONFIG_IS_ENABLED(VIDEO)) { > ulong addr; > int ret;
Hi Devarsh, On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: > > Hi Simon, > > On 24/07/23 20:22, 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") > > --- > > > > common/board_f.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/common/board_f.c b/common/board_f.c > > index 7d2c380e91e2..5c8646b22283 100644 > > --- a/common/board_f.c > > +++ b/common/board_f.c > > @@ -419,7 +419,6 @@ static int reserve_video(void) > > if (!ho) > > return log_msg_ret("blf", -ENOENT); > > video_reserve_from_bloblist(ho); > > - gd->relocaddr = ho->fb; > > I think this change was done as relocaddr pointer was required to be updated > to move after frame-buffer reserved area to ensure that any further memory > reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) > don't overlap with frame-buffer reserved area passed from blob, so I think > removing this line may cause further memory reservations to overlap with > reserved framebuffer. > > Could you please confirm? SPL and U-Boot have different memory layouts. The current code is interrupting U-Boot's careful building up of chunks of memory used for different purposes. The video memory has already been allocated by SPL, so we don't need to insert a new one here, as your code demonstrates. But also we have no way of knowing if it is legal to relocate U-Boot (and various other things) just below the frame buffer chosen by SPL. The SPL frame buffer needs to be considered pre-allocated. It makes no sense to set relocaddr to this value. It will break all sorts of things. E.g. qemu-x86_64 crashes with this. > > > Regards > Devarsh > > > } else if (CONFIG_IS_ENABLED(VIDEO)) { > > ulong addr; > > int ret; Regards, Simon
Hi Simon, On 26/07/23 02:58, Simon Glass wrote: > Hi Devarsh, > > On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: >> >> Hi Simon, >> >> On 24/07/23 20:22, 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") >>> --- >>> >>> common/board_f.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/common/board_f.c b/common/board_f.c >>> index 7d2c380e91e2..5c8646b22283 100644 >>> --- a/common/board_f.c >>> +++ b/common/board_f.c >>> @@ -419,7 +419,6 @@ static int reserve_video(void) >>> if (!ho) >>> return log_msg_ret("blf", -ENOENT); >>> video_reserve_from_bloblist(ho); >>> - gd->relocaddr = ho->fb; >> >> I think this change was done as relocaddr pointer was required to be updated >> to move after frame-buffer reserved area to ensure that any further memory >> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) >> don't overlap with frame-buffer reserved area passed from blob, so I think >> removing this line may cause further memory reservations to overlap with >> reserved framebuffer. >> >> Could you please confirm? > > SPL and U-Boot have different memory layouts. The current code is > interrupting U-Boot's careful building up of chunks of memory used for > different purposes. > But it is possible they could be using similar memory layout for some components like framebuffer. For e.g. in our case we are using same video_reserve func in A53 SPL too and which allocates framebuffer from gd->relocaddr as seen here: https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 > The video memory has already been allocated by SPL, so we don't need > to insert a new one here, as your code demonstrates. > Agreed. > But also we have no way of knowing if it is legal to relocate U-Boot > (and various other things) just below the frame buffer chosen by SPL. > Yes, so i suppose your case is that framebuffer address which is being passed by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in any manner since relocaddr points to ramtop (i.e. near to end address of DDR). In that case I agree it doesn't make sense to move relocaddr to ho->fb. But for the scenario where gd->relocaddr and ho->fb are nearby there is every possibility that gd->relocaddr may overlap with framebuffer, also the code in reserve_trace, reserve_uboot doesn't have any intelligence or check that it is overlapping with framebuffer area or not. I think one thing that can probably be done here is to have a check that if passed framebuffer area falls within current relocaddr region, then update the relocaddr else don't touch relocaddr : if (ho->fb <= gd->relocaddr - ho->size) //It means framebuffer are is overlapping with current relocaddr so update relocaddr gd->relocaddr = ho->fb else //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr Could you please share your opinion on this and if above logic suffice your case too ? Regards Devarsh > The SPL frame buffer needs to be considered pre-allocated. It makes no > sense to set relocaddr to this value. It will break all sorts of > things. E.g. qemu-x86_64 crashes with this. > >> >> >> Regards >> Devarsh >> >>> } else if (CONFIG_IS_ENABLED(VIDEO)) { >>> ulong addr; >>> int ret; > > Regards, > Simon
Hi Devarsh, On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: > > Hi Simon, > > On 26/07/23 02:58, Simon Glass wrote: > > Hi Devarsh, > > > > On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: > >> > >> Hi Simon, > >> > >> On 24/07/23 20:22, 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") > >>> --- > >>> > >>> common/board_f.c | 1 - > >>> 1 file changed, 1 deletion(-) > >>> > >>> diff --git a/common/board_f.c b/common/board_f.c > >>> index 7d2c380e91e2..5c8646b22283 100644 > >>> --- a/common/board_f.c > >>> +++ b/common/board_f.c > >>> @@ -419,7 +419,6 @@ static int reserve_video(void) > >>> if (!ho) > >>> return log_msg_ret("blf", -ENOENT); > >>> video_reserve_from_bloblist(ho); > >>> - gd->relocaddr = ho->fb; > >> > >> I think this change was done as relocaddr pointer was required to be updated > >> to move after frame-buffer reserved area to ensure that any further memory > >> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) > >> don't overlap with frame-buffer reserved area passed from blob, so I think > >> removing this line may cause further memory reservations to overlap with > >> reserved framebuffer. > >> > >> Could you please confirm? > > > > SPL and U-Boot have different memory layouts. The current code is > > interrupting U-Boot's careful building up of chunks of memory used for > > different purposes. > > > > But it is possible they could be using similar memory layout for some > components like framebuffer. > For e.g. in our case we are using same video_reserve func in A53 SPL too > and which allocates framebuffer from gd->relocaddr as seen here: > > https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 Even if it is similar, the point is that U-Boot proper needs to do its own allocation stuff. Of course, if SPL sets up the video, it will provide the framebuffer address, but only that. The other addresses need to be done using the normal mechanism. > > > The video memory has already been allocated by SPL, so we don't need > > to insert a new one here, as your code demonstrates. > > > > Agreed. > > > But also we have no way of knowing if it is legal to relocate U-Boot > > (and various other things) just below the frame buffer chosen by SPL. > > > > Yes, so i suppose your case is that framebuffer address which is being passed > by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is > at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in > any manner since relocaddr points to ramtop (i.e. near to end address of DDR). > > In that case I agree it doesn't make sense to move relocaddr to ho->fb. > > But for the scenario where gd->relocaddr and ho->fb are nearby there is every > possibility that gd->relocaddr may overlap with framebuffer, also the code in > reserve_trace, reserve_uboot doesn't have any intelligence or check that it is > overlapping with framebuffer area or not. > > I think one thing that can probably be done here is to have a check that if > passed framebuffer area falls within current relocaddr region, then update the > relocaddr else don't touch relocaddr : > > if (ho->fb <= gd->relocaddr - ho->size) > //It means framebuffer are is overlapping with current relocaddr so update > relocaddr > gd->relocaddr = ho->fb > else > //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr > > Could you please share your opinion on this and if above logic suffice your > case too ? I don't think this line is needed at all, which is why this patch removes it. What problem are you seeing? Regards, Simon > > Regards > Devarsh > > > The SPL frame buffer needs to be considered pre-allocated. It makes no > > sense to set relocaddr to this value. It will break all sorts of > > things. E.g. qemu-x86_64 crashes with this. > > > >> > >> > >> Regards > >> Devarsh > >> > >>> } else if (CONFIG_IS_ENABLED(VIDEO)) { > >>> ulong addr; > >>> int ret; > > > > Regards, > > Simon
Hi Simon, On 27/07/23 06:23, Simon Glass wrote: > Hi Devarsh, > > On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: >> >> Hi Simon, >> >> On 26/07/23 02:58, Simon Glass wrote: >>> Hi Devarsh, >>> >>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On 24/07/23 20:22, 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") >>>>> --- >>>>> >>>>> common/board_f.c | 1 - >>>>> 1 file changed, 1 deletion(-) >>>>> >>>>> diff --git a/common/board_f.c b/common/board_f.c >>>>> index 7d2c380e91e2..5c8646b22283 100644 >>>>> --- a/common/board_f.c >>>>> +++ b/common/board_f.c >>>>> @@ -419,7 +419,6 @@ static int reserve_video(void) >>>>> if (!ho) >>>>> return log_msg_ret("blf", -ENOENT); >>>>> video_reserve_from_bloblist(ho); >>>>> - gd->relocaddr = ho->fb; >>>> >>>> I think this change was done as relocaddr pointer was required to be updated >>>> to move after frame-buffer reserved area to ensure that any further memory >>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) >>>> don't overlap with frame-buffer reserved area passed from blob, so I think >>>> removing this line may cause further memory reservations to overlap with >>>> reserved framebuffer. >>>> >>>> Could you please confirm? >>> >>> SPL and U-Boot have different memory layouts. The current code is >>> interrupting U-Boot's careful building up of chunks of memory used for >>> different purposes. >>> >> >> But it is possible they could be using similar memory layout for some >> components like framebuffer. >> For e.g. in our case we are using same video_reserve func in A53 SPL too >> and which allocates framebuffer from gd->relocaddr as seen here: >> >> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 > > Even if it is similar, the point is that U-Boot proper needs to do its > own allocation stuff. > > Of course, if SPL sets up the video, it will provide the framebuffer > address, but only that. The other addresses need to be done using the > normal mechanism. > >> >>> The video memory has already been allocated by SPL, so we don't need >>> to insert a new one here, as your code demonstrates. >>> >> >> Agreed. >> >>> But also we have no way of knowing if it is legal to relocate U-Boot >>> (and various other things) just below the frame buffer chosen by SPL. >>> >> >> Yes, so i suppose your case is that framebuffer address which is being passed >> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is >> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in >> any manner since relocaddr points to ramtop (i.e. near to end address of DDR). >> >> In that case I agree it doesn't make sense to move relocaddr to ho->fb. >> >> But for the scenario where gd->relocaddr and ho->fb are nearby there is every >> possibility that gd->relocaddr may overlap with framebuffer, also the code in >> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is >> overlapping with framebuffer area or not. >> >> I think one thing that can probably be done here is to have a check that if >> passed framebuffer area falls within current relocaddr region, then update the >> relocaddr else don't touch relocaddr : >> >> if (ho->fb <= gd->relocaddr - ho->size) >> //It means framebuffer are is overlapping with current relocaddr so update >> relocaddr >> gd->relocaddr = ho->fb >> else >> //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr >> >> Could you please share your opinion on this and if above logic suffice your >> case too ? > > I don't think this line is needed at all, which is why this patch > removes it. What problem are you seeing? > Across SPL stage and U-boot we are keeping same memory layout and ensuring that same memory regions are used, this way it doesn't interfere in the way of u-boot while allocating memory regions for various purposes. This allowed us to display splash screen without any flicker across the stages. Now if you remove the line gd->relocaddr = ho->fb, the frame buffer region will be used for reserving memory for other purposes which corrupts the frame buffer. One solution which we are planning to implement is move the ram_top to a lower address leaving out a region for video buffer and u-boot can do the allocation from the new ram_top address without spl video handoff interfering in the u-boot's allocation of memory.The region above the ram_top can be used for video. Present Scenario +---------------------+ram_top | | | page_table | | | | | +---------------------+ | | | | | | | | | | | video frame buffer | | | | | | | | | | | | | +---------------------+ | | | | | reserve_trace | | | | | | | +---------------------+ Proposed Solution +---------------------+ | | | | | | | | | | | video frame buffer | | | | | | | | | | | +---------------------+ram_top | | | | | page_table | | | | | | | +---------------------+ | | | | | reserve_trace | | | | | +---------------------+ > Regards, > Simon > > >> >> Regards >> Devarsh >> >>> The SPL frame buffer needs to be considered pre-allocated. It makes no >>> sense to set relocaddr to this value. It will break all sorts of >>> things. E.g. qemu-x86_64 crashes with this. >>> >>>> >>>> >>>> Regards >>>> Devarsh >>>> >>>>> } else if (CONFIG_IS_ENABLED(VIDEO)) { >>>>> ulong addr; >>>>> int ret; >>> >>> Regards, >>> Simon Regards, Nikhil
Hi Nikhil, On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote: > > Hi Simon, > > On 27/07/23 06:23, Simon Glass wrote: > > Hi Devarsh, > > > > On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: > >> > >> Hi Simon, > >> > >> On 26/07/23 02:58, Simon Glass wrote: > >>> Hi Devarsh, > >>> > >>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 24/07/23 20:22, 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") > >>>>> --- > >>>>> > >>>>> common/board_f.c | 1 - > >>>>> 1 file changed, 1 deletion(-) > >>>>> > >>>>> diff --git a/common/board_f.c b/common/board_f.c > >>>>> index 7d2c380e91e2..5c8646b22283 100644 > >>>>> --- a/common/board_f.c > >>>>> +++ b/common/board_f.c > >>>>> @@ -419,7 +419,6 @@ static int reserve_video(void) > >>>>> if (!ho) > >>>>> return log_msg_ret("blf", -ENOENT); > >>>>> video_reserve_from_bloblist(ho); > >>>>> - gd->relocaddr = ho->fb; > >>>> > >>>> I think this change was done as relocaddr pointer was required to be updated > >>>> to move after frame-buffer reserved area to ensure that any further memory > >>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) > >>>> don't overlap with frame-buffer reserved area passed from blob, so I think > >>>> removing this line may cause further memory reservations to overlap with > >>>> reserved framebuffer. > >>>> > >>>> Could you please confirm? > >>> > >>> SPL and U-Boot have different memory layouts. The current code is > >>> interrupting U-Boot's careful building up of chunks of memory used for > >>> different purposes. > >>> > >> > >> But it is possible they could be using similar memory layout for some > >> components like framebuffer. > >> For e.g. in our case we are using same video_reserve func in A53 SPL too > >> and which allocates framebuffer from gd->relocaddr as seen here: > >> > >> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 > > > > Even if it is similar, the point is that U-Boot proper needs to do its > > own allocation stuff. > > > > Of course, if SPL sets up the video, it will provide the framebuffer > > address, but only that. The other addresses need to be done using the > > normal mechanism. > > > >> > >>> The video memory has already been allocated by SPL, so we don't need > >>> to insert a new one here, as your code demonstrates. > >>> > >> > >> Agreed. > >> > >>> But also we have no way of knowing if it is legal to relocate U-Boot > >>> (and various other things) just below the frame buffer chosen by SPL. > >>> > >> > >> Yes, so i suppose your case is that framebuffer address which is being passed > >> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is > >> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in > >> any manner since relocaddr points to ramtop (i.e. near to end address of DDR). > >> > >> In that case I agree it doesn't make sense to move relocaddr to ho->fb. > >> > >> But for the scenario where gd->relocaddr and ho->fb are nearby there is every > >> possibility that gd->relocaddr may overlap with framebuffer, also the code in > >> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is > >> overlapping with framebuffer area or not. > >> > >> I think one thing that can probably be done here is to have a check that if > >> passed framebuffer area falls within current relocaddr region, then update the > >> relocaddr else don't touch relocaddr : > >> > >> if (ho->fb <= gd->relocaddr - ho->size) > >> //It means framebuffer are is overlapping with current relocaddr so update > >> relocaddr > >> gd->relocaddr = ho->fb > >> else > >> //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr > >> > >> Could you please share your opinion on this and if above logic suffice your > >> case too ? > > > > I don't think this line is needed at all, which is why this patch > > removes it. What problem are you seeing? > > > Across SPL stage and U-boot we are keeping same memory layout and > ensuring that same memory regions are used, this way it doesn't > interfere in the way of u-boot while allocating memory regions for > various purposes. This allowed us to display splash screen without any > flicker across the stages. > > Now if you remove the line gd->relocaddr = ho->fb, the frame buffer > region will be used for reserving memory for other purposes which > corrupts the frame buffer. > > One solution which we are planning to implement is move the ram_top to a > lower address leaving out a region for video buffer and u-boot can do > the allocation from the new ram_top address without spl video handoff > interfering in the u-boot's allocation of memory.The region above the > ram_top can be used for video. > > Present Scenario > +---------------------+ram_top > | | > | page_table | > | | > | | > +---------------------+ > | | > | | > | | > | | > | | > | video frame buffer | > | | > | | > | | > | | > | | > | | > +---------------------+ > | | > | | > | reserve_trace | > | | > | | > | | > +---------------------+ > > > Proposed Solution > +---------------------+ > | | > | | > | | > | | > | | > | video frame buffer | > | | > | | > | | > | | > | | > +---------------------+ram_top > | | > | | > | page_table | > | | > | | > | | > +---------------------+ > | | > | | > | reserve_trace | > | | > | | > +---------------------+ Sure, whatever you need to do is fine. You could pass the ram top from SPL to U-Boot perhaps. Regards, Simon
Hi Simon, On 27/07/23 23:31, Simon Glass wrote: > Hi Nikhil, > > On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote: >> >> Hi Simon, >> >> On 27/07/23 06:23, Simon Glass wrote: >>> Hi Devarsh, >>> >>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On 26/07/23 02:58, Simon Glass wrote: >>>>> Hi Devarsh, >>>>> >>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> On 24/07/23 20:22, 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") >>>>>>> --- >>>>>>> >>>>>>> common/board_f.c | 1 - >>>>>>> 1 file changed, 1 deletion(-) >>>>>>> >>>>>>> diff --git a/common/board_f.c b/common/board_f.c >>>>>>> index 7d2c380e91e2..5c8646b22283 100644 >>>>>>> --- a/common/board_f.c >>>>>>> +++ b/common/board_f.c >>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void) >>>>>>> if (!ho) >>>>>>> return log_msg_ret("blf", -ENOENT); >>>>>>> video_reserve_from_bloblist(ho); >>>>>>> - gd->relocaddr = ho->fb; >>>>>> >>>>>> I think this change was done as relocaddr pointer was required to be updated >>>>>> to move after frame-buffer reserved area to ensure that any further memory >>>>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) >>>>>> don't overlap with frame-buffer reserved area passed from blob, so I think >>>>>> removing this line may cause further memory reservations to overlap with >>>>>> reserved framebuffer. >>>>>> >>>>>> Could you please confirm? >>>>> >>>>> SPL and U-Boot have different memory layouts. The current code is >>>>> interrupting U-Boot's careful building up of chunks of memory used for >>>>> different purposes. >>>>> >>>> >>>> But it is possible they could be using similar memory layout for some >>>> components like framebuffer. >>>> For e.g. in our case we are using same video_reserve func in A53 SPL too >>>> and which allocates framebuffer from gd->relocaddr as seen here: >>>> >>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 >>> >>> Even if it is similar, the point is that U-Boot proper needs to do its >>> own allocation stuff. >>> >>> Of course, if SPL sets up the video, it will provide the framebuffer >>> address, but only that. The other addresses need to be done using the >>> normal mechanism. >>> >>>> >>>>> The video memory has already been allocated by SPL, so we don't need >>>>> to insert a new one here, as your code demonstrates. >>>>> >>>> >>>> Agreed. >>>> >>>>> But also we have no way of knowing if it is legal to relocate U-Boot >>>>> (and various other things) just below the frame buffer chosen by SPL. >>>>> >>>> >>>> Yes, so i suppose your case is that framebuffer address which is being passed >>>> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is >>>> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in >>>> any manner since relocaddr points to ramtop (i.e. near to end address of DDR). >>>> >>>> In that case I agree it doesn't make sense to move relocaddr to ho->fb. >>>> >>>> But for the scenario where gd->relocaddr and ho->fb are nearby there is every >>>> possibility that gd->relocaddr may overlap with framebuffer, also the code in >>>> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is >>>> overlapping with framebuffer area or not. >>>> >>>> I think one thing that can probably be done here is to have a check that if >>>> passed framebuffer area falls within current relocaddr region, then update the >>>> relocaddr else don't touch relocaddr : >>>> >>>> if (ho->fb <= gd->relocaddr - ho->size) >>>> //It means framebuffer are is overlapping with current relocaddr so update >>>> relocaddr >>>> gd->relocaddr = ho->fb We should go ahead with this check because it won't disrupt u-boot's allocation of memory and will allow both the cases if a platform is using same memory layout or different memory layout across SPL and u-boot proper. Below are the logs for both scenarios. https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df >>>> else >>>> //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr >>>> >>>> Could you please share your opinion on this and if above logic suffice your >>>> case too ? >>> >>> I don't think this line is needed at all, which is why this patch >>> removes it. What problem are you seeing? >>> >> Across SPL stage and U-boot we are keeping same memory layout and >> ensuring that same memory regions are used, this way it doesn't >> interfere in the way of u-boot while allocating memory regions for >> various purposes. This allowed us to display splash screen without any >> flicker across the stages. >> >> Now if you remove the line gd->relocaddr = ho->fb, the frame buffer >> region will be used for reserving memory for other purposes which >> corrupts the frame buffer. >> >> One solution which we are planning to implement is move the ram_top to a >> lower address leaving out a region for video buffer and u-boot can do >> the allocation from the new ram_top address without spl video handoff >> interfering in the u-boot's allocation of memory.The region above the >> ram_top can be used for video. >> >> Present Scenario >> +---------------------+ram_top >> | | >> | page_table | >> | | >> | | >> +---------------------+ >> | | >> | | >> | | >> | | >> | | >> | video frame buffer | >> | | >> | | >> | | >> | | >> | | >> | | >> +---------------------+ >> | | >> | | >> | reserve_trace | >> | | >> | | >> | | >> +---------------------+ >> >> >> Proposed Solution >> +---------------------+ >> | | >> | | >> | | >> | | >> | | >> | video frame buffer | >> | | >> | | >> | | >> | | >> | | >> +---------------------+ram_top >> | | >> | | >> | page_table | >> | | >> | | >> | | >> +---------------------+ >> | | >> | | >> | reserve_trace | >> | | >> | | >> +---------------------+ > > Sure, whatever you need to do is fine. You could pass the ram top from > SPL to U-Boot perhaps. > In this solution problem arises when user doesn't want's to hand-off buffer, the frame buffer region will be reallocated by u-boot or will require us to hard code buffer address, which we don't want to do. > Regards, > Simon Thanks, Nikhil
On 28/07/23 14:05, Nikhil M Jain wrote: > Hi Simon, > > On 27/07/23 23:31, Simon Glass wrote: >> Hi Nikhil, >> >> On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote: >>> >>> Hi Simon, >>> >>> On 27/07/23 06:23, Simon Glass wrote: >>>> Hi Devarsh, >>>> >>>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: >>>>> >>>>> Hi Simon, >>>>> >>>>> On 26/07/23 02:58, Simon Glass wrote: >>>>>> Hi Devarsh, >>>>>> >>>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> >>>>>> wrote: >>>>>>> >>>>>>> Hi Simon, >>>>>>> >>>>>>> On 24/07/23 20:22, 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") >>>>>>>> --- >>>>>>>> >>>>>>>> common/board_f.c | 1 - >>>>>>>> 1 file changed, 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/common/board_f.c b/common/board_f.c >>>>>>>> index 7d2c380e91e2..5c8646b22283 100644 >>>>>>>> --- a/common/board_f.c >>>>>>>> +++ b/common/board_f.c >>>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void) >>>>>>>> if (!ho) >>>>>>>> return log_msg_ret("blf", -ENOENT); >>>>>>>> video_reserve_from_bloblist(ho); >>>>>>>> - gd->relocaddr = ho->fb; >>>>>>> >>>>>>> I think this change was done as relocaddr pointer was required to >>>>>>> be updated >>>>>>> to move after frame-buffer reserved area to ensure that any >>>>>>> further memory >>>>>>> reservations done using gd->relocaddr (for e.g. in >>>>>>> reserve_trace/uboot/malloc) >>>>>>> don't overlap with frame-buffer reserved area passed from blob, >>>>>>> so I think >>>>>>> removing this line may cause further memory reservations to >>>>>>> overlap with >>>>>>> reserved framebuffer. >>>>>>> >>>>>>> Could you please confirm? >>>>>> >>>>>> SPL and U-Boot have different memory layouts. The current code is >>>>>> interrupting U-Boot's careful building up of chunks of memory used >>>>>> for >>>>>> different purposes. >>>>>> >>>>> >>>>> But it is possible they could be using similar memory layout for some >>>>> components like framebuffer. >>>>> For e.g. in our case we are using same video_reserve func in A53 >>>>> SPL too >>>>> and which allocates framebuffer from gd->relocaddr as seen here: >>>>> >>>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 >>>> >>>> Even if it is similar, the point is that U-Boot proper needs to do its >>>> own allocation stuff. >>>> >>>> Of course, if SPL sets up the video, it will provide the framebuffer >>>> address, but only that. The other addresses need to be done using the >>>> normal mechanism. >>>> >>>>> >>>>>> The video memory has already been allocated by SPL, so we don't need >>>>>> to insert a new one here, as your code demonstrates. >>>>>> >>>>> >>>>> Agreed. >>>>> >>>>>> But also we have no way of knowing if it is legal to relocate U-Boot >>>>>> (and various other things) just below the frame buffer chosen by SPL. >>>>>> >>>>> >>>>> Yes, so i suppose your case is that framebuffer address which is >>>>> being passed >>>>> by SPL is totally disjoint and too far away from gd->relocaddr, for >>>>> e.g. it is >>>>> at the start (or bottom of DDR) and doesn't interfere with >>>>> gd->relocaddr in >>>>> any manner since relocaddr points to ramtop (i.e. near to end >>>>> address of DDR). >>>>> >>>>> In that case I agree it doesn't make sense to move relocaddr to >>>>> ho->fb. >>>>> >>>>> But for the scenario where gd->relocaddr and ho->fb are nearby >>>>> there is every >>>>> possibility that gd->relocaddr may overlap with framebuffer, also >>>>> the code in >>>>> reserve_trace, reserve_uboot doesn't have any intelligence or check >>>>> that it is >>>>> overlapping with framebuffer area or not. >>>>> >>>>> I think one thing that can probably be done here is to have a check >>>>> that if >>>>> passed framebuffer area falls within current relocaddr region, then >>>>> update the >>>>> relocaddr else don't touch relocaddr : >>>>> >>>>> if (ho->fb <= gd->relocaddr - ho->size) Just a small correction here if (ho->fb >= gd->relocaddr - ho->size) >>>>> //It means framebuffer are is overlapping with current >>>>> relocaddr so update >>>>> relocaddr >>>>> gd->relocaddr = ho->fb > > We should go ahead with this check because it won't disrupt u-boot's > allocation of memory and will allow both the cases if a platform is > using same memory layout or different memory layout across SPL and > u-boot proper. Below are the logs for both scenarios. > > https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df > >>>>> else >>>>> //don't update gd->relocaddr since ho->fb is disjoint to >>>>> gd->relocaddr >>>>> >>>>> Could you please share your opinion on this and if above logic >>>>> suffice your >>>>> case too ? >>>> >>>> I don't think this line is needed at all, which is why this patch >>>> removes it. What problem are you seeing? >>>> >>> Across SPL stage and U-boot we are keeping same memory layout and >>> ensuring that same memory regions are used, this way it doesn't >>> interfere in the way of u-boot while allocating memory regions for >>> various purposes. This allowed us to display splash screen without any >>> flicker across the stages. >>> >>> Now if you remove the line gd->relocaddr = ho->fb, the frame buffer >>> region will be used for reserving memory for other purposes which >>> corrupts the frame buffer. >>> >>> One solution which we are planning to implement is move the ram_top to a >>> lower address leaving out a region for video buffer and u-boot can do >>> the allocation from the new ram_top address without spl video handoff >>> interfering in the u-boot's allocation of memory.The region above the >>> ram_top can be used for video. >>> >>> Present Scenario >>> +---------------------+ram_top >>> | | >>> | page_table | >>> | | >>> | | >>> +---------------------+ >>> | | >>> | | >>> | | >>> | | >>> | | >>> | video frame buffer | >>> | | >>> | | >>> | | >>> | | >>> | | >>> | | >>> +---------------------+ >>> | | >>> | | >>> | reserve_trace | >>> | | >>> | | >>> | | >>> +---------------------+ >>> >>> >>> Proposed Solution >>> +---------------------+ >>> | | >>> | | >>> | | >>> | | >>> | | >>> | video frame buffer | >>> | | >>> | | >>> | | >>> | | >>> | | >>> +---------------------+ram_top >>> | | >>> | | >>> | page_table | >>> | | >>> | | >>> | | >>> +---------------------+ >>> | | >>> | | >>> | reserve_trace | >>> | | >>> | | >>> +---------------------+ >> >> Sure, whatever you need to do is fine. You could pass the ram top from >> SPL to U-Boot perhaps. >> > > In this solution problem arises when user doesn't want's to hand-off > buffer, the frame buffer region will be reallocated by u-boot or will > require us to hard code buffer address, which we don't want to do. > >> Regards, >> Simon > > Thanks, > Nikhil
Hi, +Tom Rini for comment On Fri, 28 Jul 2023 at 02:35, Nikhil M Jain <n-jain1@ti.com> wrote: > > Hi Simon, > > On 27/07/23 23:31, Simon Glass wrote: > > Hi Nikhil, > > > > On Wed, 26 Jul 2023 at 23:22, Nikhil M Jain <n-jain1@ti.com> wrote: > >> > >> Hi Simon, > >> > >> On 27/07/23 06:23, Simon Glass wrote: > >>> Hi Devarsh, > >>> > >>> On Wed, 26 Jul 2023 at 05:09, Devarsh Thakkar <devarsht@ti.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 26/07/23 02:58, Simon Glass wrote: > >>>>> Hi Devarsh, > >>>>> > >>>>> On Tue, 25 Jul 2023 at 03:21, Devarsh Thakkar <devarsht@ti.com> wrote: > >>>>>> > >>>>>> Hi Simon, > >>>>>> > >>>>>> On 24/07/23 20:22, 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") > >>>>>>> --- > >>>>>>> > >>>>>>> common/board_f.c | 1 - > >>>>>>> 1 file changed, 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/common/board_f.c b/common/board_f.c > >>>>>>> index 7d2c380e91e2..5c8646b22283 100644 > >>>>>>> --- a/common/board_f.c > >>>>>>> +++ b/common/board_f.c > >>>>>>> @@ -419,7 +419,6 @@ static int reserve_video(void) > >>>>>>> if (!ho) > >>>>>>> return log_msg_ret("blf", -ENOENT); > >>>>>>> video_reserve_from_bloblist(ho); > >>>>>>> - gd->relocaddr = ho->fb; > >>>>>> > >>>>>> I think this change was done as relocaddr pointer was required to be updated > >>>>>> to move after frame-buffer reserved area to ensure that any further memory > >>>>>> reservations done using gd->relocaddr (for e.g. in reserve_trace/uboot/malloc) > >>>>>> don't overlap with frame-buffer reserved area passed from blob, so I think > >>>>>> removing this line may cause further memory reservations to overlap with > >>>>>> reserved framebuffer. > >>>>>> > >>>>>> Could you please confirm? > >>>>> > >>>>> SPL and U-Boot have different memory layouts. The current code is > >>>>> interrupting U-Boot's careful building up of chunks of memory used for > >>>>> different purposes. > >>>>> > >>>> > >>>> But it is possible they could be using similar memory layout for some > >>>> components like framebuffer. > >>>> For e.g. in our case we are using same video_reserve func in A53 SPL too > >>>> and which allocates framebuffer from gd->relocaddr as seen here: > >>>> > >>>> https://source.denx.de/u-boot/u-boot/-/blob/v2023.10-rc1/common/board_f.c?ref_type=tags#L427 > >>> > >>> Even if it is similar, the point is that U-Boot proper needs to do its > >>> own allocation stuff. > >>> > >>> Of course, if SPL sets up the video, it will provide the framebuffer > >>> address, but only that. The other addresses need to be done using the > >>> normal mechanism. > >>> > >>>> > >>>>> The video memory has already been allocated by SPL, so we don't need > >>>>> to insert a new one here, as your code demonstrates. > >>>>> > >>>> > >>>> Agreed. > >>>> > >>>>> But also we have no way of knowing if it is legal to relocate U-Boot > >>>>> (and various other things) just below the frame buffer chosen by SPL. > >>>>> > >>>> > >>>> Yes, so i suppose your case is that framebuffer address which is being passed > >>>> by SPL is totally disjoint and too far away from gd->relocaddr, for e.g. it is > >>>> at the start (or bottom of DDR) and doesn't interfere with gd->relocaddr in > >>>> any manner since relocaddr points to ramtop (i.e. near to end address of DDR). > >>>> > >>>> In that case I agree it doesn't make sense to move relocaddr to ho->fb. > >>>> > >>>> But for the scenario where gd->relocaddr and ho->fb are nearby there is every > >>>> possibility that gd->relocaddr may overlap with framebuffer, also the code in > >>>> reserve_trace, reserve_uboot doesn't have any intelligence or check that it is > >>>> overlapping with framebuffer area or not. > >>>> > >>>> I think one thing that can probably be done here is to have a check that if > >>>> passed framebuffer area falls within current relocaddr region, then update the > >>>> relocaddr else don't touch relocaddr : > >>>> > >>>> if (ho->fb <= gd->relocaddr - ho->size) > >>>> //It means framebuffer are is overlapping with current relocaddr so update > >>>> relocaddr > >>>> gd->relocaddr = ho->fb > > We should go ahead with this check because it won't disrupt u-boot's > allocation of memory and will allow both the cases if a platform is > using same memory layout or different memory layout across SPL and > u-boot proper. Below are the logs for both scenarios. > > https://gist.github.com/NikMJain/aca198ae77b6f1855459bc8fbdd683df > > >>>> else > >>>> //don't update gd->relocaddr since ho->fb is disjoint to gd->relocaddr > >>>> > >>>> Could you please share your opinion on this and if above logic suffice your > >>>> case too ? > >>> > >>> I don't think this line is needed at all, which is why this patch > >>> removes it. What problem are you seeing? > >>> > >> Across SPL stage and U-boot we are keeping same memory layout and > >> ensuring that same memory regions are used, this way it doesn't > >> interfere in the way of u-boot while allocating memory regions for > >> various purposes. This allowed us to display splash screen without any > >> flicker across the stages. > >> > >> Now if you remove the line gd->relocaddr = ho->fb, the frame buffer > >> region will be used for reserving memory for other purposes which > >> corrupts the frame buffer. > >> > >> One solution which we are planning to implement is move the ram_top to a > >> lower address leaving out a region for video buffer and u-boot can do > >> the allocation from the new ram_top address without spl video handoff > >> interfering in the u-boot's allocation of memory.The region above the > >> ram_top can be used for video. > >> > >> Present Scenario > >> +---------------------+ram_top > >> | | > >> | page_table | > >> | | > >> | | > >> +---------------------+ > >> | | > >> | | > >> | | > >> | | > >> | | > >> | video frame buffer | > >> | | > >> | | > >> | | > >> | | > >> | | > >> | | > >> +---------------------+ > >> | | > >> | | > >> | reserve_trace | > >> | | > >> | | > >> | | > >> +---------------------+ > >> > >> > >> Proposed Solution > >> +---------------------+ > >> | | > >> | | > >> | | > >> | | > >> | | > >> | video frame buffer | > >> | | > >> | | > >> | | > >> | | > >> | | > >> +---------------------+ram_top > >> | | > >> | | > >> | page_table | > >> | | > >> | | > >> | | > >> +---------------------+ > >> | | > >> | | > >> | reserve_trace | > >> | | > >> | | > >> +---------------------+ > > > > Sure, whatever you need to do is fine. You could pass the ram top from > > SPL to U-Boot perhaps. > > > > In this solution problem arises when user doesn't want's to hand-off > buffer, the frame buffer region will be reallocated by u-boot or will > require us to hard code buffer address, which we don't want to do. How about using a Kconfig instead? It seems to me that the idea of SPL and U-Boot happening to place the video buffer at the same place is more the exception than the rule. I've tried to explain that the sequence of reserve...() calls that U-Boot proper does really should not be molested by some data arriving from SPL. If you don't want to do a Kconfig, then please send a patch that works for you and I'll test it, then update my series. Regards, Simon
diff --git a/common/board_f.c b/common/board_f.c index 7d2c380e91e2..5c8646b22283 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -419,7 +419,6 @@ static int reserve_video(void) if (!ho) return log_msg_ret("blf", -ENOENT); video_reserve_from_bloblist(ho); - gd->relocaddr = ho->fb; } else if (CONFIG_IS_ENABLED(VIDEO)) { ulong addr; int ret;
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") --- common/board_f.c | 1 - 1 file changed, 1 deletion(-)