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

Devarsh Thakkar
2023-04-03T11:30:23Z

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