Re: New patch for video subsystem...

[Marco Gerards]

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

Hi Vesa,

Thanks for your patch.  I have done a review, but you should know that
I am not a graphics expert and don't understand all of the code in
detail.  Most issues are GCS related, they don't only apply to the
line I commented on, but in general.  Please make sure all the code is
fixed and not this single line.

I hope other people will do a quick review as well, so the code can be
committed soon in a state everyone is happy with.

Thanks,
Marco

> How to use it:
> In order to have graphical console, you need to first load video driver
> (that would be vbe module) and then load videoterm module. After that
> you will need to tell what video mode you would like to use. And last
> you just initialize it with "terminal videoterm".
>
> Example command sequence:
> insmod font
> font /boot/grub/unifont.pff
> insmod terminal
> insmod vbe
> insmod videoterm
> set video_mode=1024x768
> terminal videoterm
>
> How to fine tune video mode:
> Handling of video_mode environment variable is a flexible one. You can
> specify multiple settings at one line. If video_mode is not set, it will
> use 1024x768 as a default resolution and will use highes number of
> colors available. Supported options are following:

Personally I would prefer a VGA compatible default.  The reason for
that is that not always videoterm.mod is loaded from grub.cfg.  For
example, the module might be added to the (GRUB 2) kernel when loading
GRUB via the network.

As for the name, I do not think videoterm is a good name.  For my
feeling something like `fbterm' or so is better.  Perhaps someone else
has some good suggestions.

> width=<value>
> height=<value>
> index
> rgb
> <width>x<height>x<depth>
>
> And you can make combo's of those like:
>
> set video_mode="width=1024 height=768 rgb"

I prefer to just use a simplified notation.  So 1024x768x24.  Most
people understand this notation, it will make scripting easier and
doesn't require that much code for parsing.

> +2006-02-25  Vesa Jaaskelainen  <address@hidden>
> +
> +     * DISTLIST: Added include/grub/video.h, term/videoterm.c,
> +     video/video.c.
> +     Removed term/i386/pc/vesafb.c.
> +
> +     * conf/i386-pc.rmk (pkgdata_MODULES): Added video.mod,
> +     videoterm.mod.
> +     Removed vga.mod, vesafb.mod.

Just put the removed line on the same line as where you say things
were added.  It looks a bit nicer and that is what we in general do.

> +     * font/manager.c (fill_with_default_glyph): Modified to use
> +     grub_font_glyph.
> +     (grub_font_get_glyph): Modified to use grub_font_glyph.
> +     (fontmanager): Renamed to font_manager.
> +
> +     * include/grub/font.h (grub_font_glyph): Added.

Please say `New function.' or `New variable.'.  It's in general better
to do that, you can immediatly see what it is.

> +     * include/grub/i386/pc/vbe.h (GRUB_VBE_STATUS_OK): Added.
> +     (GRUB_VBE_MEMORY_MODEL_PACKED_PIXEL): Likewise.
> +     (GRUB_VBE_MEMORY_MODEL_DIRECT_COLOR): Likewise.

`New macro.' would be better.

> +     (grub_vbe_get_controller_info): Renamed to
> +     grub_vbe_bios_get_controller_info.

we use this syntax:

        (grub_vbe_get_controller_info): Renamed from this...
        (grub_vbe_bios_get_controller_info): ... to this.  All users
        changed to use the new function name.

It's important that it is visible that the new function did not just
pop up.  So now you can look back in the changelog entry where it was
added.

> +     * include/grub/video.h: Added.

`New file.' is more consistent with the rest of GRUB.

> +     * normal/cmdline.c (cl_set_pos): Made it refresh screen, to enable
> +     better visual experience to video drivers.

Just use something like `Refresh the screen.'.  It's a general bug
fix, not only for the video drivers (although it unfortunately shows
up there :)).

> +     * term/videoterm.c: Added.
> +
> +     * video/video.c: Added

`New file.' again.

> +     * video/i386/pc/vbe.c: Refactored to new video subsystem.

Just leave away this line.  Usually this is done by using `All users
changed.' when interfaces change or functions are renamed.

> -# For vga.mod.
> -vga_mod_SOURCES = term/i386/pc/vga.c
> -vga_mod_CFLAGS = $(COMMON_CFLAGS)
> -vga_mod_LDFLAGS = $(COMMON_LDFLAGS)

Why do you remove this?

>  include $(srcdir)/conf/common.mk
> diff -ruN -x '*CVS*' grub2/font/manager.c grub2.new/font/manager.c
> --- grub2/font/manager.c      2005-11-13 17:47:09.000000000 +0200
> +++ grub2.new/font/manager.c  2006-02-25 14:16:16.000000000 +0200
> @@ -24,6 +24,7 @@
>  #include <grub/types.h>
>  #include <grub/mm.h>
>  #include <grub/font.h>
> +//#include <grub/gzio.h>

Can you remove this line?

>  struct entry
>  {
> @@ -50,6 +51,7 @@
>    struct font *font = 0;
>  
>    file = grub_file_open (filename);
> +  //file = grub_gzfile_open (filename, 1);

And this one.

>  /* Get a glyph corresponding to the codepoint CODE. Always fill BITMAP
>     and WIDTH with something, even if no glyph is found.  */
>  int
>  grub_font_get_glyph (grub_uint32_t code,
> -                  unsigned char bitmap[32], unsigned *width)
> +                  grub_font_glyph_t glyph)

The prototype was changed, but the comment wasn't.

>  {
>    struct font *font;
> +  grub_uint8_t bitmap[32];
>  
>    /* FIXME: It is necessary to cache glyphs!  */
>    
> @@ -183,12 +186,19 @@
>        if (offset)
>       {
>         grub_uint32_t w;
> +       grub_uint8_t x;
> +       grub_uint8_t y;
> +       grub_uint32_t len;

Please just use int here.  You read w from disk, so I understand why
it is a grub_uint32_t, but the others don't come from disk, right?

> -       if (grub_file_read (font->file, (char *) &w, 4) != 4)
> +       if ((len = grub_file_read (font->file, (char *) &w, 4)) != 4)

I'm sorry that I am commenting on old code, but can you change this to
some sizeof test or so?

> +       glyph->baseline = (16*3)/4;

Can you place some spaces around the operators, so:

          glyph->baseline = (16 * 3) / 4;

> +struct grub_font_glyph
> +{
> +  /* Glyph width in pixels.  */
> +  grub_uint8_t width;
> +  
> +  /* Glyph height in pixels.  */
> +  grub_uint8_t height;
> +  
> +  /* Glyph width in characters.  */
> +  grub_uint8_t char_width;
> +  
> +  /* Glyph baseline position in pixels (from up).  */
> +  grub_uint8_t baseline;
> +  
> +  /* Glyph bitmap data array of bytes in ((width + 7) / 8) * height.
> +     Bitmap is formulated by height scanlines, each scanline having
> +     width number of pixels. Pixels are coded as bits, value 1 meaning
> +     of opaque pixel and 0 is transparent. If width does not fit byte
> +     boundary, it will be padded with 0 to make it fit.  */
> +  grub_uint8_t bitmap[32];
> +};
> +
> +typedef struct grub_font_glyph *grub_font_glyph_t;

I hope Okuji will look at this, I am clueless about the font manager
as for now. :-)

>  #endif /* ! GRUB_MISC_HEADER */
> diff -ruN -x '*CVS*' grub2/include/grub/video.h grub2.new/include/grub/video.h
> --- grub2/include/grub/video.h        1970-01-01 02:00:00.000000000 +0200
> +++ grub2.new/include/grub/video.h    2006-03-02 15:33:32.650225312 +0200
> @@ -0,0 +1,300 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2002,2003,2005  Free Software Foundation, Inc.

I think it should just be 2006, right?

> +typedef grub_uint32_t grub_color_t;

I think grub_video_color_t is better.

> +struct grub_video_mode_info {

It think it is more appropriate to use `int' instead of
`grub_uint32_t' for the members of this struct.

> +};

> +  grub_err_t (*get_info) (struct grub_video_mode_info * mode_info);

Can you remove the space between `*' and `mode_info'?

> +  grub_err_t (*get_viewport) (unsigned int * x,
> +                           unsigned int * y,
> +                           unsigned int * width,
> +                           unsigned int * height);

Same here.  Please look at other places where you did this too.

> +struct grub_virtual_screen
> +{
> +  /* Dimensions of the virual screen.  */

Typo.

Can you change grub_uint32_t to ints inside this struct?

> +};


> +/* Make seure text buffer is not marked as allocated.  */

Typo: seure->sure

> +static struct grub_virtual_screen virtual_screen =
> +  {
> +    .text_buffer = 0
> +  };

You can leave away the initiazation, it is done automatically.

> +static struct grub_video_render_target *text_layer = 0;

Same here.


> +  /* Cleare new render target for text layer.  */

Typo: Cleare->Create (right?)

> +  grub_video_create_render_target (&text_layer,
> +                                   GRUB_VIDEO_MODE_TYPE_RGB
> +                                   | GRUB_VIDEO_MODE_TYPE_ALPHA,
> +                                   virtual_screen.width,
> +                                   virtual_screen.height);

> +static grub_err_t
> +grub_videoterm_init (void)
> +{
> +  char *modevar;
> +  int width = 1024;
> +  int height = 768;

Please make macros for these defaults.  In that case it can be easily
changed without looking at the code.

> +  /* Leave borders for virtual screen.  */
> +  width = mode_info.width - (2 * 10);
> +  height = mode_info.height - (2 * 10);

Can you explain this?  Isn't it better to use some font width
multiple?

> +      if (x < dirty_region.top_left_x)
> +        {
> +          dirty_region.top_left_x = x;
> +        }

You don't have to use { and } for a singe line.  It makes this code
hard to read.  So please don't use it for such if statements.

> diff -ruN -x '*CVS*' grub2/video/i386/pc/vbe.c grub2.new/video/i386/pc/vbe.c
> --- grub2/video/i386/pc/vbe.c 2005-09-19 00:04:41.000000000 +0300
> +++ grub2.new/video/i386/pc/vbe.c     2006-03-02 20:35:43.719014568 +0200
> @@ -1,6 +1,6 @@
>  /*
>   *  GRUB  --  GRand Unified Bootloader
> - *  Copyright (C) 2005  Free Software Foundation, Inc.
> + *  Copyright (C) 2005-2006  Free Software Foundation, Inc.

Please use commas to separate lines.  For legal reasons a range of
years is unfortunately not allowed.

> +      render_target->mode_info.bpp = active_mode_info.bits_per_pixel;
> +      render_target->mode_info.bytes_per_pixel = framebuffer.bytes_per_pixel;
> +      render_target->mode_info.pitch = framebuffer.bytes_per_scan_line;
> +      render_target->mode_info.number_of_colors = 256; /* TODO: fix me.  */
> +      render_target->mode_info.red_mask_size = 
> active_mode_info.red_mask_size;
> +      render_target->mode_info.red_field_pos = 
> active_mode_info.red_field_position;
> +      render_target->mode_info.green_mask_size = 
> active_mode_info.green_mask_size;
> +      render_target->mode_info.green_field_pos = 
> active_mode_info.green_field_position;
> +      render_target->mode_info.blue_mask_size = 
> active_mode_info.blue_mask_size;
> +      render_target->mode_info.blue_field_pos = 
> active_mode_info.blue_field_position;
> +      render_target->mode_info.reserved_mask_size = 
> active_mode_info.rsvd_mask_size;
> +      render_target->mode_info.reserved_field_pos = 
> active_mode_info.rsvd_field_position;

Isn't it possible to copy this in a single statement?

> +  /* If we have negative coordintes, optimize drawing to minimum.  */

Typo.

> +  width = render_target->viewport.width - grub_abs(dx);
> +  height = render_target->viewport.height - grub_abs(dy);

Please add a space after `grub_abs'.

> +  // TODO: PROOFREAD THIS!.

Ehm? :-)

> +static grub_err_t 
> +grub_video_vbe_swap_buffers (void)
> +{
> +  /* TODO: Implement buffer swapping.  */

Sin't this just activating another region in the video memory?  I
think it would be quite trivial when using VESA.

> +  /* TODO: Implement other types too.  */
> +  size = (width * 4) * height;

Please use sizeof here.

> +  /* Setup render target format.  */
> +  target->mode_info.width = width;
> +  target->mode_info.height = height;
> +  target->mode_info.mode_type = GRUB_VIDEO_MODE_TYPE_RGB 
> +                                | GRUB_VIDEO_MODE_TYPE_ALPHA;
> +  target->mode_info.bpp = 32;
> +  target->mode_info.bytes_per_pixel = 4;
> +  target->mode_info.pitch = target->mode_info.bytes_per_pixel * width;
> +  target->mode_info.number_of_colors = 256;
> +  target->mode_info.red_mask_size = 8;
> +  target->mode_info.red_field_pos = 0;
> +  target->mode_info.green_mask_size = 8;
> +  target->mode_info.green_field_pos = 8;
> +  target->mode_info.blue_mask_size = 8;
> +  target->mode_info.blue_field_pos = 16;
> +  target->mode_info.reserved_mask_size = 8;
> +  target->mode_info.reserved_field_pos = 24;

These are all defaults, right?  First of all, I prefer macros instead
of hardcoded values.

bpp is set to 32, while number of colors says 256, are you sure this
is correct?  I prefer a bpp of 8 as default, for compatibility
reasons.

> +static grub_err_t
> +grub_video_vbe_delete_render_target (struct grub_video_render_target * 
> target)
> +{
> +  /* If there is no target, then just return without error.  */
> +  if (! target)
> +    {
> +      return GRUB_ERR_NONE;
> +    }

No braces, like before.  Same for the code below.

> +static grub_err_t
> +grub_cmd_videotest (struct grub_arg_list *state __attribute__ ((unused)),
> +                    int argc __attribute__ ((unused)),
> +                    char **args __attribute__ ((unused)))

It's better to move this to another file, like commands/videotest.c.
Or do you think this is only useful for debugging, in that case better
put it in some #ifdef.