[U-Boot] [PATCH v3] Add 16bpp BMP support

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Mark Jackson]

This patch adds 16bpp BMP support to the common lcd code.

Use CONFIG_BMP_16BPP and set LCD_BPP to LCD_COLOR16 to enable the code.

At the moment it's only been tested on the MIMC200 AVR32 board, but extending 
this to other platforms should be a simple task !!

Signed-off-by: Mark Jackson <mpfj@mimc.co.uk>
---

  common/lcd.c |   49 +++++++++++++++++++++++++++++++++++++++----------
  1 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index ae79051..16d6f2a 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -84,7 +84,7 @@ extern void lcd_enable (void);
  static void *lcd_logo (void);


-#if LCD_BPP == LCD_COLOR8
+#if (LCD_BPP == LCD_COLOR8) || (LCD_BPP == LCD_COLOR16)
  extern void lcd_setcolreg (ushort regno,
  				ushort red, ushort green, ushort blue);
  #endif
@@ -656,7 +656,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)

  	bpix = NBITS(panel_info.vl_bpix);

-	if ((bpix != 1) && (bpix != 8)) {
+	if ((bpix != 1) && (bpix != 8) && (bpix != 16)) {
  		printf ("Error: %d bit/pixel mode not supported by U-Boot\n",
  			bpix);
  		return 1;
@@ -738,17 +738,46 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
  	bmap = (uchar *)bmp + le32_to_cpu (bmp->header.data_offset);
  	fb   = (uchar *) (lcd_base +
  		(y + height - 1) * lcd_line_length + x);
-	for (i = 0; i < height; ++i) {
-		WATCHDOG_RESET();
-		for (j = 0; j < width ; j++)
+
+	switch (bpix) {
+	case 1: /* pass through */
+	case 8:
+		for (i = 0; i < height; ++i) {
+			WATCHDOG_RESET();
+			for (j = 0; j < width ; j++)
  #if defined(CONFIG_PXA250) || defined(CONFIG_ATMEL_LCD)
-			*(fb++) = *(bmap++);
+				*(fb++) = *(bmap++);
  #elif defined(CONFIG_MPC823) || defined(CONFIG_MCC200)
-			*(fb++)=255-*(bmap++);
+				*(fb++)=255-*(bmap++);
  #endif
-		bmap += (width - padded_line);
-		fb   -= (width + lcd_line_length);
-	}
+			bmap += (width - padded_line);
+			fb   -= (width + lcd_line_length);
+		}
+		break;
+	
+#if defined(CONFIG_BMP_16BPP)
+	case 16:
+		for (i = 0; i < height; ++i) {
+			WATCHDOG_RESET();
+			for (j = 0; j < width; j++) {
+#if defined(CONFIG_ATMEL_LCD_BGR555)
+				*(fb++) = ((bmap[0] & 0x1f) << 2) | (bmap[1] & 0x03);
+				*(fb++) = (bmap[0] & 0xe0) | ((bmap[1] & 0x7c) >> 2);
+				bmap += 2;
+#else
+				*(fb++) = *(bmap++);
+				*(fb++) = *(bmap++);
+#endif
+			}
+			bmap += (padded_line - width) * 2;
+			fb   -= (width * 2 + lcd_line_length);
+		}
+		break;
+#endif /* CONFIG_BMP_16BPP */
+
+	default:
+		break;
+	};

  	return (0);
  }

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Wolfgang Denk]

Dear Mark Jackson,

In message <497F1732.6050901@mimc.co.uk> you wrote:
> This patch adds 16bpp BMP support to the common lcd code.
> 
> Use CONFIG_BMP_16BPP and set LCD_BPP to LCD_COLOR16 to enable the code.
> 
> At the moment it's only been tested on the MIMC200 AVR32 board, but extending 
> this to other platforms should be a simple task !!
> 
> Signed-off-by: Mark Jackson <mpfj@mimc.co.uk>
> ---
> 
>   common/lcd.c |   49 +++++++++++++++++++++++++++++++++++++++----------
>   1 files changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/common/lcd.c b/common/lcd.c
> index ae79051..16d6f2a 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -84,7 +84,7 @@ extern void lcd_enable (void);
>   static void *lcd_logo (void);
> 
> 
> -#if LCD_BPP == LCD_COLOR8
> +#if (LCD_BPP == LCD_COLOR8) || (LCD_BPP == LCD_COLOR16)
>   extern void lcd_setcolreg (ushort regno,
>   				ushort red, ushort green, ushort blue);
>   #endif
> @@ -656,7 +656,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
> 
>   	bpix = NBITS(panel_info.vl_bpix);
> 
> -	if ((bpix != 1) && (bpix != 8)) {
> +	if ((bpix != 1) && (bpix != 8) && (bpix != 16)) {
>   		printf ("Error: %d bit/pixel mode not supported by U-Boot\n",
>   			bpix);
>   		return 1;
> @@ -738,17 +738,46 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
>   	bmap = (uchar *)bmp + le32_to_cpu (bmp->header.data_offset);
>   	fb   = (uchar *) (lcd_base +
>   		(y + height - 1) * lcd_line_length + x);
> -	for (i = 0; i < height; ++i) {
> -		WATCHDOG_RESET();
> -		for (j = 0; j < width ; j++)
> +
> +	switch (bpix) {
> +	case 1: /* pass through */
> +	case 8:
> +		for (i = 0; i < height; ++i) {
> +			WATCHDOG_RESET();
> +			for (j = 0; j < width ; j++)
>   #if defined(CONFIG_PXA250) || defined(CONFIG_ATMEL_LCD)
> -			*(fb++) = *(bmap++);
> +				*(fb++) = *(bmap++);
>   #elif defined(CONFIG_MPC823) || defined(CONFIG_MCC200)
> -			*(fb++)=255-*(bmap++);
> +				*(fb++)=255-*(bmap++);
>   #endif
> -		bmap += (width - padded_line);
> -		fb   -= (width + lcd_line_length);
> -	}
> +			bmap += (width - padded_line);
> +			fb   -= (width + lcd_line_length);
> +		}
> +		break;
> +	
> +#if defined(CONFIG_BMP_16BPP)
> +	case 16:
> +		for (i = 0; i < height; ++i) {
> +			WATCHDOG_RESET();
> +			for (j = 0; j < width; j++) {
> +#if defined(CONFIG_ATMEL_LCD_BGR555)
> +				*(fb++) = ((bmap[0] & 0x1f) << 2) | (bmap[1] & 0x03);
> +				*(fb++) = (bmap[0] & 0xe0) | ((bmap[1] & 0x7c) >> 2);
> +				bmap += 2;
> +#else
> +				*(fb++) = *(bmap++);
> +				*(fb++) = *(bmap++);
> +#endif
> +			}
> +			bmap += (padded_line - width) * 2;
> +			fb   -= (width * 2 + lcd_line_length);

Is it intentional that you reverse padded_line and width here, i.e.
you are sure it's not

			bmap += (width - padded_line) * 2;
?

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
If you're not part of the solution, you're part of the problem.

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Mark Jackson]

Wolfgang Denk wrote:
> Dear Mark Jackson,
> 
> In message <497F1732.6050901@mimc.co.uk> you wrote:
>> This patch adds 16bpp BMP support to the common lcd code.
>>
>> Use CONFIG_BMP_16BPP and set LCD_BPP to LCD_COLOR16 to enable the code.
>>
>> At the moment it's only been tested on the MIMC200 AVR32 board, but extending 
>> this to other platforms should be a simple task !!
>>
>> Signed-off-by: Mark Jackson <mpfj@mimc.co.uk>
>> ---
>>
>>   common/lcd.c |   49 +++++++++++++++++++++++++++++++++++++++----------
>>   1 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/common/lcd.c b/common/lcd.c
>> index ae79051..16d6f2a 100644
>> --- a/common/lcd.c
>> +++ b/common/lcd.c

<snip>

>> +			bmap += (padded_line - width) * 2;
>> +			fb   -= (width * 2 + lcd_line_length);
> 
> Is it intentional that you reverse padded_line and width here, i.e.
> you are sure it's not
> 
> 			bmap += (width - padded_line) * 2;
> ?

The "bmap += ..." line is to step forward to the start of the next line of bmp 
data, taking into account any padding bytes.

If I read the code correct, padded_line is defined as ...

	padded_line = (width&0x3) ? ((width&~0x3)+4) : (width);

... so it will always be >= width.  Correct ?

If so, then ...

	bmap += (width - padded_line) * 2;

... will be <= 0, and so will actually step bmap back into the data you've 
just used, whereas ...

	bmap += (padded_line - width) * 2;

... will be >= 0, and will step forward to the start of the next line as required.

Or have I misunderstood the bmp format and the existing code ?

Regards
Mark

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Wolfgang Denk]

Dear Mark Jackson,

In message <49817E75.7060907@mimc.co.uk> you wrote:
> >> +			bmap += (padded_line - width) * 2;
> >> +			fb   -= (width * 2 + lcd_line_length);
> > 
> > Is it intentional that you reverse padded_line and width here, i.e.
> > you are sure it's not
> > 
> > 			bmap += (width - padded_line) * 2;
> > ?
> 
> The "bmap += ..." line is to step forward to the start of the next line of bmp 
> data, taking into account any padding bytes.
> 
> If I read the code correct, padded_line is defined as ...
> 
> 	padded_line = (width&0x3) ? ((width&~0x3)+4) : (width);
> 
> ... so it will always be >= width.  Correct ?
> 
> If so, then ...
> 
> 	bmap += (width - padded_line) * 2;
> 
> ... will be <= 0, and so will actually step bmap back into the data you've 
> just used, whereas ...
> 
> 	bmap += (padded_line - width) * 2;
> 
> ... will be >= 0, and will step forward to the start of the next line as required.
> 
> Or have I misunderstood the bmp format and the existing code ?

I don't know - I'm just asking because the 16 bpp case is different
from the 1 and 8 bpp cases where the operands are swapped.

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
Genius doesn't work on an assembly line basis.  You can't simply say,
"Today I will be brilliant."
	-- Kirk, "The Ultimate Computer", stardate 4731.3

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Mark Jackson]

Wolfgang Denk wrote:
> Dear Mark Jackson,
> 
> In message <49817E75.7060907@mimc.co.uk> you wrote:

<snip>

>>
>> Or have I misunderstood the bmp format and the existing code ?
> 
> I don't know - I'm just asking because the 16 bpp case is different
> from the 1 and 8 bpp cases where the operands are swapped.

It works for me ... is that enough ?

Mark

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Guennadi Liakhovetski]

On Tue, 27 Jan 2009, Mark Jackson wrote:

> This patch adds 16bpp BMP support to the common lcd code.
> 
> Use CONFIG_BMP_16BPP and set LCD_BPP to LCD_COLOR16 to enable the code.
> 
> At the moment it's only been tested on the MIMC200 AVR32 board, but extending 
> this to other platforms should be a simple task !!
> 
> Signed-off-by: Mark Jackson <mpfj@mimc.co.uk>

It is a pity I didn't notice these patches earlier, and I didn't notice 
them, because it's only today that I realized that I needed to change this 
code too. But - for the other case  8 bit bmp on a 16 bit lcd without a 
colourmap support. I have implemented that, it works now, a patch will 
follow, but while working on it I noticed how this generic code is 
difficult to work with due to all the ifdefs and especially 
platform-specific types and code. So, looking at this your patch - do we 
really need the one more CONFIG_ define for CONFIG_BMP_16BPP? What are the 
drawbacks of adding your code unconditionally? extra 100 bytes for all 
configurations using LCD?

Another question - do you really need 16bpp bmp? I saw a discussion on 
this list, that other picture formats should not be added to U-Boot - you 
can easily convert any format to bmp. Are 256 colours really not enough 
for you? I used a real photo today as a test image, converted to an 8-bit 
bmp. It looked well enough on my qvga. And normally you use this lcd code 
to display a splashscreen, which is usually a computer-generated image, so 
256 colours should suffice? Although, I am not an expert in graphical 
desing.

If we really add more bmp formats, we also get more combinations like of 
bmp / lcd:

BMP	LCD
1-bit	1-bit
8-bit	1-bit
16-bit	1-bit
1-bit	8-bit
...

if we really want to go that way, maybe better break this code into 
several functions for different format conversions?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0901302206180.4617@axis700.grange> you wrote:
> 
> platform-specific types and code. So, looking at this your patch - do we 
> really need the one more CONFIG_ define for CONFIG_BMP_16BPP? What are the 
> drawbacks of adding your code unconditionally? extra 100 bytes for all 
> configurations using LCD?

Yes.

> Another question - do you really need 16bpp bmp? I saw a discussion on 
> this list, that other picture formats should not be added to U-Boot - you 
> can easily convert any format to bmp. Are 256 colours really not enough 
> for you? I used a real photo today as a test image, converted to an 8-bit 
> bmp. It looked well enough on my qvga. And normally you use this lcd code 
> to display a splashscreen, which is usually a computer-generated image, so 
> 256 colours should suffice? Although, I am not an expert in graphical 
> desing.

I can understand that 8 bpp doe snot satisfy anoybode with more than
just basic graphics needs.

> If we really add more bmp formats, we also get more combinations like of 
> bmp / lcd:

Not necessarily. We can always request that bitmap images match the
"natural" color depth of the display. It makes no sense to send a 16
bpp image to a 1 bpp display, nor does it vice versa.

> BMP	LCD
> 1-bit	1-bit
> 8-bit	1-bit
> 16-bit	1-bit
> 1-bit	8-bit
> ...
> 
> if we really want to go that way, maybe better break this code into 
> several functions for different format conversions?

We do NOT want to do everything that is possible, but only what is
reasonable.

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
A Freudian slip is when you say one thing but mean your mother.

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Guennadi Liakhovetski]

On Fri, 30 Jan 2009, Wolfgang Denk wrote:

> > If we really add more bmp formats, we also get more combinations like of 
> > bmp / lcd:
> 
> Not necessarily. We can always request that bitmap images match the
> "natural" color depth of the display. It makes no sense to send a 16
> bpp image to a 1 bpp display, nor does it vice versa.

Hm, then I wouldn't be able to present 8bpp BMP on i.MX31 with 16bpp 
colours?

> > BMP	LCD
> > 1-bit	1-bit
> > 8-bit	1-bit
> > 16-bit	1-bit
> > 1-bit	8-bit
> > ...
> > 
> > if we really want to go that way, maybe better break this code into 
> > several functions for different format conversions?
> 
> We do NOT want to do everything that is possible, but only what is
> reasonable.

Isn't sending RGB24 image with 256 colours to a 16-bit display reasonable?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Mark Jackson]

Wolfgang Denk wrote:
> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0901302206180.4617@axis700.grange> you wrote:
>> platform-specific types and code. So, looking at this your patch - do we 
>> really need the one more CONFIG_ define for CONFIG_BMP_16BPP? What are the 
>> drawbacks of adding your code unconditionally? extra 100 bytes for all 
>> configurations using LCD?
> 
> Yes.

In fact, there's almost a case for adding *even more* #defines to remove 
the 1bpp and 8bpp code when you've #defined your board to use 16bpp.

> 
>> Another question - do you really need 16bpp bmp? I saw a discussion on 
>> this list, that other picture formats should not be added to U-Boot - you 
>> can easily convert any format to bmp. Are 256 colours really not enough 
>> for you? I used a real photo today as a test image, converted to an 8-bit 
>> bmp. It looked well enough on my qvga. And normally you use this lcd code 
>> to display a splashscreen, which is usually a computer-generated image, so 
>> 256 colours should suffice? Although, I am not an expert in graphical 
>> desing.
> 
> I can understand that 8 bpp doe snot satisfy anoybode with more than
> just basic graphics needs.

Exactly ... in my case, I boot up linux which is also using 16bpp.  My 
aim was to have the bootsplash image displayed by u-boot, and remain 
*intact* throughtout the linux boot sequence.  Switching from 8bpp (in 
u-boot) to 16bpp (in linux) would cause some nasty screen corruption, 
and require the image to be re-displayed, which kind of spoils the whole 
concept of a boot logo.

> 
>> If we really add more bmp formats, we also get more combinations like of 
>> bmp / lcd:
> 
> Not necessarily. We can always request that bitmap images match the
> "natural" color depth of the display. It makes no sense to send a 16
> bpp image to a 1 bpp display, nor does it vice versa.

As far as I understand, U-boot was not written be some fully-fledged OS 
... rather to just allow a smooth transition from power-on to "real" OS. 
  Thus we only need to support some fairly "simple" combinations of 
options, but enough to keep the majority happy.

I guess up till now, 1bpp and 8bpp have been sufficient.

> 
>> BMP	LCD
>> 1-bit	1-bit
>> 8-bit	1-bit
>> 16-bit	1-bit
>> 1-bit	8-bit
>> ...
>>
>> if we really want to go that way, maybe better break this code into 
>> several functions for different format conversions?
> 
> We do NOT want to do everything that is possible, but only what is
> reasonable.

Exactly ... otherwise where do you stop ?  JPG, GIF, TIFF, PNG, etc ? 
We're *only* meant to be showing a simply boot up image (not view lots 
of different sized photos or movies !!), in a very controlled 
environment (i.e. no "user" options ... just what the designers want / 
require).

Regards
Mark

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Haavard Skinnemoen]

Mark Jackson wrote:
> > We do NOT want to do everything that is possible, but only what is
> > reasonable.  
> 
> Exactly ... otherwise where do you stop ?  JPG, GIF, TIFF, PNG, etc ? 
> We're *only* meant to be showing a simply boot up image (not view lots 
> of different sized photos or movies !!), in a very controlled 
> environment (i.e. no "user" options ... just what the designers want / 
> require).

Why not do it even simpler? Drop BMP and generate an image matching the
native format of the LCD controller at compile time :-)

Haavard

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Mark Jackson]

Haavard Skinnemoen wrote:
> Mark Jackson wrote:
>>> We do NOT want to do everything that is possible, but only what is
>>> reasonable.  
>> Exactly ... otherwise where do you stop ?  JPG, GIF, TIFF, PNG, etc ? 
>> We're *only* meant to be showing a simply boot up image (not view lots 
>> of different sized photos or movies !!), in a very controlled 
>> environment (i.e. no "user" options ... just what the designers want / 
>> require).
> 
> Why not do it even simpler? Drop BMP and generate an image matching the
> native format of the LCD controller at compile time :-)

Not sure if you're serious, but that'd reduce some of the functionality I was 
expecting to use.

My splashimage is stored in a particular, separate flash partition, and I'm 
intending to allow end-users to change the boot logo (via a userspace app ?) 
to customise / personalise the unit as they require (e.g. their own company logo).

Hard-coding the image would render this impossible.

Regards
Mark

[U-Boot] [PATCH v3] Add 16bpp BMP support

[Haavard Skinnemoen]

Mark Jackson wrote:
> Haavard Skinnemoen wrote:
> > Mark Jackson wrote:  
> >>> We do NOT want to do everything that is possible, but only what is
> >>> reasonable.    
> >> Exactly ... otherwise where do you stop ?  JPG, GIF, TIFF, PNG, etc ? 
> >> We're *only* meant to be showing a simply boot up image (not view lots 
> >> of different sized photos or movies !!), in a very controlled 
> >> environment (i.e. no "user" options ... just what the designers want / 
> >> require).  
> > 
> > Why not do it even simpler? Drop BMP and generate an image matching the
> > native format of the LCD controller at compile time :-)  
> 
> Not sure if you're serious, but that'd reduce some of the functionality I was 
> expecting to use.

Well, it was just a thought that struck me, so I'm not going to claim I
considered all the pros and cons.

> My splashimage is stored in a particular, separate flash partition, and I'm 
> intending to allow end-users to change the boot logo (via a userspace app ?) 
> to customise / personalise the unit as they require (e.g. their own company logo).

You can still do this if the userspace app knows how to generate an
image in the correct format -- I'm not arguing against storing the
image in a separate flash partition or anything like that. I just think
it might be possible to reduce the run-time size and complexity of
u-boot by being more strict about the image format.

You could also add support for PNG, JPEG and any format you want to the
userspace app -- this will probably be much easier than adding similar
support to u-boot itself.

> Hard-coding the image would render this impossible.

Of course. But hard-coding the image _format_ isn't the same thing. In
a way, we're already using a hard-coded image format, but it's one that
is easy to generate for the host, not one that's easy to display by the
target.

Haavard