[V6,08/13] cmd: bmp: Split bmp commands and functions

To enable splash screen at SPL, need to compile cmd/bmp.c which also
includes bmp commands, since SPL doesn't use commands split bmp.c into
common/bmp.c which includes all bmp functions and cmd/bmp.c which only
contains bmp commands.

Add function delclaration for bmp_info in video.h.

Signed-off-by: Nikhil M Jain <n-jain1@ti.com>
---
V6:
- Fix commit  message.
- Remove unused header files.

V5:
- Rename cmd/bmp_cmd to cmd/bmp.

V4:
- No change.

V3 (patch introduced):
- Split bmp functions and commands.

 cmd/bmp.c       | 162 +-----------------------------------------------
 common/bmp.c    | 152 +++++++++++++++++++++++++++++++++++++++++++++
 include/video.h |   7 +++
 3 files changed, 161 insertions(+), 160 deletions(-)
 create mode 100644 common/bmp.c

On Wed, 5 Apr 2023 at 01:06, Nikhil M Jain <n-jain1@ti.com> wrote:
>
> To enable splash screen at SPL, need to compile cmd/bmp.c which also
> includes bmp commands, since SPL doesn't use commands split bmp.c into
> common/bmp.c which includes all bmp functions and cmd/bmp.c which only
> contains bmp commands.
>
> Add function delclaration for bmp_info in video.h.
>
> Signed-off-by: Nikhil M Jain <n-jain1@ti.com>
> ---
> V6:
> - Fix commit  message.
> - Remove unused header files.
>
> V5:
> - Rename cmd/bmp_cmd to cmd/bmp.
>
> V4:
> - No change.
>
> V3 (patch introduced):
> - Split bmp functions and commands.
>
>  cmd/bmp.c       | 162 +-----------------------------------------------
>  common/bmp.c    | 152 +++++++++++++++++++++++++++++++++++++++++++++
>  include/video.h |   7 +++
>  3 files changed, 161 insertions(+), 160 deletions(-)
>  create mode 100644 common/bmp.c

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

Please see below


> diff --git a/common/bmp.c b/common/bmp.c
> new file mode 100644
> index 0000000000..6134ada5a7
> --- /dev/null
> +++ b/common/bmp.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2002
> + * Detlev Zundel, DENX Software Engineering, dzu@denx.de.
> + */
> +
> +/*
> + * BMP handling routines
> + */
> +
> +#include <bmp_layout.h>
> +#include <command.h>
> +#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

You can drop this #ifdef and see below

> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> +                            void **alloc_addr)
> +{
> +       void *dst;
> +       unsigned long len;
> +       struct bmp_image *bmp;
> +

if (!IS_ENABLED(CONFIG_VIDEO_BMP_GZIP))
   return NULL;

> +       /*
> +        * 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

Return: ...

> + */
> +int bmp_info(ulong addr);
> +
>  #endif
> --
> 2.34.1
>

Regards,
SImon

Hi Simon,

On 06/04/23 00:07, Simon Glass wrote:
> On Wed, 5 Apr 2023 at 01:06, Nikhil M Jain <n-jain1@ti.com> wrote:
>>
>> To enable splash screen at SPL, need to compile cmd/bmp.c which also
>> includes bmp commands, since SPL doesn't use commands split bmp.c into
>> common/bmp.c which includes all bmp functions and cmd/bmp.c which only
>> contains bmp commands.
>>
>> Add function delclaration for bmp_info in video.h.
>>
>> Signed-off-by: Nikhil M Jain <n-jain1@ti.com>
>> ---
>> V6:
>> - Fix commit  message.
>> - Remove unused header files.
>>
>> V5:
>> - Rename cmd/bmp_cmd to cmd/bmp.
>>
>> V4:
>> - No change.
>>
>> V3 (patch introduced):
>> - Split bmp functions and commands.
>>
>>   cmd/bmp.c       | 162 +-----------------------------------------------
>>   common/bmp.c    | 152 +++++++++++++++++++++++++++++++++++++++++++++
>>   include/video.h |   7 +++
>>   3 files changed, 161 insertions(+), 160 deletions(-)
>>   create mode 100644 common/bmp.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see below
> 
> 
>> diff --git a/common/bmp.c b/common/bmp.c
>> new file mode 100644
>> index 0000000000..6134ada5a7
>> --- /dev/null
>> +++ b/common/bmp.c
>> @@ -0,0 +1,152 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2002
>> + * Detlev Zundel, DENX Software Engineering, dzu@denx.de.
>> + */
>> +
>> +/*
>> + * BMP handling routines
>> + */
>> +
>> +#include <bmp_layout.h>
>> +#include <command.h>
>> +#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
> 
> You can drop this #ifdef and see below
> 
>> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>> +                            void **alloc_addr)
>> +{
>> +       void *dst;
>> +       unsigned long len;
>> +       struct bmp_image *bmp;
>> +
> 
> if (!IS_ENABLED(CONFIG_VIDEO_BMP_GZIP))
>     return NULL;
> 

I preferred to use #if to avoid compilation of code when not required.

For example,  if someone doesn't want to display a gzip bmp image they 
wouldn't want the code to be compiled, so that binary size doesn't 
increase.

>> +       /*
>> +        * 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
> 
> Return: ...
> 
>> + */
>> +int bmp_info(ulong addr);
>> +
>>   #endif
>> --
>> 2.34.1
>>
> 
> Regards,
> SImon

Hi Nikhil,

On 06/04/23 11:57, Nikhil M Jain wrote:
>>> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>>> +                            void **alloc_addr)
>>> +{
>>> +       void *dst;
>>> +       unsigned long len;
>>> +       struct bmp_image *bmp;
>>> +
>>
>> if (!IS_ENABLED(CONFIG_VIDEO_BMP_GZIP))
>>     return NULL;
>>
> 
> I preferred to use #if to avoid compilation of code when not required.
> 
> For example,  if someone doesn't want to display a gzip bmp image they
> wouldn't want the code to be compiled, so that binary size doesn't
> increase.

Both are equivalent. Compiler will optimize out the function if
CONFIG_VIDEO_BMP_GZIP is not defined.

#ifdefs are complicated to read compared to inline if()s (at least for me).

> 
>>> +       /*
>>> +        * 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
>>> + 

[...]

On 06/04/23 13:11, Vignesh Raghavendra wrote:
> Hi Nikhil,
> 
> On 06/04/23 11:57, Nikhil M Jain wrote:
>>>> +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>>>> +                            void **alloc_addr)
>>>> +{
>>>> +       void *dst;
>>>> +       unsigned long len;
>>>> +       struct bmp_image *bmp;
>>>> +
>>>
>>> if (!IS_ENABLED(CONFIG_VIDEO_BMP_GZIP))
>>>      return NULL;
>>>
>>
>> I preferred to use #if to avoid compilation of code when not required.
>>
>> For example,  if someone doesn't want to display a gzip bmp image they
>> wouldn't want the code to be compiled, so that binary size doesn't
>> increase.
> 
> Both are equivalent. Compiler will optimize out the function if
> CONFIG_VIDEO_BMP_GZIP is not defined.
> 
> #ifdefs are complicated to read compared to inline if()s (at least for me).
> 
>>
>>>> +       /*
>>>> +        * 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
>>>> +
> 
> [...]
> 

I will add a new patch which replaces #ifdef to if(CONFIG_IS_ENABLED()) 
and send V7 series.

Thank you,
Nikhil