drivers:lcd: fix unaligned access on lcd
PATCH 1/1 drivers:lcd: fix unaligned access on lcd
This patch replace 'le32_to_cpu' function with 'get_unaligend_le32' to avoid unaligned access exception on some ARM platforms (ex Trats2).
--- common/lcd.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
common/lcd.c +7 -5
@@ -42,6 +42,7 @@#endif #include <lcd.h> #include <watchdog.h> +#include <asm/unaligned.h> #if defined(CONFIG_CPU_PXA25X ) || defined(CONFIG_CPU_PXA27X ) || \ defined(CONFIG_CPU_MONAHANS )
@@ -907,9 +908,9 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)return 1 ; } - width =le32_to_cpu (bmp->header.width); - height =le32_to_cpu (bmp->header.height); - bmp_bpix =le16_to_cpu (bmp->header.bit_count); + width =get_unaligned_le32 (&bmp->header.width); + height =get_unaligned_le32 (&bmp->header.height); + bmp_bpix =get_unaligned_le32 (&bmp->header.bit_count); colors =1 << bmp_bpix; bpix =NBITS (panel_info.vl_bpix);
@@ -994,7 +995,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)if ((y + height) > panel_info.vl_row) height = panel_info.vl_row - y; - bmap = (uchar *) bmp +le32_to_cpu (bmp->header.data_offset); + bmap = (uchar *)bmp +get_unaligned_le32 (&bmp->header.data_offset); fb = (uchar *) (lcd_base + (y + height -1 ) * lcd_line_length + x * bpix /8 );
@@ -1002,7 +1003,8 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)case 1 :/* pass through */ case 8 :#ifdef CONFIG_LCD_BMP_RLE8 -if (le32_to_cpu (bmp->header.compression) ==BMP_BI_RLE8 ) { +if (get_unaligned_le32 (&bmp->header.compression) == +BMP_BI_RLE8 ) {if (bpix !=16 ) {/* TODO implement render code for bpix != 16 */ printf ("Error: only support 16 bpix" );
Signature · 1 line
1.7.9.5
PATCH 1/1 drivers:lcd: fix unaligned access on lcd
Dear Piotr Wilczek,
Quoted 2 lines · In message <[email protected]> you wrote:
This patch replace '
le32_to_cpu' function with 'get_unaligend_le32' to avoid unaligned access exception on some ARM platforms (ex Trats2).
I think we've been discussng this before, and then decided to rather make sure that there will be no unaligned accesses because the header will be kept properly aligned.
What is your new use case that requires this?
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "Spock, did you see the looks on their faces?" "Yes, Captain, a sort of vacant contentment."
PATCH 1/1 drivers:lcd: fix unaligned access on lcd
Dear Wolfgang Denk,
Quoted 12 lines
-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Friday, May 24, 2013 8:33 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; Kyungmin Park
Subject: Re: [U-Boot] [PATCH] drivers:lcd: fix unaligned access on lcdDear Piotr Wilczek,
In message <[email protected]>
Quoted 1 line · you
This patch replace '
le32_to_cpu' function with 'get_unaligend_le32'
Quoted 8 lines
to
Quoted 1 line
avoid unaligned access exception on some ARM platforms (ex Trats2).
I think we've been discussng this before, and then decided to rather make sure that there will be no unaligned accesses because the header will be kept properly aligned.
What is your new use case that requires this?
My case is a bmp unzipped to a 4-byte aligned address. The two signature bytes of the bmp header make the other fields 4-byte unaligned. We could force unzip the bmp to an aligned address + 2.
Is this a better solution?
Quoted 11 lines
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "Spock, did you see the looks on their faces?"
"Yes, Captain, a sort of vacant contentment."
Best regards,
Piotr Wilczek
PATCH 1/1 drivers:lcd: fix unaligned access on lcd
Hi Piotr,
Quoted 9 lines · Piotr Wilczek
Dear Wolfgang Denk,
Quoted 7 lines
-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Friday, May 24, 2013 8:33 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; Kyungmin Park
Subject: Re: [U-Boot] [PATCH] drivers:lcd: fix unaligned access on lcd
[...]
Quoted 4 lines
My case is a bmp unzipped to a 4-byte aligned address. The two signature bytes of the bmp header make the other fields 4-byte unaligned. We could force unzip the bmp to an aligned address + 2.
Is this a better solution?
That is the solution we settled on the last time this was discussed. See CONFIG_SPLASHIMAGE_GUARD in the README file if your use case involves user input, or just pick an aligned address + 2 in your code.
Signature · 2 lines
Regards, Nikita.
PATCH 1/1 drivers:lcd: fix unaligned access on lcd
In reply to Piotr WilczekHello Piotr,
Quoted 7 lines · Piotr Wilczek
This patch replace '
le32_to_cpu' function with 'get_unaligend_le32' to avoid unaligned access exception on some ARM platforms (ex Trats2).Signed-off-by Piotr Wilczek <[email protected]>Signed-off-by Kyungmin Park <[email protected]>CC Minkyu Kang <[email protected]>CC Anatolij Gustschin <[email protected]>
Quoted 5 lines
--- common/lcd.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
ntf("Error: only support 16 bpix");
If you use an aligned address + 2 as the BMP location, the aligned accesses should work. If you have user input (which I doubt) you can set CONFIG_SPLASHIMAGE_GUARD (see README).
The cm_t35 LCD thread has more info if you are interested.
Regards,
Jeroen
PATCH 1/1 lcd: align bmp header when uncopmressing image
In reply to Piotr WilczekWhen compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
--- common/cmd_bmp.c | 40 +++++++++++++++++++++++++++------------- include/lcd.h | 3 ++- 2 files changed, 29 insertions(+), 14 deletions(-)
common/cmd_bmp.c +27 -13
@@ -38,14 +38,19 @@ static int bmp_info (ulong addr);/* * Allocate and decompress a BMP image using gunzip(). * -* Returns a pointer to the decompressed image data. Must be freed by -* the caller after use. +* Returns a pointer to the decompressed image data. This pointer is +* 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 -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;
@@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Error: malloc in gunzip failed!\n" );return NULL ; } -if (gunzip (dst,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) { + + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)(((unsigned int )dst +0x02 ) & ~0x01 ); + +if (gunzip (bmp,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) {free (dst);return NULL ; }
@@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n" ); - bmp = dst; -/* * Check for bmp mark 'BM' */
@@ -81,10 +91,12 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)debug ("Gzipped BMP image detected!\n" ); + *alloc_addr = dst; return bmp; }#else -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {return NULL ; }
@@ -189,11 +201,12 @@ U_BOOT_CMD(static int bmp_info (ulong addr) {bmp_image_t *bmp=(bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (bmp ==NULL ) {printf ("There is no valid bmp file at the given address\n" );
@@ -205,8 +218,8 @@ static int bmp_info(ulong addr)printf ("Bits per pixel: %d\n" ,le16_to_cpu (bmp->header.bit_count));printf ("Compression : %d\n" ,le32_to_cpu (bmp->header.compression)); -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return (0 ); }
@@ -225,11 +238,12 @@ int bmp_display(ulong addr, int x, int y) {int ret;bmp_image_t *bmp = (bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (!bmp) {printf ("There is no valid bmp file at the given address\n" );
@@ -244,8 +258,8 @@ int bmp_display(ulong addr, int x, int y)# error bmp_display() requires CONFIG_LCD or CONFIG_VIDEO#endif -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return ret; }
include/lcd.h +2 -1
@@ -46,7 +46,8 @@ void lcd_initcolregs(void);int lcd_getfgcolor (void );/* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ -struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp); +struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr);int bmp_display (ulong addr,int x,int y);/**
Signature · 1 line
1.7.9.5
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 3 lines · In message <[email protected]> you wrote:
When compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
If you do this, you must also account for the up to 2 additional bytes needed in the allocated buffer.
Otherwise you might write over the end of the buffer...
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Die ganzen Zahlen hat der liebe Gott geschaffen, alles andere ist Menschenwerk... Leopold Kronecker
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Wolfgang Denk,
Quoted 13 lines
-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Friday, May 31, 2013 4:23 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; Minkyu Kang; Kyungmin Park; Lukasz Majewski; Anatolij Gustschin
Subject: Re: [PATCH] lcd: align bmp header when uncopmressing imageDear Piotr Wilczek,
In message <[email protected]>
Quoted 1 line · you
When compressed image is loaded, it must be decompressed to an
Quoted 9 lines
aligned
Quoted 1 line
address + 2 to avoid unaligned access exception on some ARM
platforms.
If you do this, you must also account for the up to 2 additional bytes needed in the allocated buffer.
Otherwise you might write over the end of the buffer...
Because 8-byte alignment is guaranteed by malloc I don't think might over write anything when moving by only 2 bytes. But you are right that in principle extra bytes should be allocated.
Quoted 10 lines
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Die ganzen Zahlen hat der liebe Gott geschaffen, alles andere ist
Menschenwerk... Leopold Kronecker
Best regards,
Piotr Wilczek
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 8 lines · In message <000601ce6021$fcb8f490$f62addb0$%[email protected]> you wrote:
Quoted 6 lines
If you do this, you must also account for the up to 2 additional bytes needed in the allocated buffer.
Otherwise you might write over the end of the buffer...
Because 8-byte alignment is guaranteed by malloc I don't think might over write anything when moving by only 2 bytes.
Oops??? Initial alignment has NOTHING to do with writing over the allocated end of memory!
Quoted 1 line
But you are right that in principle extra bytes should be allocated.
This is not a questions of "pricniple", but plainly of invoking undefined behaviour.
Please fix.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de A direct quote from the Boss: "We passed over a lot of good people to get the ones we hired."
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Wolfgang Denk,
Quoted 16 lines
-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Monday, June 03, 2013 1:16 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; 'Minkyu Kang'; 'Kyungmin Park'; Lukasz Majewski; 'Anatolij Gustschin'
Subject: Re: [PATCH] lcd: align bmp header when uncopmressing imageDear Piotr Wilczek,
In message <000601ce6021$fcb8f490$f62addb0$%[email protected]> you
Quoted 4 lines · wrote:
Quoted 4 lines
If you do this, you must also account for the up to 2 additional bytes needed in the allocated buffer.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Otherwise you might write over the end of the buffer...
Because 8-byte alignment is guaranteed by malloc I don't think might over write anything when moving by only 2 bytes.
Oops??? Initial alignment has NOTHING to do with writing over the allocated end of memory!
No, I meant only that malloc allocates memory in at least 4-byte resolution. I surely should have allocated extra memory for the aligned header in the first patch.
Please see my fixed patch.
Best regards,
Piotr Wilczek
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 5 lines · In message <000b01ce6064$b52a7de0$1f7f79a0$%[email protected]> you wrote:
Quoted 3 lines
Oops??? Initial alignment has NOTHING to do with writing over the allocated end of memory!
No, I meant only that malloc allocates memory in at least 4-byte resolution.
Does it? I see no such guarantee in the specification.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de No problem is insoluble. -- Dr. Janet Wallace, "The Deadly Years", stardate 3479.4
PATCH 1/1 lcd: align bmp header when uncopmressing image
In reply to Piotr WilczekHello Piotr,
Quoted 7 lines · Piotr Wilczek
common/cmd_bmp.c +2
-1-bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 73
Quoted 10 lines
common/cmd_bmp.c +6
-1@@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Error: malloc in gunzip failed!\n" );return NULL ; } -if (gunzip (dst,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) { + + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)(((unsigned int )dst +0x02 ) & ~0x01 );
This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address.
Quoted 9 lines
common/cmd_bmp.c +2
-0+ +if (gunzip (bmp,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) {free (dst);return NULL ; }@@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n" );
Signature · 2 lines
Regards, Nikita.
PATCH 1/1 lcd: align bmp header when uncopmressing image
Hello Nikita,
Quoted 24 lines
-----Original Message-----
From: Nikita Kiryanov [mailto:nikita at compulab.co.il]
Sent: Sunday, June 02, 2013 1:06 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; Kyungmin Park
Subject: Re: [U-Boot] [PATCH] lcd: align bmp header when uncopmressing imageHello Piotr,
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr,
Quoted 7 lines · Piotr Wilczek
common/cmd_bmp.c +2
-1-bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;@@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr,unsigned long *lenp)
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)
Quoted 4 lines
common/cmd_bmp.c +0
-1puts ("Error: malloc in gunzip failed!\n" );return NULL ; } - if (gunzip(dst, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr,&len) != 0) {
Quoted 10 lines
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 73
Quoted 6 lines
common/cmd_bmp.c +6
-0+ + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)(((unsigned int )dst +0x02 ) & ~0x01 );This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address.
You are right but here I'm aligning a pointer returned by malloc which is guaranteed to be aligned to 8 bytes. In fact it is sufficient only to add two bytes.
Anyway, to make the code universal I provide a better alignment method.
Quoted 9 lines
Review bmp_image_t *gunzip_bmp(unsigned long addr, · line 75
Quoted 2 lines
(quoted diff) +2
-0+ + if (gunzip(bmp, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr,&len)
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr,
Quoted 5 lines
common/cmd_bmp.c +1
-0+!= 0) {free (dst);return NULL ; }@@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr,unsigned long *lenp)
Quoted 8 lines
Review bmp_image_t *gunzip_bmp(unsigned long addr, · line 81
Quoted 2 lines
(quoted diff) +0
-0puts("Image could be truncated" " (increaseCONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n");
Quoted 1 line
--
Regards,
Nikita.
Best regards,
Piotr
PATCH 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 11 lines · In message <000701ce6025$173886c0$45a99440$%[email protected]> you wrote:
Quoted 8 lines
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 73
Quoted 4 lines
common/cmd_bmp.c +3
-0+/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)(((unsigned int )dst +0x02 ) & ~0x01 );This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address.
You are right but here I'm aligning a pointer returned by malloc which is guaranteed to be aligned to 8 bytes. In fact it is sufficient only to add two bytes.
Such assumptions are black magic at best.
Please always consider the mentalk welfare of your follow programmers who for example might change this into a character array (say, a buffer on the stack) and suddenly experience mysetrious crashes.
Please always write code such that it is NOT based on non-obvious requirements.
Quoted 1 line
Anyway, to make the code universal I provide a better alignment method.
Thanks.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "God is a comedian playing to an audience too afraid to laugh." - Voltaire
PATCH v2 1/1 lcd: align bmp header when uncopmressing image
In reply to Piotr WilczekWhen compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
---
Changes for V2:
- add additional space for uncompressed bmp header due to alignment requirements
- fix to 32-bit-aligned-address + 2 alignent
common/cmd_bmp.c | 42 ++++++++++++++++++++++++++++-------------- include/lcd.h | 3 ++- 2 files changed, 30 insertions(+), 15 deletions(-)
common/cmd_bmp.c +28 -14
@@ -38,14 +38,19 @@ static int bmp_info (ulong addr);/* * Allocate and decompress a BMP image using gunzip(). * -* Returns a pointer to the decompressed image data. Must be freed by -* the caller after use. +* Returns a pointer to the decompressed image data. This pointer is +* 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 -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;
@@ -55,12 +60,19 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)* Decompress bmp image */ len =CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ; - dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ); + dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE +0x04 );if (dst ==NULL ) {puts ("Error: malloc in gunzip failed!\n" );return NULL ; } -if (gunzip (dst,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) { + + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)((((unsigned int )dst +0x2 ) & ~0x1 ) |0x2 ); + +if (gunzip (bmp,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) {free (dst);return NULL ; }
@@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n" ); - bmp = dst; -/* * Check for bmp mark 'BM' */
@@ -81,10 +91,12 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)debug ("Gzipped BMP image detected!\n" ); + *alloc_addr = dst; return bmp; }#else -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {return NULL ; }
@@ -189,11 +201,12 @@ U_BOOT_CMD(static int bmp_info (ulong addr) {bmp_image_t *bmp=(bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (bmp ==NULL ) {printf ("There is no valid bmp file at the given address\n" );
@@ -205,8 +218,8 @@ static int bmp_info(ulong addr)printf ("Bits per pixel: %d\n" ,le16_to_cpu (bmp->header.bit_count));printf ("Compression : %d\n" ,le32_to_cpu (bmp->header.compression)); -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return (0 ); }
@@ -225,11 +238,12 @@ int bmp_display(ulong addr, int x, int y) {int ret;bmp_image_t *bmp = (bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (!bmp) {printf ("There is no valid bmp file at the given address\n" );
@@ -244,8 +258,8 @@ int bmp_display(ulong addr, int x, int y)# error bmp_display() requires CONFIG_LCD or CONFIG_VIDEO#endif -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return ret; }
include/lcd.h +2 -1
@@ -46,7 +46,8 @@ void lcd_initcolregs(void);int lcd_getfgcolor (void );/* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ -struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp); +struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr);int bmp_display (ulong addr,int x,int y);/**
Signature · 1 line
1.7.9.5
PATCH v2 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 3 lines · In message <[email protected]> you wrote:
When compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
...
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 63
Quoted 2 lines
common/cmd_bmp.c +1
-1- dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ); + dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE +0x04 );
Why + 4? This needs a comment. And please write 4 as 4, and not as 0x4 when there is no good reason for it.
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 73
Quoted 3 lines
common/cmd_bmp.c +3
-0+/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %0x04 !=0x02 ) + bmp = (bmp_image_t *)((((unsigned int )dst +0x2 ) & ~0x1 ) |0x2 );
This is difficult to read and inefficient.
If you want to do it manually, just write it as
bmp = (bmp_image_t *)((((unsigned int)dst + 1) & ~3) + 2);
[Actually adding 3 to the "malloc()" size above would be sufficient with this].
A more readable way but slightly less efficient way might be:
u32_t n = (u32_t)dst; bmp = (bmp_image_t *)(ALIGN(n, 4) + 2);
Best regards,
Wolfgang Denk
Signature · 6 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I've got to get something inside me. Some coffee or something. And
then the world will somehow be better.
- Terry Pratchett, _Men at Arms_PATCH v3 1/1 lcd: align bmp header when uncopmressing image
In reply to Piotr WilczekWhen compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
---
Changes for V3:
- add comment on why extra space is allocated for for uncompressed bmp header
- change the + 2 aligment method as Wolfgang Denk suggested
Changes for V2:
- add additional space for uncompressed bmp header due to alignment requirements
- fix to 32-bit-aligned-address + 2 alignent
common/cmd_bmp.c | 43 +++++++++++++++++++++++++++++-------------- include/lcd.h | 3 ++- 2 files changed, 31 insertions(+), 15 deletions(-)
common/cmd_bmp.c +29 -14
@@ -38,14 +38,19 @@ static int bmp_info (ulong addr);/* * Allocate and decompress a BMP image using gunzip(). * -* Returns a pointer to the decompressed image data. Must be freed by -* the caller after use. +* Returns a pointer to the decompressed image data. This pointer is +* 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 -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;
@@ -55,12 +60,20 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)* Decompress bmp image */ len =CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ; - dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ); +/* allocate extra 3 bytes for 32-bit-aligned-address + 2 alignment */ + dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE +3 );if (dst ==NULL ) {puts ("Error: malloc in gunzip failed!\n" );return NULL ; } -if (gunzip (dst,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) { + + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %4 !=2 ) + bmp = (bmp_image_t *)((((unsigned int )dst +1 ) & ~3 ) +2 ); + +if (gunzip (bmp,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) {free (dst);return NULL ; }
@@ -68,8 +81,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n" ); - bmp = dst; -/* * Check for bmp mark 'BM' */
@@ -81,10 +92,12 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)debug ("Gzipped BMP image detected!\n" ); + *alloc_addr = dst; return bmp; }#else -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {return NULL ; }
@@ -189,11 +202,12 @@ U_BOOT_CMD(static int bmp_info (ulong addr) {bmp_image_t *bmp=(bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (bmp ==NULL ) {printf ("There is no valid bmp file at the given address\n" );
@@ -205,8 +219,8 @@ static int bmp_info(ulong addr)printf ("Bits per pixel: %d\n" ,le16_to_cpu (bmp->header.bit_count));printf ("Compression : %d\n" ,le32_to_cpu (bmp->header.compression)); -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return (0 ); }
@@ -225,11 +239,12 @@ int bmp_display(ulong addr, int x, int y) {int ret;bmp_image_t *bmp = (bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (!bmp) {printf ("There is no valid bmp file at the given address\n" );
@@ -244,8 +259,8 @@ int bmp_display(ulong addr, int x, int y)# error bmp_display() requires CONFIG_LCD or CONFIG_VIDEO#endif -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return ret; }
include/lcd.h +2 -1
@@ -46,7 +46,8 @@ void lcd_initcolregs(void);int lcd_getfgcolor (void );/* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ -struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp); +struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr);int bmp_display (ulong addr,int x,int y);/**
Signature · 1 line
1.7.9.5
PATCH v3 1/1 lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
Quoted 3 lines · In message <[email protected]> you wrote:
When compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
...
common/cmd_bmp.c bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) · line 74
Quoted 3 lines
common/cmd_bmp.c +3
-0+/* align to 32-bit-aligned-address + 2 */ +if ((unsigned int )bmp %4 !=2 ) + bmp = (bmp_image_t *)((((unsigned int )dst +1 ) & ~3 ) +2 );
Please drop the "if()" - it is not necessary and just adds to the code size.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Never underestimate the bandwidth of a station wagon full of tapes.
-- Dr. Warren Jackson, Director, UTCSPATCH v4 1/1 lcd: align bmp header when uncopmressing image
In reply to Piotr WilczekWhen compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
---
Changes for V4:
- dropped the if condition
Changes for V3:
- add comment on why extra space is allocated for uncompressed bmp header
- change the + 2 aligment method as Wolfgang Denk suggested
Changes for V2:
- add additional space for uncompressed bmp header due to alignment requirements
- fix to 32-bit-aligned-address + 2 alignent
common/cmd_bmp.c | 42 ++++++++++++++++++++++++++++-------------- include/lcd.h | 3 ++- 2 files changed, 30 insertions(+), 15 deletions(-)
common/cmd_bmp.c +28 -14
@@ -38,14 +38,19 @@ static int bmp_info (ulong addr);/* * Allocate and decompress a BMP image using gunzip(). * -* Returns a pointer to the decompressed image data. Must be freed by -* the caller after use. +* Returns a pointer to the decompressed image data. This pointer is +* 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 -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {void *dst;unsigned long len;
@@ -55,12 +60,19 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)* Decompress bmp image */ len =CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ; - dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE ); +/* allocate extra 3 bytes for 32-bit-aligned-address + 2 alignment */ + dst =malloc (CONFIG_SYS_VIDEO_LOGO_MAX_SIZE +3 );if (dst ==NULL ) {puts ("Error: malloc in gunzip failed!\n" );return NULL ; } -if (gunzip (dst,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) { + + bmp = dst; + +/* align to 32-bit-aligned-address + 2 */ + bmp = (bmp_image_t *)((((unsigned int )dst +1 ) & ~3 ) +2 ); + +if (gunzip (bmp,CONFIG_SYS_VIDEO_LOGO_MAX_SIZE , (uchar *)addr, &len) !=0 ) {free (dst);return NULL ; }
@@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)puts ("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n" ); - bmp = dst; -/* * Check for bmp mark 'BM' */
@@ -81,10 +91,12 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp)debug ("Gzipped BMP image detected!\n" ); + *alloc_addr = dst; return bmp; }#else -bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp) +bmp_image_t *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr) {return NULL ; }
@@ -189,11 +201,12 @@ U_BOOT_CMD(static int bmp_info (ulong addr) {bmp_image_t *bmp=(bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (bmp ==NULL ) {printf ("There is no valid bmp file at the given address\n" );
@@ -205,8 +218,8 @@ static int bmp_info(ulong addr)printf ("Bits per pixel: %d\n" ,le16_to_cpu (bmp->header.bit_count));printf ("Compression : %d\n" ,le32_to_cpu (bmp->header.compression)); -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return (0 ); }
@@ -225,11 +238,12 @@ int bmp_display(ulong addr, int x, int y) {int ret;bmp_image_t *bmp = (bmp_image_t *)addr; +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 =gunzip_bmp (addr, &len, &bmp_alloc_addr);if (!bmp) {printf ("There is no valid bmp file at the given address\n" );
@@ -244,8 +258,8 @@ int bmp_display(ulong addr, int x, int y)# error bmp_display() requires CONFIG_LCD or CONFIG_VIDEO#endif -if ((unsigned long )bmp != addr) -free (bmp); +if (bmp_alloc_addr) +free (bmp_alloc_addr);return ret; }
include/lcd.h +2 -1
@@ -46,7 +46,8 @@ void lcd_initcolregs(void);int lcd_getfgcolor (void );/* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ -struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp); +struct bmp_image *gunzip_bmp (unsigned long addr,unsigned long *lenp, +void **alloc_addr);int bmp_display (ulong addr,int x,int y);/**
Signature · 1 line
1.7.9.5
PATCH v4 1/1 lcd: align bmp header when uncopmressing image
Quoted 16 lines · On Wed, 05 Jun 2013 08:14:30 +0200 Piotr Wilczek <[email protected]> wrote:
When compressed image is loaded, it must be decompressed to an aligned address + 2 to avoid unaligned access exception on some ARM platforms.
Signed-off-by Piotr Wilczek <[email protected]>Signed-off-by Kyungmin Park <[email protected]>CC Anatolij Gustschin <[email protected]>CC Wolfgang Denk <[email protected]>---
Changes for V4:
- dropped the if conditionChanges for V3:
- add comment on why extra space is allocated for uncompressed bmp header
- change the + 2 aligment method as Wolfgang Denk suggested
Quoted 7 lines
Changes for V2:
- add additional space for uncompressed bmp header due to alignment requirements
- fix to 32-bit-aligned-address + 2 alignentcommon/cmd_bmp.c | 42 ++++++++++++++++++++++++++++-------------- include/lcd.h | 3 ++- 2 files changed, 30 insertions(+), 15 deletions(-)
Applied to u-boot-video/master (slightly corrected the comment for gunzip_bmp()). Thanks!
Anatolij