[PATCH v2 5/9] board_f: Fix corruption of relocaddr

[Devarsh Thakkar]

Hi Simon,

Thanks for the patch.

On 30/07/23 22:46, 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 at chromium.org>
> Fixes: 5bc610a7d9d ("common: board_f: Pass frame buffer info from SPL to u-boot")
> Suggested-by: Nikhil M Jain <n-jain1 at ti.com>
> ---
> 
> Changes in v2:
> - Add a Kconfig as the suggested conditional did not work

Overall this approach looks fine too but just curious to know, what is the
ho->fb and gd->relocaddr in your case, so that we could understand your
scenario better and maybe devise a more generic condition as a later-on patch
since you mentioned that shared condition did not work for you.

> 
>  common/board_f.c      | 3 ++-
>  drivers/video/Kconfig | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index 7d2c380e91e2..5173d0a0c2d5 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;

Ok, so as I understand in your case you have enabled CONFIG_SPL_VIDEO but not
calling reserve_video in SPL stage ?

> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index b41dc60cec59..e0e07ed0cda5 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -1106,6 +1106,14 @@ 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 video being allocated to U-Boot, thus having some
{some minor suggestion for correction on above text}
avoids the memory used for framebuffer from being allocated to u-boot, thus
preventing any further memory reservations done by u-boot from overwriting the
framebuffer ?

I think as mentioned earlier, overall this approach looks ok to me too, maybe
later on we can add some conditions to check validity of passed framebuffer
address and move the relocaddr pointer more intelligently.

Regards
Devarsh


> +	  data structures overwrite the framebuffer.
> +
>  if SPL_SPLASH_SCREEN
>  
>  config SPL_SPLASH_SCREEN_ALIGN