Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan <rmh@aybabtu.com>]

On Sat, Jan 02, 2010 at 07:26:08PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> I followed your suggestions. I attach new draft together with drawing of
> a blue diagonal line in example kernel (it would be way better to make
> it use it for output but it's intended to just show how to use passed
> info). I also attach an implementation of multiboot on EFI which uses
> this draft. Note that this ammendment is nothing EFI-specific and can be
> used with any linear framebuffer driver. The only reason it's in the
> same patch is because it was the branch in which I was working on
> multiboot improvements and haven't yet splitted individual changes yet

Excellent.  I'll review the Multiboot part first.

>  /* The flags for the Multiboot header.  */
>  #ifdef __ELF__
> -# define MULTIBOOT_HEADER_FLAGS		0x00000003
> +# define MULTIBOOT_HEADER_FLAGS		0x00000007
>  #else
> -# define MULTIBOOT_HEADER_FLAGS		0x00010003
> +# define MULTIBOOT_HEADER_FLAGS		0x00010007
>  #endif

This was a bit messy.  I just replaced it with an ORed macro list.  I
should have done this before, but I didn't have time to.  Sorry to break
your patch, but it's a trivial fix ;-)

> +	.long 0
> +	.long 1024
> +	.long 768
> +	.long 32

Maybe better to use 640x480 instead?  Not everyone has a large display.

>  /* The bits in the required part of flags field we don't support.  */
> -#define MULTIBOOT_UNSUPPORTED			0x0000fffc
> +#define MULTIBOOT_UNSUPPORTED                   0x0000fff8

Shouldn't this be 0xeffc ?  (0xfffc & ~0x1000)

> === modified file 'doc/multiboot.texi'
> --- doc/multiboot.texi	2010-01-01 20:02:24 +0000
> +++ doc/multiboot.texi	2010-01-02 16:58:33 +0000
> @@ -479,7 +479,8 @@
>  preferred graphics mode. Note that that is only a @emph{recommended}
>  mode by the OS image. If the mode exists, the boot loader should set
>  it, when the user doesn't specify a mode explicitly. Otherwise, the
> -boot loader should fall back to a similar mode, if available.
> +boot loader should fall back to a similar mode, if available. Boot loader
> +may also choose another mode if it sees fit.

I agree with changing this, but the phrases seem to contradict each other.  If
the mode exists, what should bootloader do?

I suggest we remove from "If the mode exists..." untill
"...if available", leaving your "Boot loader may also..." only.

> @@ -488,7 +489,9 @@
>  Contains @samp{0} for linear graphics mode or @samp{1} for
>  EGA-standard text mode. Everything else is reserved for future
>  expansion. Note that the boot loader may set a text mode, even if this
> -field contains @samp{0}.
> +field contains @samp{0}. If video adapter doesn't support EGA text mode or
> +bootloader doesn't know how to initialise this mode it may set video
> +mode even if field contains @samp{1}

If the EGA option is obsolete/useless, I'd just make it:

  Reserved for backward compatibility.  Always contains @samp{0}.

and schedule it for removal in next major version.

>  84      | vbe_interface_off |
>  86      | vbe_interface_len |
>          +-------------------+
> +88      | framebuffer_addr  |    (present if flags[12] is set)
> +96      | framebuffer_pitch |
> +100     | framebuffer_width |
> +104     | framebuffer_height|
> +108     | framebuffer_bpp   |
> +109     | framebuffer_type  |
> +110-115 | color_info        |

Perhaps it would be simpler for us to arrange it so that flags 11 and 12
can't be used at the same time.  We could just say that flag 11 is reserved
and unused, and then put these declarations in the offset that used to be for
VBE.  IIRC there were no possible sections after VBE, so the Framebuffer
section size wouldn't be constrained by that.

But if you prefer to keep flag 11 operational, I don't object.  I would
probably object to GRUB implementing it though.

> In this case color_info is defined as following:

I'd appreciate if a native English speaker confirms this, but I believe
this should say "as follows" (same for the other instances of this
construct).

> +@samp{framebuffer_palette_addr} contains address of array of @samp{framebuffer_palette_num_colors} following structures:

There seems to be some word missing here.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi