Re: New patch for video subsystem...

[Marco Gerards]

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

Hi Vesa,

Thanks a lot for the amount of work you did again!  Sorry I am this
late with reviewing the patch.  I assume if someone else wanted to
comment on your code, he would've done so by now.  Can you please
correct the patch using my comments (and check if they also apply
elsewhere in your code) and commit the patch after that?

First of all some general comments.  Please look for these issues in
the entire file, I didn't mark all:

- Between two lines in a ChangeLog entry or comment, use two spaces
  after the `.'.
- Sometimes in function calls/prototypes you can join lines by using
  multiple arguments on a line.  Please do this, but make sure the
  line does not get too long (stay below 80).
- Sometimes you use too many braces for if statements.  Can you change
  this so the code looks a bit more like the rest of GRUB?

Thanks,
Marco


> Marco Gerards wrote:
>> 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.
>
> That wasn't a small job ;).. I hope I got all right.

:-)

>> 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.
>
> Sounds fine.
>
> I have made modifications and attached it as a new diff in this message.

Cool, I will review it now.

>>> 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.
>
> Well that default is in videoterm, and if it is not loaded then I do not
> see that as a problem. We can change the default resolution easily if
> that is the case. And if you need it for other purposes currently you
> will need to tell what mode you want.
>
> Nowadays 1024x768 is a normal standard resolution.

Right.  The problem is this:

When using network boot, you can specify which modules you want to
use.  If you do not have a grub.cfg, or can't load it for some reason,
you will not get a working terminal if you only can do VGA.  I think
we can assume everyone has VGA, so we can use 640x400x4bpp.  Of course
it should be possible to select a higher resolution afterwards.

>> 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.
>
> I am all ears for suggestions.

What did you think of fbterm, as I proposed?  I think it is better,
but I do not think it is perfect, so better suggestions are still
welcome. :-)

>>> 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.
>
> Reason why I added more complex one is that there might be settings that
> would be good to be parsed to flags field.

Can you give an example?  In that case we can better have a
video_flags variable or so, I think.

>>> +   * 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.
>
> Hmm. But then there is no mentioning about changes done in this file at
> all?. (I have removed it from current diff.)

Oh, if there are other changes, they should be mentioned of course.
But in that case your comment was not sufficient anyways.

>>> -# 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?
>
> After modifying the fontmanager it would have needed to make some
> changes to VGA code. And I am not familiar with VGA planes and such, so
> I left it out.

So the VGA code is broken now?

>>> +typedef grub_uint32_t grub_color_t;
>> 
>> I think grub_video_color_t is better.
>
> It is a long one, but changed.

If you use Emacs, you can use the M-/ key combination to complete
words.  It helps a bit in such cases. ;-)

>>> +  /* 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?
>
> It was something from the "hat". Basic idea here is here to show that it
> can be anything. In future this most likely changes to something
> completely different as we want(?) to have more flexibility on configure
> things.

Oh, I see.  In that case it would be better to use a macro so the
border width can be easily changed.  And it will make the code a bit
easier to read (code with a lot of magic numbers is hard to read).

>>> +      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?
>
> No. As structure types are different.

Ok.

>>> +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.
>
> Depends if we have enough memory on card. But will be done. I was
> intending to implement this functionality after we have version
> in CVS.

Cool :-)

>> 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.
>
> number_of_colors tells how many indexed colors we have in mode. In RGB
> rendering target we have always 256 emulated indexed colors.
>
> As there is a comment in file (now perhaps better written), currently
> only RGB rendering targets are supported. This is not a huge problem as
> blit function can handle it even for indexed color modes.

Ehm?  So it doesn't work, but it does?  Sorry, I do not understand
you.


---------------------------------------------------------------------

The new patch:


> diff -ruN -x '*CVS*' grub2/ChangeLog grub2.new/ChangeLog
> --- grub2/ChangeLog   2006-02-09 00:42:47.000000000 +0200
> +++ grub2.new/ChangeLog       2006-03-03 16:51:42.214034272 +0200
> @@ -1,3 +1,110 @@
> +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.

Please use double spaces between sentences.  Both in changelog entries
and in comments.

> diff -ruN -x '*CVS*' grub2/commands/videotest.c grub2.new/commands/videotest.c
> --- grub2/commands/videotest.c        1970-01-01 02:00:00.000000000 +0200
> +++ grub2.new/commands/videotest.c    2006-03-03 16:56:57.792059120 +0200
> @@ -0,0 +1,133 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006  Free Software Foundation, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <grub/machine/memory.h>
> +#include <grub/video.h>
> +#include <grub/types.h>
> +#include <grub/dl.h>
> +#include <grub/misc.h>
> +#include <grub/normal.h>
> +#include <grub/arg.h>
> +#include <grub/mm.h>
> +#include <grub/font.h>
> +#include <grub/term.h>
> +
> +static grub_err_t
> +grub_cmd_videotest (struct grub_arg_list *state __attribute__ ((unused)),
> +                    int argc __attribute__ ((unused)),
> +                    char **args __attribute__ ((unused)))
> +{
> +  if (grub_video_setup (1024, 
> +                        768,
> +                        GRUB_VIDEO_MODE_TYPE_INDEX_COLOR) != GRUB_ERR_NONE)
> +    {
> +      return grub_errno;
> +    }

Can you remove the braces?

> +  grub_getkey ();
> +
> +  grub_video_color_t color;
> +  unsigned int x, y, width, height;

Please just use a single declaration per line.  So you would have to
split this up into 4 lines.

> +  grub_video_set_viewport (x + 150, y + 150, width - 150*2, height - 150*2);

Spaces around the `*'.

> +  color = grub_video_map_rgb (77,33,77);

Spaces after the `,'.

> +  color = grub_video_map_rgb (255,255,255);

Likewise.

> +  grub_video_blit_glyph (&glyph, color, 16*2, 16);

And here.

> +  for (i = 0; i < 16; i++)
> +    {
> +      grub_printf("color %d: %08x\n", i, palette[i]);
> +    }

Braces.

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

Spaces.

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

The last 4 should also be a sizeof.

> +          /* Temporary workaround, fix font bitmap.  */
> +          for (y = 0; y < 16; y++)
> +            {
> +              for (x = 0; x < w; x++)
> +                {
> +                  glyph->bitmap[y * w + x] = bitmap[x * 16 + y];
> +                }
> +            }

Braces.

> +struct grub_video_mode_info {

Can you move the brace to the next line?

> +  /* Text buffer for virual screen. Contains (columns * rows) number
> +     of entries.  */

Typo.

> +  /* Intialize user requested mode.  */

Typo.

> +  /* Create virtual screen.  */
> +  if (grub_virtual_screen_setup (10,
> +                                 10,
> +                                 width,
> +                              height) != GRUB_ERR_NONE)

This fits on less lines, I think?

> +static void
> +redraw_screen_rect (unsigned int x, 
> +                    unsigned int y, 
> +                    unsigned int width, 
> +                    unsigned int height)

Doesn't this fit on two lines?

> +  grub_video_blit_render_target (text_layer,
> +                                 x,
> +                                 y,
> +                                 x - virtual_screen.offset_x,
> +                                 y - virtual_screen.offset_y,
> +                                 width,
> +                                 height);
> +}

Same here.

> +      if (x < dirty_region.top_left_x)
> +        {
> +          dirty_region.top_left_x = x;
> +        }
> +      if (y < dirty_region.top_left_y)
> +        {
> +          dirty_region.top_left_y = y;
> +        }
> +      if ((x + (int)width - 1) > dirty_region.bottom_right_x)
> +        {
> +          dirty_region.bottom_right_x = x + width - 1;
> +        }
> +      if ((y + (int)height - 1) > dirty_region.bottom_right_y)
> +        {
> +          dirty_region.bottom_right_y = y + height - 1;
> +        }

Please remove the braces.

> +static void
> +grub_videoterm_gotoxy (grub_uint8_t x, grub_uint8_t y)
> +{
> +  if (x >= virtual_screen.columns || y >= virtual_screen.rows)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_RANGE, "invalid point (%u,%u)",
> +               (unsigned) x, (unsigned) y);
> +      return;
> +    }


I wonder if it is useful to do error handing here.  First of all,
callers should be more careful.  And I think the user does not want to
be bothered with this.  I think it is better to set x and y to max and
continue as if nothing happened.

Or you might want to use grub_printf to show when something like this
happens.  Or perhaps using assertions would be nice here, but we do
not have assertions (yet).

> +/* General TODO's:
> +   - Implement mode optimized operations and automatically select best
> +
> +*/

Better move this to the wiki.

> +      /* Get alpha component.  */
> +      if (source->mode_info.reserved_mask_size > 0)
> +        {
> +          tmp = color >> source->mode_info.reserved_field_pos;
> +          tmp &= (1 << source->mode_info.reserved_mask_size) - 1;
> +          tmp <<= 8 - source->mode_info.reserved_mask_size;
> +          tmp |= (1 << (8 - source->mode_info.reserved_mask_size)) - 1;
> +        }
>        else
> -     {
> -       grub_vbe_set_pixel_rgb (x,
> -                                  y,
> -                                  0,
> -                                  0,
> -                                  0);
> -     }

Can this be done on a single line and without braces?

> +  /* Draw glyph.  */
> +  for (j = 0; j < height; j++)
> +    {
> +      for (i = 0; i < width; i++)
> +        {
> +          if ((glyph->bitmap[((i + x_offset) / 8) 
> +                             + (j + y_offset) * (charwidth / 8)] 
> +               & (1 << ((charwidth - (i + x_offset) - 1) % 8))))
> +            {
> +              grub_video_vbe_draw_pixel (x+i, y+j, color);
> +            }
> +        }
> +    }

Braces..

> +GRUB_MOD_INIT(video_i386_pc_vbe)
> +{
> +  grub_video_register (&grub_video_vbe_adapter);
> +}
> +
> +GRUB_MOD_FINI(video_i386_pc_ovbe)

Typo.

> +grub_err_t
> +grub_video_get_info (struct grub_video_mode_info *mode_info)
> +{
> +  if (! grub_video_adapter_active)
> +    {
> +      return grub_error (GRUB_ERR_BAD_DEVICE, "No video mode activated");
> +    }

Can you remove the brace?  Same for all other functions with this same
check.