Re: [PATCH V5 08/13] cmd: bmp: Split bmp commands and functions

[Devarsh Thakkar <devarsht@ti.com>]

On 03/04/23 13:44, Nikhil M Jain wrote:
> To enable splash screen at spl, need to compile cmd/bmp.c which also

s/spl/SPL
> includes bmp commands, since SPL can't have commands split bmp.c into

since SPL doesn't use bmp commands, split cmd/bmp.c into ...

> common/bmp.c which includes all bmp functions and cmd/bmp.c contains

cmd/bmp.c which only contains

> bmp commands.
> 
> Add delclaration for bmp_info in video.h.

s/delclaration/function declaration
> 
> Signed-off-by: Nikhil M Jain <n-jain1@ti.com>
> ---
> V5:
> - Rename cmd/bmp_cmd to cmd/bmp.
> 
> V4:
> - No change.
> 
> V3 (patch introduced):
> - Split bmp functions and commands.
> 
>  cmd/bmp.c       | 161 +-----------------------------------------------
>  common/bmp.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++
>  include/video.h |   7 +++
>  3 files changed, 162 insertions(+), 159 deletions(-)
>  create mode 100644 common/bmp.c
> 
> diff --git a/cmd/bmp.c b/cmd/bmp.c
> index 46d0d916e8..735790fda7 100644
> --- a/cmd/bmp.c
> +++ b/cmd/bmp.c
> @@ -11,82 +11,11 @@
>  #include <common.h>
>  #include <bmp_layout.h>
>  #include <command.h>
> -#include <dm.h>
> -#include <gzip.h>
>  #include <image.h>
> -#include <log.h>
> -#include <malloc.h>
>  #include <mapmem.h>
>  #include <splash.h>
>  #include <video.h>
> -#include <asm/byteorder.h>
> -

Just to confirm, please test once with u-boot proper and bmp display command
too if not done already.

> -static int bmp_info (ulong addr);
> -
> -/*
> - * Allocate and decompress a BMP image using gunzip().
> - *
> - * Returns a pointer to the decompressed image data. This pointer is
> - * aligned to 32-bit-aligned-address + 2.
> - * See doc/README.displaying-bmps for explanation.
> - *
> - * The allocation address is passed to 'alloc_addr' and must be freed
> - * by the caller after use.
> - *
> - * Returns NULL if decompression failed, or if the decompressed data
> - * didn't contain a valid BMP signature.
> - */
> -#ifdef CONFIG_VIDEO_BMP_GZIP
> -struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> -			     void **alloc_addr)
> -{
> -	void *dst;
> -	unsigned long len;
> -	struct bmp_image *bmp;
> -
> -	/*
> -	 * Decompress bmp image
> -	 */
> -	len = CONFIG_VIDEO_LOGO_MAX_SIZE;
> -	/* allocate extra 3 bytes for 32-bit-aligned-address + 2 alignment */
> -	dst = malloc(CONFIG_VIDEO_LOGO_MAX_SIZE + 3);
> -	if (!dst) {
> -		puts("Error: malloc in gunzip failed!\n");
> -		return NULL;
> -	}
> -
> -	/* align to 32-bit-aligned-address + 2 */
> -	bmp = dst + 2;
> -
> -	if (gunzip(bmp, CONFIG_VIDEO_LOGO_MAX_SIZE, map_sysmem(addr, 0),
> -		   &len)) {
> -		free(dst);
> -		return NULL;
> -	}
> -	if (len == CONFIG_VIDEO_LOGO_MAX_SIZE)
> -		puts("Image could be truncated (increase CONFIG_VIDEO_LOGO_MAX_SIZE)!\n");
> -
> -	/*
> -	 * Check for bmp mark 'BM'
> -	 */
> -	if (!((bmp->header.signature[0] == 'B') &&
> -	      (bmp->header.signature[1] == 'M'))) {
> -		free(dst);
> -		return NULL;
> -	}
> -
> -	debug("Gzipped BMP image detected!\n");
> -
> -	*alloc_addr = dst;
> -	return bmp;
> -}
> -#else
> -struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> -			     void **alloc_addr)
> -{
> -	return NULL;
> -}
> -#endif
> +#include <stdlib.h>
>  
>  static int do_bmp_info(struct cmd_tbl *cmdtp, int flag, int argc,
>  		       char *const argv[])
> @@ -137,7 +66,7 @@ static int do_bmp_display(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return CMD_RET_USAGE;
>  	}
>  
> -	 return (bmp_display(addr, x, y));
> +	return (bmp_display(addr, x, y));
>  }
>  
>  static struct cmd_tbl cmd_bmp_sub[] = {
> @@ -145,22 +74,6 @@ static struct cmd_tbl cmd_bmp_sub[] = {
>  	U_BOOT_CMD_MKENT(display, 5, 0, do_bmp_display, "", ""),
>  };
>  
> -#ifdef CONFIG_NEEDS_MANUAL_RELOC
> -void bmp_reloc(void) {
> -	fixup_cmdtable(cmd_bmp_sub, ARRAY_SIZE(cmd_bmp_sub));
> -}
> -#endif
> -
> -/*
> - * Subroutine:  do_bmp
> - *
> - * Description: Handler for 'bmp' command..
> - *
> - * Inputs:	argv[1] contains the subcommand
> - *
> - * Return:      None
> - *
> - */
>  static int do_bmp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	struct cmd_tbl *c;
> @@ -183,73 +96,3 @@ U_BOOT_CMD(
>  	"info <imageAddr>          - display image info\n"
>  	"bmp display <imageAddr> [x y] - display image at x,y"
>  );
> -
> -/*
> - * Subroutine:  bmp_info
> - *
> - * Description: Show information about bmp file in memory
> - *
> - * Inputs:	addr		address of the bmp file
> - *
> - * Return:      None
> - *
> - */
> -static int bmp_info(ulong addr)
> -{
> -	struct bmp_image *bmp = (struct bmp_image *)map_sysmem(addr, 0);
> -	void *bmp_alloc_addr = NULL;
> -	unsigned long len;
> -
> -	if (!((bmp->header.signature[0]=='B') &&
> -	      (bmp->header.signature[1]=='M')))
> -		bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr);
> -
> -	if (bmp == NULL) {
> -		printf("There is no valid bmp file at the given address\n");
> -		return 1;
> -	}
> -
> -	printf("Image size    : %d x %d\n", le32_to_cpu(bmp->header.width),
> -	       le32_to_cpu(bmp->header.height));
> -	printf("Bits per pixel: %d\n", le16_to_cpu(bmp->header.bit_count));
> -	printf("Compression   : %d\n", le32_to_cpu(bmp->header.compression));
> -
> -	if (bmp_alloc_addr)
> -		free(bmp_alloc_addr);
> -
> -	return(0);
> -}
> -
> -int bmp_display(ulong addr, int x, int y)
> -{
> -	struct udevice *dev;
> -	int ret;
> -	struct bmp_image *bmp = map_sysmem(addr, 0);
> -	void *bmp_alloc_addr = NULL;
> -	unsigned long len;
> -
> -	if (!((bmp->header.signature[0]=='B') &&
> -	      (bmp->header.signature[1]=='M')))
> -		bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr);
> -
> -	if (!bmp) {
> -		printf("There is no valid bmp file at the given address\n");
> -		return 1;
> -	}
> -	addr = map_to_sysmem(bmp);
> -
> -	ret = uclass_first_device_err(UCLASS_VIDEO, &dev);
> -	if (!ret) {
> -		bool align = false;
> -
> -		if (x == BMP_ALIGN_CENTER || y == BMP_ALIGN_CENTER)
> -			align = true;
> -
> -		ret = video_bmp_display(dev, addr, x, y, align);
> -	}
> -
> -	if (bmp_alloc_addr)
> -		free(bmp_alloc_addr);
> -
> -	return ret ? CMD_RET_FAILURE : 0;
> -}
> diff --git a/common/bmp.c b/common/bmp.c
> new file mode 100644
> index 0000000000..540d06e63f
> --- /dev/null
> +++ b/common/bmp.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2002
> + * Detlev Zundel, DENX Software Engineering, dzu@denx.de.
> + */
> +
> +/*
> + * BMP handling routines
> + */
> +
> +#include <common.h>
> +#include <bmp_layout.h>
> +#include <command.h>

Do you need above header ? please remove any extra headers if not needed.

Regards
Devarsh
> +#include <dm.h>
> +#include <gzip.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <splash.h>
> +#include <video.h>
> +#include <asm/byteorder.h>
> +
> +/*
> + * Allocate and decompress a BMP image using gunzip().
> + *
> + * Returns a pointer to the decompressed image data. This pointer is
> + * aligned to 32-bit-aligned-address + 2.
> + * See doc/README.displaying-bmps for explanation.
> + *
> + * The allocation address is passed to 'alloc_addr' and must be freed
> + * by the caller after use.
> + *
> + * Returns NULL if decompression failed, or if the decompressed data
> + * didn't contain a valid BMP signature.
> + */
> +#ifdef CONFIG_VIDEO_BMP_GZIP
> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> +			     void **alloc_addr)
> +{
> +	void *dst;
> +	unsigned long len;
> +	struct bmp_image *bmp;
> +
> +	/*
> +	 * Decompress bmp image
> +	 */
> +	len = CONFIG_VIDEO_LOGO_MAX_SIZE;
> +	/* allocate extra 3 bytes for 32-bit-aligned-address + 2 alignment */
> +	dst = malloc(CONFIG_VIDEO_LOGO_MAX_SIZE + 3);
> +	if (!dst) {
> +		puts("Error: malloc in gunzip failed!\n");
> +		return NULL;
> +	}
> +
> +	/* align to 32-bit-aligned-address + 2 */
> +	bmp = dst + 2;
> +
> +	if (gunzip(bmp, CONFIG_VIDEO_LOGO_MAX_SIZE, map_sysmem(addr, 0),
> +		   &len)) {
> +		free(dst);
> +		return NULL;
> +	}
> +	if (len == CONFIG_VIDEO_LOGO_MAX_SIZE)
> +		puts("Image could be truncated (increase CONFIG_VIDEO_LOGO_MAX_SIZE)!\n");
> +
> +	/*
> +	 * Check for bmp mark 'BM'
> +	 */
> +	if (!((bmp->header.signature[0] == 'B') &&
> +	      (bmp->header.signature[1] == 'M'))) {
> +		free(dst);
> +		return NULL;
> +	}
> +
> +	debug("Gzipped BMP image detected!\n");
> +
> +	*alloc_addr = dst;
> +	return bmp;
> +}
> +#else
> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> +			     void **alloc_addr)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +
> +#ifdef CONFIG_NEEDS_MANUAL_RELOC
> +void bmp_reloc(void) {
> +	fixup_cmdtable(cmd_bmp_sub, ARRAY_SIZE(cmd_bmp_sub));
> +}
> +#endif
> +
> +int bmp_info(ulong addr)
> +{
> +	struct bmp_image *bmp = (struct bmp_image *)map_sysmem(addr, 0);
> +	void *bmp_alloc_addr = NULL;
> +	unsigned long len;
> +
> +	if (!((bmp->header.signature[0]=='B') &&
> +	      (bmp->header.signature[1]=='M')))
> +		bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr);
> +
> +	if (bmp == NULL) {
> +		printf("There is no valid bmp file at the given address\n");
> +		return 1;
> +	}
> +
> +	printf("Image size    : %d x %d\n", le32_to_cpu(bmp->header.width),
> +	       le32_to_cpu(bmp->header.height));
> +	printf("Bits per pixel: %d\n", le16_to_cpu(bmp->header.bit_count));
> +	printf("Compression   : %d\n", le32_to_cpu(bmp->header.compression));
> +
> +	if (bmp_alloc_addr)
> +		free(bmp_alloc_addr);
> +
> +	return(0);
> +}
> +
> +int bmp_display(ulong addr, int x, int y)
> +{
> +	struct udevice *dev;
> +	int ret;
> +	struct bmp_image *bmp = map_sysmem(addr, 0);
> +	void *bmp_alloc_addr = NULL;
> +	unsigned long len;
> +
> +	if (!((bmp->header.signature[0]=='B') &&
> +	      (bmp->header.signature[1]=='M')))
> +		bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr);
> +
> +	if (!bmp) {
> +		printf("There is no valid bmp file at the given address\n");
> +		return 1;
> +	}
> +	addr = map_to_sysmem(bmp);
> +
> +	ret = uclass_first_device_err(UCLASS_VIDEO, &dev);
> +	if (!ret) {
> +		bool align = false;
> +
> +		if (x == BMP_ALIGN_CENTER || y == BMP_ALIGN_CENTER)
> +			align = true;
> +
> +		ret = video_bmp_display(dev, addr, x, y, align);
> +	}
> +
> +	if (bmp_alloc_addr)
> +		free(bmp_alloc_addr);
> +
> +	return ret ? CMD_RET_FAILURE : 0;
> +}
> diff --git a/include/video.h b/include/video.h
> index 4d99e5dc27..b38fb9081a 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -357,4 +357,11 @@ void *video_get_u_boot_logo(void);
>   */
>  int bmp_display(ulong addr, int x, int y);
>  
> +/*
> + * bmp_info() - Show information about bmp file
> + *
> + * @addr: address of bmp file
> + */
> +int bmp_info(ulong addr);
> +
>  #endif