[U-Boot-Users] [PATCH] wrong lcd splashscreen colors (pxa)
Hello world :)
it seems that the computation of colors in common/lcd.c:lcd_display_bitmap() is wrong for green and blue components. I checked on version 1.1.3 and on cvs.sf.net.
I'm working on a PXA270 but this seems general (as far as I understand). The error is not evident until you use full colors (i.e. if color RGB components/bytes are either 0x00 of 0xff).
------------------------------------------------------ bmp_color_table_entry_t cte = bmp->color_table[i]; ushort colreg = ( ((cte.red) << 8) & 0xf800) | - ( ((cte.green) << 4) & 0x07e0) | - ( (cte.blue) & 0x001f) ; + ( ((cte.green) << 3) & 0x07e0) | + ( ((cte.blue) >> 3) & 0x001f) ;
#ifdef CFG_INVERT_COLORS *cmap = 0xffff - colreg; #else *cmap = colreg; #endif ------------------------------------------------------
Hoping this helps. Ciao Francesco Mandracci
PS Thank you all for the great job: it's the first time I get to a bootloader prompt (on a new hardware) after only about 6 hours of work.
In message 43299EC6.6050206@primaelectronics.com you wrote:
it seems that the computation of colors in common/lcd.c:lcd_display_bitmap() is wrong for green and blue
It's working fine on all the PowerPC systems where I am able to test.
components. I checked on version 1.1.3 and on cvs.sf.net.
I'm working on a PXA270 but this seems general (as far as I understand).
Probably not general, but maybe byte-order dependent ?
bmp_color_table_entry_t cte = bmp->color_table[i]; ushort colreg = ( ((cte.red) << 8) & 0xf800) |
( ((cte.green) << 4) & 0x07e0) |( (cte.blue) & 0x001f) ;
( ((cte.green) << 3) & 0x07e0) |( ((cte.blue) >> 3) & 0x001f) ;
Please see section "Submitting Patches" in the README. In this form this is not acceptable. Also make sure that you don't break big-endian systems with your modification.
Thank you all for the great job: it's the first time I get to a bootloader prompt (on a new hardware) after only about 6 hours of work.
Congrats!
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
It's working fine on all the PowerPC systems where I am able to test.
you forgot to quote this: The error is not evident until you use full colors (i.e. if color RGB components/bytes are either 0x00 of 0xff).
In other (maybe clearer) words: "all bitmaps with palette colors whose RGB components don't differ in the 5 high bits in the blue byte and in the 6 high bits in the green byte" will display correctly.
That may seem not so important, but if you are testing the lcd wirings of the brand new hardware then you really want each color to be ok down to the last bit! [http://www.dreamvideo.it/video/immagini/monoscopio.jpg]
I'm working on a PXA270 but this seems general (as far as I understand).
Probably not general, but maybe byte-order dependent ?
No, it's not endianness dependent: 1) "bmp_color_table_entry_t" is byte order independent (4 packed __u8 fields) 2) "ushort colreg" is used coherently in each operation
The C code fills a 16bit palette entry with 3 RGB bytes in the form
RRRR RGGG GGGB BBBB [....|....|....|....] masks: (R,G,B) (0xf800, 0x07e0, 0x001f) fedc ba98 7654 3210
The used bits are the highest 5 bits for the red and blue bytes and the 6 high bits for the green byte. This did not change neither in PowerPC nor in PXA in all cvs.sf.net revisions I've found.
The computation may be either: colreg = ((red & 0xf8) << 8) /* [RRRR|Rrrr|....|....] */ | ((green & 0xfc) << 3) /* [....|.GGG|GGGg|g...] */ | ((blue & 0xf8) >> 3); /* [....|....|...B|BBBB]bbb. */ or: colreg = ((red << 8) & 0xf800) | ((green << 3) & 0x07e0) /* versus " << 4" */ | ((blue >> 3) & 0x001f); /* versus " >> 0" */ I choose the latter form because it's the way you adopted.
Please see section "Submitting Patches" in the README. In this form this is not acceptable.
Yes Sir! :) :) :) Actually, please forgive me if I didn't follow all your recommendations. As an excuse: I understand your heavy efforts (not only in this mailing list), but it was only a two-lines very-punctual not-show-stopper patch. :)
Anyway, if we'll going to use Das U-Boot in the company product then I'll ask for MACH_TYPE_ stuff and propose some bigger patches regarding CONFIGS_PXA27x. But today there is still nothing stable enough on my side... with the exception of this little splashscreen issue.
Ciao Francesco Mandracci
In message 432AA14F.8070801@primaelectronics.com you wrote:
Please see section "Submitting Patches" in the README. In this form this is not acceptable.
Yes Sir! :) :) :)
Thanks :-)
As an excuse: I understand your heavy efforts (not only in this mailing list), but it was only a two-lines very-punctual not-show-stopper patch. :)
Nevertheless it is important to me to be able to apply this with as little effort as possible, and a proper patch with CHANGELOG entry and sign-off line and everything greatly helps. So pleast resubmit as proper patch.
Anyway, if we'll going to use Das U-Boot in the company product then I'll ask for MACH_TYPE_ stuff and propose some bigger patches regarding CONFIGS_PXA27x. But today there is still nothing stable enough on my side... with the exception of this little splashscreen issue.
It's better not to wait until then, and keep functionally different patcehs separated (actually this is another of the requirements anyway).
Thanks.
Best regards,
Wolfgang Denk
participants (2)
-
Francesco Mandracci -
Wolfgang Denk