[U-Boot] [PATCH] 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).
Signed-off-by: Piotr Wilczek p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Minkyu Kang mk7.kang@samsung.com CC: Anatolij Gustschin agust@denx.de --- common/lcd.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/common/lcd.c b/common/lcd.c index edae835..577a452 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -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");
Dear Piotr Wilczek,
In message 1369381565-13946-1-git-send-email-p.wilczek@samsung.com 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
Dear Wolfgang Denk,
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Friday, May 24, 2013 8:33 PM To: Piotr Wilczek Cc: u-boot@lists.denx.de; Kyungmin Park Subject: Re: [U-Boot] [PATCH] drivers:lcd: fix unaligned access on lcd
Dear Piotr Wilczek,
In message 1369381565-13946-1-git-send-email-p.wilczek@samsung.com 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?
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?
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@denx.de "Spock, did you see the looks on their faces?" "Yes, Captain, a sort of vacant contentment."
Best regards, Piotr Wilczek
Hi Piotr,
On 05/27/2013 02:35 PM, Piotr Wilczek wrote:
Dear Wolfgang Denk,
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Friday, May 24, 2013 8:33 PM To: Piotr Wilczek Cc: u-boot@lists.denx.de; Kyungmin Park Subject: Re: [U-Boot] [PATCH] drivers:lcd: fix unaligned access on lcd
[...]
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.
Hello Piotr,
On 05/24/2013 09:46 AM, Piotr Wilczek wrote:
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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Minkyu Kang mk7.kang@samsung.com CC: Anatolij Gustschin agust@denx.de
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
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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Anatolij Gustschin agust@denx.de CC: Wolfgang Denk: wd@denx.de --- common/cmd_bmp.c | 40 +++++++++++++++++++++++++++------------- include/lcd.h | 3 ++- 2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..fea4708 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -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; } diff --git a/include/lcd.h b/include/lcd.h index c6e7fc5..482e606 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -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);
/**
Dear Piotr Wilczek,
In message 1369999573-15449-1-git-send-email-p.wilczek@samsung.com 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
Dear Wolfgang Denk,
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Friday, May 31, 2013 4:23 PM To: Piotr Wilczek Cc: u-boot@lists.denx.de; Minkyu Kang; Kyungmin Park; Lukasz Majewski; Anatolij Gustschin Subject: Re: [PATCH] lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
In message 1369999573-15449-1-git-send-email-p.wilczek@samsung.com 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...
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.
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@denx.de Die ganzen Zahlen hat der liebe Gott geschaffen, alles andere ist Menschenwerk... Leopold Kronecker
Best regards, Piotr Wilczek
Dear Piotr Wilczek,
In message 000601ce6021$fcb8f490$f62addb0$%wilczek@samsung.com you wrote:
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!
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
Dear Wolfgang Denk,
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Monday, June 03, 2013 1:16 PM To: Piotr Wilczek Cc: u-boot@lists.denx.de; 'Minkyu Kang'; 'Kyungmin Park'; Lukasz Majewski; 'Anatolij Gustschin' Subject: Re: [PATCH] lcd: align bmp header when uncopmressing image
Dear Piotr Wilczek,
In message 000601ce6021$fcb8f490$f62addb0$%wilczek@samsung.com you wrote:
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!
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
Dear Piotr Wilczek,
In message 000b01ce6064$b52a7de0$1f7f79a0$%wilczek@samsung.com you wrote:
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
Hello Piotr,
On 05/31/2013 02:26 PM, Piotr Wilczek wrote:
-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)@@ -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.
- 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");
Hello Nikita,
-----Original Message----- From: Nikita Kiryanov [mailto:nikita@compulab.co.il] Sent: Sunday, June 02, 2013 1:06 PM To: Piotr Wilczek Cc: u-boot@lists.denx.de; Kyungmin Park Subject: Re: [U-Boot] [PATCH] lcd: align bmp header when uncopmressing image
Hello Piotr,
On 05/31/2013 02:26 PM, Piotr Wilczek wrote:
-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)@@ -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.
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.
- 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" " (increaseCONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n");
-- Regards, Nikita.
Best regards, Piotr
Dear Piotr Wilczek,
In message 000701ce6025$173886c0$45a99440$%wilczek@samsung.com you wrote:
- /* 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.
Anyway, to make the code universal I provide a better alignment method.
Thanks.
Best regards,
Wolfgang Denk
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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Anatolij Gustschin agust@denx.de CC: Wolfgang Denk wd@denx.de --- 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(-)
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..06634b2 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -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; } diff --git a/include/lcd.h b/include/lcd.h index c6e7fc5..482e606 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -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);
/**
Dear Piotr Wilczek,
In message 1370267979-17800-1-git-send-email-p.wilczek@samsung.com 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.
...
- 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.
- /* 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
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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Anatolij Gustschin agust@denx.de CC: Wolfgang Denk wd@denx.de --- 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(-)
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..1c1e2a9 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -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; } diff --git a/include/lcd.h b/include/lcd.h index c6e7fc5..482e606 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -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);
/**
Dear Piotr Wilczek,
In message 1370343625-16596-1-git-send-email-p.wilczek@samsung.com 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.
...
- /* 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
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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Anatolij Gustschin agust@denx.de CC: Wolfgang Denk wd@denx.de --- 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(-)
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..675f47a 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -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; } diff --git a/include/lcd.h b/include/lcd.h index c6e7fc5..482e606 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -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);
/**
On Wed, 05 Jun 2013 08:14:30 +0200 Piotr Wilczek p.wilczek@samsung.com 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 p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Anatolij Gustschin agust@denx.de CC: Wolfgang Denk wd@denx.de
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(-)
Applied to u-boot-video/master (slightly corrected the comment for gunzip_bmp()). Thanks!
Anatolij
participants (5)
-
Anatolij Gustschin -
Jeroen Hofstee -
Nikita Kiryanov -
Piotr Wilczek -
Wolfgang Denk