Hi Simon,

On 03/08/23 19:32, Simon Glass wrote:

+Bin Meng

Hi Devarsh,

On Tue, 1 Aug 2023 at 08:04, Devarsh Thakkar devarsht@ti.com wrote:

...

When passing framebuffer address using bloblist, check that passed address is overlapping with current relocation address, if so move the relocation address after the framebuffer region to avoid overlap.

Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Signed-off-by: Devarsh Thakkar devarsht@ti.com

---

common/board_f.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c index 7d2c380e91..20fa17207a 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -419,7 +419,10 @@ static int reserve_video(void) if (!ho) return log_msg_ret("blf", -ENOENT); video_reserve_from_bloblist(ho);

•           gd->relocaddr = ho->fb;
  

•           /* Relocate after framebuffer area to avoid overlap */
  

•           if (gd->relocaddr > (unsigned long)ho->fb)
  

•                   gd->relocaddr = ho->fb;
  

•   } else if (CONFIG_IS_ENABLED(VIDEO)) {
            ulong addr;
            int ret;
  

-- 2.34.1

Yes this happens to work with qemu-x86_64. However it depends on the SPL frame buffer being below the current allocation area. Why would it be below, in general? This seems like a land mine for others to trip up on.

Basically to avoid overlap, the thing we are enforcing here is that relocaddr should always be below framebuffer so that further areas reserved do not overlap with framebuffer since we decrement relocaddr to resereve them. If relocaddr is already below framebuffer then there is no need to update relocaddr as in your case otherwise we have to do it, hence working for both scenarios.

The way I look at it is at u-boot proper is getting a blob from SPL with video handoff structure and before carrying out further reservations u-boot proper being agnostic to the framebuffer address set by SPL should have some check to make sure there is no overlap with current relocaddr.

I think this is similar to suggestion from Tom and you regarding moving the ram_top if framebuffer is reserved near the ram_top, albeit instead of moving ram_top I am moving relocaddr which is derived from ram_top.

Kindly let me know if any queries.

Regards Devarsh

Anyway I am OK with this for now since it fixes my problems. I would prefer something positive like the Kconfig I provided, since I still think it is very weird to interrupt the U-Boot reservation process like this.> Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon

Hi Simon, Tom, Bin,

Thanks for the quick feedback.

On 04/08/23 04:58, Simon Glass wrote:

Hi Devarsh,

On Thu, 3 Aug 2023 at 08:28, Devarsh Thakkar devarsht@ti.com wrote:

...

Hi Simon,

On 03/08/23 19:32, Simon Glass wrote:

...

+Bin Meng

Hi Devarsh,

On Tue, 1 Aug 2023 at 08:04, Devarsh Thakkar devarsht@ti.com wrote:

...

When passing framebuffer address using bloblist, check that passed address is overlapping with current relocation address, if so move the relocation address after the framebuffer region to avoid overlap.

Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Signed-off-by: Devarsh Thakkar devarsht@ti.com

---

common/board_f.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c index 7d2c380e91..20fa17207a 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -419,7 +419,10 @@ static int reserve_video(void) if (!ho) return log_msg_ret("blf", -ENOENT); video_reserve_from_bloblist(ho);

•           gd->relocaddr = ho->fb;
  

•           /* Relocate after framebuffer area to avoid overlap */
  

•           if (gd->relocaddr > (unsigned long)ho->fb)
  

•                   gd->relocaddr = ho->fb;
  

•   } else if (CONFIG_IS_ENABLED(VIDEO)) {
            ulong addr;
            int ret;
  

-- 2.34.1

Yes this happens to work with qemu-x86_64. However it depends on the SPL frame buffer being below the current allocation area. Why would it be below, in general? This seems like a land mine for others to trip up on.

Basically to avoid overlap, the thing we are enforcing here is that relocaddr should always be below framebuffer so that further areas reserved do not overlap with framebuffer since we decrement relocaddr to resereve them. If relocaddr is already below framebuffer then there is no need to update relocaddr as in your case otherwise we have to do it, hence working for both scenarios.

That's not true in general. Say the frame buffer is at 1GB and U-Boot's top of memory is at 2GB, then this will create a gap in the reservations, so some things are just below 2GB and some others are just below 1GB. There will be no indication of this...

Thanks Simon, I understand your point here but I think this problem is still there even with other approaches like separate Kconfig or SPL blob passing to indicate relocaddr update, as though even in that case SPL will need some base to take a decision on whether u-boot proper will require to move relocaddr or not.

For e.g. in this case there is a gap of ~1Gb between relocaddr and framebuffer and so we don't want to move relocaddr here since we assume that most likely ~1Gb is enough for u-boot to carry out and complete it's reservations without touching the framebuffer area, but this assumption may not hold true if the gap is in Mb's or Kb's.

To resolve this, the maximum value required for u-boot to carry out reservations need to be estimated and then we can make this condition more intelligent :

if (gd->relocaddr - gd->video_top < UBOOT_MAX_RESERVE_SIZE) gd->relocaddr = ho->fb;

If above looks good then, is it possible to estimate worst case common value for UBOOT_MAX_RESERVE_SIZE ? Or we want vendors to choose it say using a separate Kconfig say CONFIG_UBOOT_MAX_RESERVE_SIZE ?

Kindly let me know your feedback on above approach, I can send a V2 based on it.

Also to add, I think different vendors may choose to use different addresses for reserving framebuffer at SPL stage and I am not sure if one approach be labelled as more general above other. For e.g. Some vendors may follow same scheme as u-boot proper by calling reserve_video at SPL too and keep it near ram_top, some may keep it little away from it and some may keep it even farther.

Regards Devarsh

...

The way I look at it is at u-boot proper is getting a blob from SPL with video handoff structure and before carrying out further reservations u-boot proper being agnostic to the framebuffer address set by SPL should have some check to make sure there is no overlap with current relocaddr.

I think this is similar to suggestion from Tom and you regarding moving the ram_top if framebuffer is reserved near the ram_top, albeit instead of moving ram_top I am moving relocaddr which is derived from ram_top.

Kindly let me know if any queries.

It just doesn't make sense to me...if you want to influence U-Boot's reservation, add a handoff blob to tell U-Boot that.

Regards, Simon

...

Regards Devarsh

...

Anyway I am OK with this for now since it fixes my problems. I would prefer something positive like the Kconfig I provided, since I still think it is very weird to interrupt the U-Boot reservation process like this.> Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon

On Fri, Aug 04, 2023 at 01:10:10PM +0530, Devarsh Thakkar wrote:

Hi Simon, Tom, Bin,

Thanks for the quick feedback.

On 04/08/23 04:58, Simon Glass wrote:

...

Hi Devarsh,

On Thu, 3 Aug 2023 at 08:28, Devarsh Thakkar devarsht@ti.com wrote:

...

Hi Simon,

On 03/08/23 19:32, Simon Glass wrote:

...

+Bin Meng

Hi Devarsh,

On Tue, 1 Aug 2023 at 08:04, Devarsh Thakkar devarsht@ti.com wrote:

...

When passing framebuffer address using bloblist, check that passed address is overlapping with current relocation address, if so move the relocation address after the framebuffer region to avoid overlap.

Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot") Signed-off-by: Devarsh Thakkar devarsht@ti.com

---

common/board_f.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c index 7d2c380e91..20fa17207a 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -419,7 +419,10 @@ static int reserve_video(void) if (!ho) return log_msg_ret("blf", -ENOENT); video_reserve_from_bloblist(ho);

•           gd->relocaddr = ho->fb;
  

•           /* Relocate after framebuffer area to avoid overlap */
  

•           if (gd->relocaddr > (unsigned long)ho->fb)
  

•                   gd->relocaddr = ho->fb;
  

•   } else if (CONFIG_IS_ENABLED(VIDEO)) {
            ulong addr;
            int ret;
  

-- 2.34.1

Yes this happens to work with qemu-x86_64. However it depends on the SPL frame buffer being below the current allocation area. Why would it be below, in general? This seems like a land mine for others to trip up on.

Basically to avoid overlap, the thing we are enforcing here is that relocaddr should always be below framebuffer so that further areas reserved do not overlap with framebuffer since we decrement relocaddr to resereve them. If relocaddr is already below framebuffer then there is no need to update relocaddr as in your case otherwise we have to do it, hence working for both scenarios.

That's not true in general. Say the frame buffer is at 1GB and U-Boot's top of memory is at 2GB, then this will create a gap in the reservations, so some things are just below 2GB and some others are just below 1GB. There will be no indication of this...

Thanks Simon, I understand your point here but I think this problem is still there even with other approaches like separate Kconfig or SPL blob passing to indicate relocaddr update, as though even in that case SPL will need some base to take a decision on whether u-boot proper will require to move relocaddr or not.

For e.g. in this case there is a gap of ~1Gb between relocaddr and framebuffer and so we don't want to move relocaddr here since we assume that most likely ~1Gb is enough for u-boot to carry out and complete it's reservations without touching the framebuffer area, but this assumption may not hold true if the gap is in Mb's or Kb's.

To resolve this, the maximum value required for u-boot to carry out reservations need to be estimated and then we can make this condition more intelligent :

if (gd->relocaddr - gd->video_top < UBOOT_MAX_RESERVE_SIZE) gd->relocaddr = ho->fb;

If above looks good then, is it possible to estimate worst case common value for UBOOT_MAX_RESERVE_SIZE ? Or we want vendors to choose it say using a separate Kconfig say CONFIG_UBOOT_MAX_RESERVE_SIZE ?

Kindly let me know your feedback on above approach, I can send a V2 based on it.

Also to add, I think different vendors may choose to use different addresses for reserving framebuffer at SPL stage and I am not sure if one approach be labelled as more general above other. For e.g. Some vendors may follow same scheme as u-boot proper by calling reserve_video at SPL too and keep it near ram_top, some may keep it little away from it and some may keep it even farther.

My main comment right now is that by "vendors" I hope you mean "SoC vendors" and not "platform vendors". Within a given SoC family the address picked should be right, and if it needs adjusting (say larger display resolution) then it must be well documented how to calculate and in what direction it is safe to change it towards.