Re: vesafb terminal for testing.

[Marco Gerards]

Vesa Jääskeläinen <address@hidden> writes:

Hi Vesa,

> I have attached patch here that adds simple terminal that uses VESA BIOS
> Extension 2.0+ for rendering terminal. It is not meant to be included as
> is in GRUB 2, but I would hope that people would test it, so I could try
> to improve it for greater compatibility (even though I have tried to
> follow the standards, there might be some glitches between implementations).

Nice!  In this email I will propose some suggestions and comments on
the patch itself.

> There are couple of things that I do not like in this patch.
>
> First one is that there should be common virtual terminal for all arch's
> and then separate graphics drivers that will do the actual drawing. In
> this patch this is all implemented in one module and it is not as pretty
> as it could be.

Right, the video driver should be a new kind of interface.  It should
have functions to get/put pixels, boxes, whatever to the screen.  I am
not sure about video interfaces, but they should be simple and
generic.  In that case I can easily write a framebuffer for the PPC,
for example.

You should keep in mind that there are many depths (1, 4, 8, 16, 24 and
32 bits) that should be supported.  They all work differently.

> Second one is that this patch uses VGA BIOS fonts to draw characters and
> it might cause some problems in some cards.

We should use the font manager and nothing else IMO.

> Now how to get started on testing:
> - insmod vesafb
> - Use vbe_list_modes to find mode number (eg. 0x111) that you want to use.
> - set vbe_mode varible ('set vbe_mode=0x111')

I will test it soon.


Some comments on the patch:

>  2005-08-14  Vesa Jaaskelainen  <address@hidden>
>  
> +     * DISTLIST: Added term/i386/pc/vesafb.c
> +     * term/i386/pc/vesafb: New file.

Can you please split this up to the generic framebuffer terminal?  The
terminal for framebuffers should be generic and the videodrivers
should be arch specific.  Or do you want to do that later?

You forgot the ".c" on the last line.

> +
> +2005-08-14  Vesa Jaaskelainen  <address@hidden>
> +
>       * DISTLIST: Added include/grub/i386/pc/vbe.h.

Why two changelog entries?  I do not see this change anywhere in the
patch.
  
> +static void
> +*real2pm(grub_vbe_farptr_t ptr)

The * is a part of the type, can you put it on the previous line?


There are some long lines, can you please make them shorter so
the maximal line length is 78 characters?

You sometimes use C++ style comments.  Can you change your comments so
they match the style described in the GCS:
http://www.gnu.org/prep/standards/html_node/Comments.html#Comments

Thanks,
Marco