[RFC PATCH 0/5] Move video memory reservation for SPL at the end of RAM

[RFC PATCH 0/5] Move video memory reservation for SPL at the end of RAM

[Devarsh Thakkar]

Move video memory reservation for SPL at end of RAM so that it does
not interefere with reservations for next stage so that the next stage
need not have holes in between for passed regions and instead it can
maintain continuity in reservations.

Also catch the bloblist before starting reservations to avoid the same
problem.

While at it, also fill missing fields in video handoff struct before
passing it to next stage.

This is as per discussions at :
For moving SPL framebuffer reservation at end of RAM:

https://lore.kernel.org/all/CAPnjgZ3xSoe_G3yrqwuAvoiVjUfZ+YQgkOR0ZTVXGT9VK8TwJg@mail.gmail.com/

For filling missing video handoff fields :
https://lore.kernel.org/all/CAPnjgZ1Hs0rNf0JDirp6YPsOQ5=QqQSP9g9qRwLoOASUV8a4cw@mail.gmail.com/ *** SUBJECT HERE ***

Devarsh Thakkar (5):
  arm: mach-k3: common: Reserve video memory from end of the RAM
  board: ti: am62x: evm: Remove video_setup from spl_board_init
  common/board_f: Catch bloblist before starting resevatinos
  video: Skip framebuffer reservation if already reserved
  video: Fill video handoff in video post probe

 arch/arm/mach-k3/common.c    | 21 +++++++++++++++++++++
 board/ti/am62x/evm.c         | 18 ------------------
 common/board_f.c             | 36 +++++++++++++++++++++++++++++++++---
 drivers/video/video-uclass.c | 33 +++++++++++++++++++++++----------
 4 files changed, 77 insertions(+), 31 deletions(-)

-- 
2.34.1

[RFC PATCH 1/5] arm: mach-k3: common: Reserve video memory from end of the RAM

[Devarsh Thakkar]

Move the function to setup video memory before page table
reservation so that framebuffer memory gets reserved from
the end of RAM.

This is as per the new policy being discussed for passing
blobs where each of the reserved areas for bloblists
to be passed need to be reserved at the end of RAM.

This is to enable the next stage to directly skip
the pre-reserved area from previous stage without
having to making any gaps/holes to accomodate those
regions which was the case if previous stage
reserved region say somewhere in the middle and not at
the end of RAM.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index cc755dd1bf..3978b9ccca 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -27,6 +27,7 @@
 #include <env.h>
 #include <elf.h>
 #include <soc.h>
+#include <video.h>
 
 #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
 enum {
@@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
 	}
 }
 
+static int video_setup(void)
+{
+	if (CONFIG_IS_ENABLED(VIDEO)) {
+		ulong addr;
+		int ret;
+
+		addr = gd->relocaddr;
+		ret = video_reserve(&addr);
+		if (ret)
+			return ret;
+		debug("Reserving %luk for video at: %08lx\n",
+		      ((unsigned long)gd->relocaddr - addr) >> 10, addr);
+		gd->relocaddr = addr;
+	}
+
+	return 0;
+}
+
 void spl_enable_dcache(void)
 {
 #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
@@ -537,6 +556,8 @@ void spl_enable_dcache(void)
 	if (ram_top >= 0x100000000)
 		ram_top = (phys_addr_t) 0x100000000;
 
+	gd->relocaddr = ram_top;
+	video_setup();
 	gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
 	gd->arch.tlb_addr &= ~(0x10000 - 1);
 	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
-- 
2.34.1

[RFC PATCH 2/5] board: ti: am62x: evm: Remove video_setup from spl_board_init

[Devarsh Thakkar]

Remove video_setup from evm_init sequence since it is now
getting called at an earlier place now to make sure
video memory is reserved at the end of RAM.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 board/ti/am62x/evm.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
index ad93908840..88e02155ee 100644
--- a/board/ti/am62x/evm.c
+++ b/board/ti/am62x/evm.c
@@ -60,27 +60,9 @@ int dram_init_banksize(void)
 }
 
 #if defined(CONFIG_SPL_BUILD)
-static int video_setup(void)
-{
-	if (CONFIG_IS_ENABLED(VIDEO)) {
-		ulong addr;
-		int ret;
-
-		addr = gd->relocaddr;
-		ret = video_reserve(&addr);
-		if (ret)
-			return ret;
-		debug("Reserving %luk for video at: %08lx\n",
-		      ((unsigned long)gd->relocaddr - addr) >> 10, addr);
-		gd->relocaddr = addr;
-	}
-
-	return 0;
-}
 
 void spl_board_init(void)
 {
-	video_setup();
 	enable_caches();
 	if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP))
 		splash_display();
-- 
2.34.1

[RFC PATCH 3/5] common/board_f: Catch bloblist before starting resevatinos

[Devarsh Thakkar]

Start reservations needed for init sequence only after catching
bloblists from previous stage.

This is to avoid catching bloblists in the middle causing
gaps while u-boot is reserving.

Adjust the relocaddr as per video hand-off information
received from previous stage so that further reservations
start only after regions reserved for previous stages

Skip reservation for video memory if it was already
filled by a bloblist.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 common/board_f.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index aef395b135..ef53aa9bee 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -403,7 +403,8 @@ __weak int arch_reserve_mmu(void)
 	return 0;
 }
 
-static int reserve_video(void)
+
+static int reserve_video_from_videoblob(void)
 {
 	if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
 		struct video_handoff *ho;
@@ -412,8 +413,36 @@ 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)) {
+
+		/* Sanity check fb from blob is before current relocaddr */
+		if (likely(gd->relocaddr > (unsigned long) ho->fb)) {
+			gd->relocaddr = ho->fb;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Check if any bloblist received specifying reserved areas from previous stage and adjust
+ * gd->relocaddr accordingly, so that we start reserving after pre-reserved areas
+ * from previous stage.
+ *
+ * NOTE:
+ * IT is recommended that all bloblists from previous stage are reserved from ram_top
+ * as next stage will simply start reserving further regions after them.
+ */
+static int setup_relocaddr_from_bloblist(void)
+{
+	reserve_video_from_videoblob();
+
+	return 0;
+}
+
+
+static int reserve_video(void)
+{
+	if (CONFIG_IS_ENABLED(VIDEO)) {
 		ulong addr;
 		int ret;
 
@@ -925,6 +954,7 @@ static const init_fnc_t init_sequence_f[] = {
 	reserve_pram,
 #endif
 	reserve_round_4k,
+	setup_relocaddr_from_bloblist,
 	arch_reserve_mmu,
 	reserve_video,
 	reserve_trace,
-- 
2.34.1

[RFC PATCH 4/5] video: Skip framebuffer reservation if already reserved

[Devarsh Thakkar]

Skip framebufer reservation if it was already reserved
from previous stage and whose information was passed
using a bloblist.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 drivers/video/video-uclass.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f743ed74c8..bac30187e7 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -123,6 +123,14 @@ int video_reserve(ulong *addrp)
 	struct udevice *dev;
 	ulong size;
 
+
+	if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
+		/* Skip allocation if already received a bloblist which
+		 * filled below fields */
+		if (gd->fb_base && gd->video_top && gd->video_bottom)
+			return 0;
+	}
+
 	gd->video_top = *addrp;
 	for (uclass_find_first_device(UCLASS_VIDEO, &dev);
 	     dev;
-- 
2.34.1

[RFC PATCH 5/5] video: Fill video handoff in video post probe

[Devarsh Thakkar]

Fill video handoff fields in video_post_probe
as at this point we have full framebuffer related
information.

Also fill all the fields available in video hand-off
struct as those were missing earlier and u-boot
framework expects them to be filled for some of the
functionalities.

Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 drivers/video/video-uclass.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index bac30187e7..2a605f8339 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -149,16 +149,6 @@ int video_reserve(ulong *addrp)
 	debug("Video frame buffers from %lx to %lx\n", gd->video_bottom,
 	      gd->video_top);
 
-	if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(VIDEO_HANDOFF)) {
-		struct video_handoff *ho;
-
-		ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
-		if (!ho)
-			return log_msg_ret("blf", -ENOENT);
-		ho->fb = *addrp;
-		ho->size = size;
-	}
-
 	return 0;
 }
 
@@ -554,6 +544,21 @@ static int video_post_probe(struct udevice *dev)
 
 	priv->fb_size = priv->line_length * priv->ysize;
 
+	/* Set up video handoff fields for passing video blob to next stage */
+	if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(BLOBLIST)) {
+		struct video_handoff *ho;
+
+		ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
+		if (!ho)
+			return log_msg_ret("blf", -ENOENT);
+		ho->fb = gd->video_bottom;
+		ho->size = gd->video_top - gd->video_bottom;
+		ho->xsize = priv->xsize;
+		ho->ysize = priv->ysize;
+		ho->line_length = priv->line_length;
+		ho->bpix = priv->bpix;
+	}
+
 	if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
 		priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
 
-- 
2.34.1

Re: [RFC PATCH 1/5] arm: mach-k3: common: Reserve video memory from end of the RAM

[Simon Glass]

Hi Devarsh,

On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Move the function to setup video memory before page table
> reservation so that framebuffer memory gets reserved from
> the end of RAM.
>
> This is as per the new policy being discussed for passing
> blobs where each of the reserved areas for bloblists
> to be passed need to be reserved at the end of RAM.
>
> This is to enable the next stage to directly skip
> the pre-reserved area from previous stage without
> having to making any gaps/holes to accomodate those
> regions which was the case if previous stage
> reserved region say somewhere in the middle and not at
> the end of RAM.
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index cc755dd1bf..3978b9ccca 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -27,6 +27,7 @@
>  #include <env.h>
>  #include <elf.h>
>  #include <soc.h>
> +#include <video.h>
>
>  #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
>  enum {
> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
>         }
>  }
>
> +static int video_setup(void)

Shouldn't this be in generic code?

> +{
> +       if (CONFIG_IS_ENABLED(VIDEO)) {
> +               ulong addr;
> +               int ret;
> +
> +               addr = gd->relocaddr;
> +               ret = video_reserve(&addr);
> +               if (ret)
> +                       return ret;
> +               debug("Reserving %luk for video at: %08lx\n",
> +                     ((unsigned long)gd->relocaddr - addr) >> 10, addr);
> +               gd->relocaddr = addr;
> +       }
> +
> +       return 0;
> +}
> +
>  void spl_enable_dcache(void)
>  {
>  #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
> @@ -537,6 +556,8 @@ void spl_enable_dcache(void)
>         if (ram_top >= 0x100000000)
>                 ram_top = (phys_addr_t) 0x100000000;
>
> +       gd->relocaddr = ram_top;
> +       video_setup();
>         gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>         gd->arch.tlb_addr &= ~(0x10000 - 1);
>         debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
> --
> 2.34.1
>

Regards,
Simon

Re: [RFC PATCH 4/5] video: Skip framebuffer reservation if already reserved

[Simon Glass]

Hi Devarsh,

On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Skip framebufer reservation if it was already reserved
> from previous stage and whose information was passed
> using a bloblist.
>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  drivers/video/video-uclass.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f743ed74c8..bac30187e7 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -123,6 +123,14 @@ int video_reserve(ulong *addrp)
>         struct udevice *dev;
>         ulong size;
>
> +
> +       if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
> +               /* Skip allocation if already received a bloblist which
> +                * filled below fields */

Comment style

/*
 * Skip allocation ...

 */

> +               if (gd->fb_base && gd->video_top && gd->video_bottom)
> +                       return 0;
> +       }
> +
>         gd->video_top = *addrp;
>         for (uclass_find_first_device(UCLASS_VIDEO, &dev);
>              dev;
> --
> 2.34.1
>

Re: [RFC PATCH 1/5] arm: mach-k3: common: Reserve video memory from end of the RAM

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 19/10/23 19:26, Simon Glass wrote:
> Hi Devarsh,
> 
> On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Move the function to setup video memory before page table
>> reservation so that framebuffer memory gets reserved from
>> the end of RAM.
>>
>> This is as per the new policy being discussed for passing
>> blobs where each of the reserved areas for bloblists
>> to be passed need to be reserved at the end of RAM.
>>
>> This is to enable the next stage to directly skip
>> the pre-reserved area from previous stage without
>> having to making any gaps/holes to accomodate those
>> regions which was the case if previous stage
>> reserved region say somewhere in the middle and not at
>> the end of RAM.
>>
>> Suggested-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>>  arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index cc755dd1bf..3978b9ccca 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -27,6 +27,7 @@
>>  #include <env.h>
>>  #include <elf.h>
>>  #include <soc.h>
>> +#include <video.h>
>>
>>  #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
>>  enum {
>> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
>>         }
>>  }
>>
>> +static int video_setup(void)
> 
> Shouldn't this be in generic code?
> 

The reason I kept it here instead of video-uclass was since this function also
uses and updates gd->relocaddr and not just reserves the memory region and I
don't see any function in video-uclass playing with gd->relocaddr
and this is inspired by and parallel to reserve_video from common/board_f.c
which is also static function defined in board_f instead of video-uclass.

This basically is a wrapper function which calls video_reserve and also
uses/updates gd->relocaddr, since I am calling this for SPL I could not find
any common layer to define this since most vendors call board_init_f from
platform specific file which in turn call this function.

Could you please share your opinion and suggestions for generic location if
above still not look good?

Regards
Devarsh

>> +{
>> +       if (CONFIG_IS_ENABLED(VIDEO)) {
>> +               ulong addr;
>> +               int ret;
>> +
>> +               addr = gd->relocaddr;
>> +               ret = video_reserve(&addr);
>> +               if (ret)
>> +                       return ret;
>> +               debug("Reserving %luk for video at: %08lx\n",
>> +                     ((unsigned long)gd->relocaddr - addr) >> 10, addr);
>> +               gd->relocaddr = addr;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  void spl_enable_dcache(void)
>>  {
>>  #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
>> @@ -537,6 +556,8 @@ void spl_enable_dcache(void)
>>         if (ram_top >= 0x100000000)
>>                 ram_top = (phys_addr_t) 0x100000000;
>>
>> +       gd->relocaddr = ram_top;
>> +       video_setup();
>>         gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>>         gd->arch.tlb_addr &= ~(0x10000 - 1);
>>         debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon

Re: [RFC PATCH 1/5] arm: mach-k3: common: Reserve video memory from end of the RAM

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On Mon, Oct 23, 2023 at 05:41:10PM +0530, Devarsh Thakkar wrote:
> Hi Simon,
> 
> Thanks for the review.
> 
> On 19/10/23 19:26, Simon Glass wrote:
> > Hi Devarsh,
> > 
> > On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>
> >> Move the function to setup video memory before page table
> >> reservation so that framebuffer memory gets reserved from
> >> the end of RAM.
> >>
> >> This is as per the new policy being discussed for passing
> >> blobs where each of the reserved areas for bloblists
> >> to be passed need to be reserved at the end of RAM.
> >>
> >> This is to enable the next stage to directly skip
> >> the pre-reserved area from previous stage without
> >> having to making any gaps/holes to accomodate those
> >> regions which was the case if previous stage
> >> reserved region say somewhere in the middle and not at
> >> the end of RAM.
> >>
> >> Suggested-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> >> ---
> >>  arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
> >>  1 file changed, 21 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> >> index cc755dd1bf..3978b9ccca 100644
> >> --- a/arch/arm/mach-k3/common.c
> >> +++ b/arch/arm/mach-k3/common.c
> >> @@ -27,6 +27,7 @@
> >>  #include <env.h>
> >>  #include <elf.h>
> >>  #include <soc.h>
> >> +#include <video.h>
> >>
> >>  #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
> >>  enum {
> >> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
> >>         }
> >>  }
> >>
> >> +static int video_setup(void)
> > 
> > Shouldn't this be in generic code?
> > 
> 
> The reason I kept it here instead of video-uclass was since this function also
> uses and updates gd->relocaddr and not just reserves the memory region and I
> don't see any function in video-uclass playing with gd->relocaddr
> and this is inspired by and parallel to reserve_video from common/board_f.c
> which is also static function defined in board_f instead of video-uclass.
> 
> This basically is a wrapper function which calls video_reserve and also
> uses/updates gd->relocaddr, since I am calling this for SPL I could not find
> any common layer to define this since most vendors call board_init_f from
> platform specific file which in turn call this function.
> 
> Could you please share your opinion and suggestions for generic location if
> above still not look good?

Unless Simon speaks up, this is probably fine enough and we can always
do further cleanups afterwards.  Please do a non-RFC where you address
Simon's minor comments, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]