Re: Native CD test results

Re: Native CD test results

[Kalamatee]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

I cant seem to find the commands to load such an image on the wiki.. could
someone be so kind as to explain how its done?

Also... does this splash image work only for the console terminal or for the
gfxterm?

Finally .. the unifont.hex.gz file linked to from the wiki seems to no
longer be available.. is there another similar one I can use?

[-- Attachment #2: Type: text/html, Size: 410 bytes --]

Re: Native CD test results

[Pavel Roskin]

On Fri, 2008-03-28 at 02:54 +0000, Kalamatee wrote:
> I cant seem to find the commands to load such an image on the wiki..
> could someone be so kind as to explain how its done?

Perhaps the grub.cfg I used on the Fedora disk would illustrate it:

set default=0
set timeout=5
if font /boot/grub/unifont.pff ; then
  set gfxmode=640x480
  insmod gfxterm
  insmod vbe
  terminal gfxterm
  insmod png
  background_image /isolinux/splash.png
fi

menuentry "Install Fedora 9 Beta" {
	linux	/isolinux/vmlinuz
	initrd	/isolinux/initrd.img
}

menuentry "Install Fedora 9 Beta (text mode)" {
	linux	/isolinux/vmlinuz text
	initrd	/isolinux/initrd.img
}

menuentry "Rescue installed system" {
	linux	/isolinux/vmlinuz rescue
	initrd	/isolinux/initrd.img
}

menuentry "Boot from local drive" {
	chainloader	(hd0)+1
}

menuentry "Test memory" {
	linux	/boot/memtest
}

 
> Also... does this splash image work only for the console terminal or
> for the gfxterm?

It only works for gfxterm.

> Finally .. the unifont.hex.gz file linked to from the wiki seems to no
> longer be available.. is there another similar one I can use?

Debian still has it:
http://ftp.de.debian.org/debian/pool/main/u/unifont/unifont_1.0.orig.tar.gz

No idea what happened to the original distributor.

-- 
Regards,
Pavel Roskin

Native CD test results

[Pavel Roskin]

Hello!

I was installing Fedora 9 Beta on several systems and used that  
opportunity to test current GRUB as well.  I've found several issues,  
and it's easier for me to put them into one message, so that it's a  
single story, but please feel free to change subject to discuss  
particular issues.

I remastered the network install CD to use GRUB.  To do that, I  
extracted the disk contents to a separate directory, added  
/boot/grub/grub.cfg to it.  I tried to emulate all functionality  
present in the isolinux menu.

It turned out that the splash image called isolinux/splash.jpg is  
actually a png file.  What's worse, GRUB won't use it because it's not  
8-bit:

$ identify splash.jpg
splash.jpg PNG 640x480 640x480+0+0 DirectClass 16-bit 436.441kb

If we want to support fancy Fedora logos with their subtle color  
changes across the screen, we may need to support 16-bit PNG images.   
I ended up converting that file to tga (which is always 8-bit), but I  
see now that converting to 8-bit PNG would work too.  My monitors are  
not to good to see the difference, but some designers won't  
compromise, I'm afraid.  While at that, It would be great to support  
XPM as well, as it's used in the patched GRUB 1 installed by Fedora.

To make the CD image, I used grub-mkrescue with the --overlay option.   
I tested the image on several machines, and it would boot with only  
one exception.  One rather old system with a Syntax SV266A motherboard  
and 800 MHz AMD Duron would hang while showing "Welcome to GRUB!"   
That system is also unique for having 3 IDE CD drives (2 CD-RWs and  
one DVD+-RW), and GRUB would hang regardless of which drive the CD is  
in.

Other systems would boot.  Older systems were showing large drive  
numbers for the CD, like hd37 or hd59 (I don't remember exact  
numbers), whereas newer systems would show the CD as hd2.

I was surprised to see that "ls" would not show partitions on the hard  
drives.  It turns out the "pc" module wasn't loaded.  Perhaps it  
should be preloaded, or maybe it would be autoloaded when a PC style  
partition table is detected.

Once I had Fedora 9 installed, I tried to install the latest GRUB on  
it.  But I would get a strange message: "Warning: syntax error  
(missing slash) in `'"

It turned out that grub_parse_color_name_pair() was called from  
normal/menu.c, which didn't know a prototype for that function.  Even  
though NULL was passed as the "name" argument,  
grub_parse_color_name_pair() would see some non-zero value.  Adding  
the declaration to normal.h fixed the problem.

It's a 32-bit system (AMD 1.1 GHz) running 32-bit Fedora, and I'm  
surprised that lack of declaration caused such problem when no floats  
or 64-bit integers were involved.  It's probably a bug in gcc 4.3.0  
since it's essentially a silent change in the calling convention for  
the most popular architecture.  But it's a bug I like to have, as it  
forces us to pay attention to function declarations.  This would be  
much more important on 64-bit systems, such as IA-64.

Perhaps we should enable more warnings.  Also, it would be great to  
make the build system less noisy by default, so that the warnings  
stand out as they should.  And I'd like to be able to check GRUB with  
sparse one day.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Bean]

On Thu, Mar 27, 2008 at 10:09 AM, Pavel Roskin <proski@gnu.org> wrote:
>  It turned out that the splash image called isolinux/splash.jpg is
>  actually a png file.  What's worse, GRUB won't use it because it's not
>  8-bit:
>
>  $ identify splash.jpg
>  splash.jpg PNG 640x480 640x480+0+0 DirectClass 16-bit 436.441kb

It's quite easy to support 16-bit png, but i don't have one to test.
would you please send the image to me ?

>  Once I had Fedora 9 installed, I tried to install the latest GRUB on
>  it.  But I would get a strange message: "Warning: syntax error
>  (missing slash) in `'"
>
>  It turned out that grub_parse_color_name_pair() was called from
>  normal/menu.c, which didn't know a prototype for that function.  Even
>  though NULL was passed as the "name" argument,
>  grub_parse_color_name_pair() would see some non-zero value.  Adding
>  the declaration to normal.h fixed the problem.

I think this is caused by compile optimization. grub use -mregparm=3
option, which means use register instead of stack to pass parameter.
When caller encounter a function without prototype, it use the c
calling convention, which conflict with callee. But i think gcc is not
to be blame here, because it just have no way to know better, we could
be calling a function in the c library, in which case, the standard
calling convention is the correct one.

-- 
Bean

Re: Native CD test results

[Pavel Roskin]

On Thu, 2008-03-27 at 16:24 +0800, Bean wrote:
> On Thu, Mar 27, 2008 at 10:09 AM, Pavel Roskin <proski@gnu.org> wrote:
> >  It turned out that the splash image called isolinux/splash.jpg is
> >  actually a png file.  What's worse, GRUB won't use it because it's not
> >  8-bit:
> >
> >  $ identify splash.jpg
> >  splash.jpg PNG 640x480 640x480+0+0 DirectClass 16-bit 436.441kb
> 
> It's quite easy to support 16-bit png, but i don't have one to test.
> would you please send the image to me ?

http://red-bean.com/proski/splash.png

> >  Once I had Fedora 9 installed, I tried to install the latest GRUB on
> >  it.  But I would get a strange message: "Warning: syntax error
> >  (missing slash) in `'"
> >
> >  It turned out that grub_parse_color_name_pair() was called from
> >  normal/menu.c, which didn't know a prototype for that function.  Even
> >  though NULL was passed as the "name" argument,
> >  grub_parse_color_name_pair() would see some non-zero value.  Adding
> >  the declaration to normal.h fixed the problem.
> 
> I think this is caused by compile optimization. grub use -mregparm=3
> option, which means use register instead of stack to pass parameter.
> When caller encounter a function without prototype, it use the c
> calling convention, which conflict with callee. But i think gcc is not
> to be blame here, because it just have no way to know better, we could
> be calling a function in the c library, in which case, the standard
> calling convention is the correct one.

That makes sense.  Thanks for the explanation.

By the way, the fix for GRUB hanging when booting from a CD is not
making any difference on the system where I discovered it initially.  I
need to look deeper.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Bean]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Thu, Mar 27, 2008 at 11:30 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Thu, 2008-03-27 at 16:24 +0800, Bean wrote:
>  > On Thu, Mar 27, 2008 at 10:09 AM, Pavel Roskin <proski@gnu.org> wrote:
>  > >  It turned out that the splash image called isolinux/splash.jpg is
>  > >  actually a png file.  What's worse, GRUB won't use it because it's not
>  > >  8-bit:
>  > >
>  > >  $ identify splash.jpg
>  > >  splash.jpg PNG 640x480 640x480+0+0 DirectClass 16-bit 436.441kb
>  >
>  > It's quite easy to support 16-bit png, but i don't have one to test.
>  > would you please send the image to me ?
>
>  http://red-bean.com/proski/splash.png

Hi,

This patch add support for 16-bit png. Unfortuanate, grub itself
doesn't support 16-bit color, so I convert the image to 8 bit to
display.

-- 
Bean

[-- Attachment #2: png.diff --]
[-- Type: text/plain, Size: 2995 bytes --]

diff --git a/video/readers/png.c b/video/readers/png.c
index fd97ca4..608fa5e 100644
--- a/video/readers/png.c
+++ b/video/readers/png.c
@@ -89,7 +89,8 @@ struct grub_png_data
 
   grub_uint32_t next_offset;
 
-  int image_width, image_height, bpp, raw_bytes;
+  int image_width, image_height, bpp, is_16bit, raw_bytes;
+  grub_uint8_t *image_data;
 
   int inside_idat, idat_remain;
 
@@ -211,6 +212,7 @@ static grub_err_t
 grub_png_decode_image_header (struct grub_png_data *data)
 {
   int color_type;
+  int color_bits;
 
   data->image_width = grub_png_get_dword (data);
   data->image_height = grub_png_get_dword (data);
@@ -218,8 +220,11 @@ grub_png_decode_image_header (struct grub_png_data *data)
   if ((!data->image_height) || (!data->image_width))
     return grub_error (GRUB_ERR_BAD_FILE_TYPE, "png: invalid image size");
 
-  if (grub_png_get_byte (data) != 8)
-    return grub_error (GRUB_ERR_BAD_FILE_TYPE, "png: bit depth must be 8");
+  color_bits = grub_png_get_byte (data);
+  if ((color_bits != 8) && (color_bits != 16))
+    return grub_error (GRUB_ERR_BAD_FILE_TYPE,
+                       "png: bit depth must be 8 or 16");
+  data->is_16bit = (color_bits == 16);
 
   color_type = grub_png_get_byte (data);
   if (color_type == PNG_COLOR_TYPE_RGB)
@@ -242,9 +247,25 @@ grub_png_decode_image_header (struct grub_png_data *data)
     return grub_error (GRUB_ERR_BAD_FILE_TYPE,
 		       "png: color type not supported");
 
+  if (data->is_16bit)
+    {
+      data->bpp <<= 1;
+
+      data->image_data = grub_malloc (data->image_height *
+                                      data->image_width *  data->bpp);
+      if (grub_errno)
+        return grub_errno;
+
+      data->cur_rgb = data->image_data;
+    }
+  else
+    {
+      data->image_data = 0;
+      data->cur_rgb = (*data->bitmap)->data;
+    }
+
   data->raw_bytes = data->image_height * (data->image_width + 1) * data->bpp;
 
-  data->cur_rgb = (*data->bitmap)->data;
   data->cur_colume = 0;
   data->first_line = 1;
 
@@ -705,6 +726,21 @@ grub_png_decode_image_data (struct grub_png_data *data)
 static const grub_uint8_t png_magic[8] =
   { 0x89, 0x50, 0x4e, 0x47, 0xd, 0xa, 0x1a, 0x0a };
 
+static void
+grub_png_convert_image (struct grub_png_data *data)
+{
+  int i;
+  grub_uint8_t *d1, *d2;
+
+  d1 = (*data->bitmap)->data;
+  d2 = data->image_data + 1;
+
+  /* Only copy the upper 8 bit.  */
+  for (i = 0; i < (data->image_width * data->image_height * data->bpp >> 1);
+       i++, d1++, d2+=2)
+    *d1 = *d2;
+}
+
 static grub_err_t
 grub_png_decode_png (struct grub_png_data *data)
 {
@@ -741,6 +777,9 @@ grub_png_decode_png (struct grub_png_data *data)
 	  break;
 
 	case PNG_CHUNK_IEND:
+          if (data->is_16bit)
+            grub_png_convert_image (data);
+
 	  return grub_errno;
 
 	default:
@@ -778,6 +817,7 @@ grub_video_reader_png (struct grub_video_bitmap **bitmap,
 
       grub_png_decode_png (data);
 
+      grub_free (data->image_data);
       grub_free (data);
     }
 

Re: Native CD test results

[Pavel Roskin]

On Fri, 2008-03-28 at 04:25 +0800, Bean wrote:

> This patch add support for 16-bit png.

Thanks was quick!  It's working for me.

>  Unfortuanate, grub itself
> doesn't support 16-bit color, so I convert the image to 8 bit to
> display.

That's better than nothing.  Thank you!

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Pavel Roskin]

On Thu, 2008-03-27 at 16:48 -0400, Pavel Roskin wrote:
> On Fri, 2008-03-28 at 04:25 +0800, Bean wrote:
> 
> > This patch add support for 16-bit png.
> 
> Thanks was quick!  It's working for me.

By the way, we should recognize images by their internal data, not by
the filenames.  As it stands now, the jpeg reader only supports files
with ".jpeg" extension, not with ".jpg", which is more popular.  And the
error message is unclear if the extension is unknown.

PNG begins with 89:50:4E:47, JPEG begins with FF:D8:FF:E0 and Targa can
start with 00:00:01:01, 00:00:02:00, 00:00:03:00, 00:00:09:01,
00:00:0A:00 and 00:00:0B:00.

If we have a good PNG support, we could drop Targa, which is rare and
obsolete.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> On Thu, 2008-03-27 at 16:48 -0400, Pavel Roskin wrote:
>> On Fri, 2008-03-28 at 04:25 +0800, Bean wrote:
>>
>>> This patch add support for 16-bit png.
>> Thanks was quick!  It's working for me.
> 
> By the way, we should recognize images by their internal data, not by
> the filenames.  As it stands now, the jpeg reader only supports files
> with ".jpeg" extension, not with ".jpg", which is more popular.  And the
> error message is unclear if the extension is unknown.

.jpg should of course be supported. But why we _should_ support others 
(incorrectly named files) ? I think file extension is good enough to 
detect images... If someone makes incorrect extensions its their 
problem... we just make sure our driver do not die at that...

Re: Native CD test results

[Pavel Roskin]

On Thu, 2008-03-27 at 23:55 +0200, Vesa Jääskeläinen wrote:
> Pavel Roskin wrote:
> > On Thu, 2008-03-27 at 16:48 -0400, Pavel Roskin wrote:
> >> On Fri, 2008-03-28 at 04:25 +0800, Bean wrote:
> >>
> >>> This patch add support for 16-bit png.
> >> Thanks was quick!  It's working for me.
> > 
> > By the way, we should recognize images by their internal data, not by
> > the filenames.  As it stands now, the jpeg reader only supports files
> > with ".jpeg" extension, not with ".jpg", which is more popular.  And the
> > error message is unclear if the extension is unknown.
> 
> .jpg should of course be supported.

Yes, I could not figure out why it wasn't working until I checked the
sources.

>  But why we _should_ support others 
> (incorrectly named files) ?

The name may be in upper case.  Or maybe it's a compressed file ending
with .gz that is being uncompressed on the fly.

>  I think file extension is good enough to 
> detect images... If someone makes incorrect extensions its their 
> problem... we just make sure our driver do not die at that...

I don't insist, but GRUB traditionally gives its users power to force
things in a specific way, for example, prevent automatic decompression
or force loading a kernel as a FreeBSD kernel.  Besides, users cannot
rename images while in GRUB.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Pavel Roskin]

On Thu, 2008-03-27 at 11:30 -0400, Pavel Roskin wrote:
> By the way, the fix for GRUB hanging when booting from a CD is not
> making any difference on the system where I discovered it initially.
> I
> need to look deeper.

Actually, it is making the difference, I must have used a wrong disk
when testing.  It's working fine.  So I've applied the patch.

And while "looking deeper", it turned out that there is a scary timebomb
in kern/i386/pc/startup.S.  Only the initial part of that file is
supposed to be uncompressed, but in fact, the error handlers of the
decompression function itself were spilling into the compressed area.
Adding even minimal debugging code would push the main part of
lzo1x_decompress() beyond the boundary defined by
GRUB_KERNEL_MACHINE_RAW_SIZE, which would crash even before "Welcome to
GRUB".

So I've applied another patch that assures that to uncompressed code
stays below GRUB_KERNEL_MACHINE_RAW_SIZE.  It would be nice to avoid any
constant here and make grub-mkimage.c use a label in the image, but it
would need more work and more testing.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> And while "looking deeper", it turned out that there is a scary timebomb
> in kern/i386/pc/startup.S.  Only the initial part of that file is
> supposed to be uncompressed, but in fact, the error handlers of the
> decompression function itself were spilling into the compressed area.
> Adding even minimal debugging code would push the main part of
> lzo1x_decompress() beyond the boundary defined by
> GRUB_KERNEL_MACHINE_RAW_SIZE, which would crash even before "Welcome to
> GRUB".
> 
> So I've applied another patch that assures that to uncompressed code
> stays below GRUB_KERNEL_MACHINE_RAW_SIZE.  It would be nice to avoid any
> constant here and make grub-mkimage.c use a label in the image, but it
> would need more work and more testing.

Yeah... I noticed that too when I was playing with IDT's some time ago 
and Bean found out the reason. I think we need to update this so that it 
will never happen. Perhaps divide code to sections or so? We could 
provide this in LD variable... hmm... I could investigate what would be 
required for it... Good place to learn more about LD scripts.

Re: Native CD test results

[Pavel Roskin]

On Sat, 2008-03-29 at 13:13 +0200, Vesa Jääskeläinen wrote:

> Yeah... I noticed that too when I was playing with IDT's some time ago 
> and Bean found out the reason. I think we need to update this so that it 
> will never happen. Perhaps divide code to sections or so? We could 
> provide this in LD variable... hmm... I could investigate what would be 
> required for it... Good place to learn more about LD scripts.

If I understand correctly, kernel.img is a binary, so grub-mkimage won't
be able to get any linker data.

I think we could use one of the fields in the beginning of startup.S to
store the size of the part that should not be compressed.  For instance,
grub_compressed_size could be used for that, and grub-mkimage read it,
use it instead of GRUB_KERNEL_MACHINE_RAW_SIZE and then replace it with
the value it would normally put there.

That's the change in startup.S

--- kern/i386/pc/startup.S
+++ kern/i386/pc/startup.S
@@ -91,7 +91,7 @@
 VARIABLE(grub_kernel_image_size)
        .long   0
 VARIABLE(grub_compressed_size)
-       .long   0
+       .long   grub_stop - _start
 VARIABLE(grub_install_dos_part)
        .long   0xFFFFFFFF
 VARIABLE(grub_install_bsd_part)

Of course, we could put another label next to grub_stop with a more
descriptive name, like grub_uncompressed_end.

lnxboot.S needs to be changed to remove GRUB_KERNEL_MACHINE_RAW_SIZE as
well.  I don't understand at the first glance why lnxboot.S needs it.
Perhaps it could be calculated in a different way.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Robert Millan]

On Wed, Mar 26, 2008 at 10:09:48PM -0400, Pavel Roskin wrote:
> 
> I was surprised to see that "ls" would not show partitions on the hard  
> drives.  It turns out the "pc" module wasn't loaded.  Perhaps it  
> should be preloaded, or maybe it would be autoloaded when a PC style  
> partition table is detected.

I think my last commit fixed that:

        * util/i386/pc/grub-mkrescue.in: Generate grub.cfg that loads needed
        modules (including all partition maps), instead of preloading them.

> Perhaps we should enable more warnings.  Also, it would be great to  
> make the build system less noisy by default, so that the warnings  
> stand out as they should.  And I'd like to be able to check GRUB with  
> sparse one day.

I would even go for -Werror mode.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: Native CD test results

[Pavel Roskin]

On Sun, 2008-04-13 at 13:08 +0200, Robert Millan wrote:
> On Wed, Mar 26, 2008 at 10:09:48PM -0400, Pavel Roskin wrote:
> > 
> > I was surprised to see that "ls" would not show partitions on the hard  
> > drives.  It turns out the "pc" module wasn't loaded.  Perhaps it  
> > should be preloaded, or maybe it would be autoloaded when a PC style  
> > partition table is detected.
> 
> I think my last commit fixed that:
> 
>         * util/i386/pc/grub-mkrescue.in: Generate grub.cfg that loads needed
>         modules (including all partition maps), instead of preloading them.

Yes, it's working now.

> > Perhaps we should enable more warnings.  Also, it would be great to  
> > make the build system less noisy by default, so that the warnings  
> > stand out as they should.  And I'd like to be able to check GRUB with  
> > sparse one day.
> 
> I would even go for -Werror mode.

We cannot go there yet.  There are some format string warnings that are
hard to fix nicely.  Sure, we can cast everything to long long and use
"%llx" to be sure, but it doesn't look nice to me.  There are some other
warnings that need work.

But we could use -Werror-implicit-function-declaration for the compilers
that understand it.  Missing declarations can cause some pretty weird
errors.

-- 
Regards,
Pavel Roskin

Re: Native CD test results

[Robert Millan]

On Sun, Apr 13, 2008 at 09:09:11PM -0400, Pavel Roskin wrote:
> 
> But we could use -Werror-implicit-function-declaration for the compilers
> that understand it.  Missing declarations can cause some pretty weird
> errors.

They do :-(

I'm fine with your proposed change.  Does anyone object?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)