Re: New patch for video subsystem...

[Vesa Jääskeläinen]

Marco Gerards wrote:
> 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 `.'.

Hope I catched all instances you were referring to. I think all comments
had those already.

> - 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).

Actually not all lines can be limited to 80 :|... But I will try to make
sure most of them fit nicely.

> - 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?

I'll try to follow this as good as I can :)

I will send new patch a bit later as a new email as this email gets
quite large :).

Thanks,
Vesa Jääskeläinen

>> Marco Gerards wrote:
>>> Vesa Jääskeläinen <address@hidden> writes:
>> 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.

Default was changed to 640x480 as per Okuji's request. I think we should
adapt that VGA console as a VGA driver that supports those "general"
cases when requested. Other memory models (like 4 bit planar modes) can
be supported later on if there is a real need.

Only problem there is that VBE does not list those VGA modes at all. In
example my card only lists modes that are better than 8 bit indexed
modes. Adding support for requesting 4 bit modes actually means to add
more flags to video setup interface functionality and then hard coding
those modes as a list.

OR we could perhaps add support to request specific mode number. But
there is currently no way to give that specific information from console
to driver. If we assume that this number is not anything, then it could
be added as set of bits in flags.

>>> 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. :-)

I do not think that fbterm informs user that it is using to video
subsystem. I think that videoterm informs user that it needs video driver.

As discussed on IRC, I have now renamed it to gfxterm.

>>>> 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.

One example could be that you want double buffered mode or something odd
that I cannot think at the moment. But if this information is taken out
of video_mode definition then there must be a video_flags.

As discussed on IRC, I have now modified it to following:
<width>x<height>[x<bits>]

If bits are omitted, then it would auto detect mode with best number of
colors for that resolution.

An example:
"1024x768x32" would initialize mode with resolution 1024x768 and only
accept 32 bit mode. If not found it will fail.

"1024x768x8" would initialize index color mode.

"1024x768" would initialize mode with best number of colors. If there is
a 32 bit mode, it would be selected. But if there is only index color
mode it would be selected.

>>>> +  * 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.

Ok, I will try to summarize it more precisely then.

>>>> -# 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?

That is correct. I think it would be more preferable to port that
terminal as video driver. But it could stay as impedendant terminal
driver too. I can try to learn how how VGA planes work if that is
required, but if someone else has knowledge how it works, it would
fasten this process a bit.

>>>> +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. ;-)

I will try to stay as far as I can from Emacs ;)... But let's drop this
religious subject here.

>>>> +  /* 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).

Changed as a DEFINE.

>>> 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.

In order to support indexed color images or indexed color values in
rendering code. There must be a support to have a list of RGB values for
every 256 color indices. Now this functionality is called as emulated
palette (as it is after all emulated, eg. not supported by video mode in
use).

Currently when you are creating a custom rendering target (a layer of
bitmap data), it only support creating of 32 bit rendering targets. So
hard coding this value to 256 will work correctly in this case as we
would use emulated palette anyway. If we initialize hardware indexed
color mode, even then we are going to store palette information in
emulated palette so we can freely use that information when emulating
indexed color operations for render targets in RGB modes.

You can even draw RGB render target to index color video screen as it
will try to find best match for colors. (though it might be quite ugly
in some cases)

> ---------------------------------------------------------------------
> 
> 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.

Ok.

>> 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?

Ok.

>> +  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.

Ok.

>> +  grub_video_set_viewport (x + 150, y + 150, width - 150*2, height - 150*2);
> 
> Spaces around the `*'.

Ok.

>> +  color = grub_video_map_rgb (77,33,77);
> 
> Spaces after the `,'.

Ok.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

>> -      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.

Ok.

>> +          /* 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.

Ok.

>> +struct grub_video_mode_info {
> 
> Can you move the brace to the next line?

Ok.

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

Fixed.

>> +  /* Intialize user requested mode.  */
> 
> Typo.

Fixed.

> 
>> +  /* Create virtual screen.  */
>> +  if (grub_virtual_screen_setup (10,
>> +                                 10,
>> +                                 width,
>> +                             height) != GRUB_ERR_NONE)
> 
> This fits on less lines, I think?

Yes, changed 10's to DEFAULT_BORDER_SIZE.

>> +static void
>> +redraw_screen_rect (unsigned int x, 
>> +                    unsigned int y, 
>> +                    unsigned int width, 
>> +                    unsigned int height)
> 
> Doesn't this fit on two lines?

Ok.

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

Ok. I made it as 4 lines.

>> +      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.

Ok.

>> +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).

Changed it to clamp to maximum size.

>> +/* General TODO's:
>> +   - Implement mode optimized operations and automatically select best
>> +
>> +*/
> 
> Better move this to the wiki.

Already have some local implementation for this, so removed the line.
Will discuss this after the commit.

>> +      /* 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?

You probably though something else here ;). Anyway there were some extra
braces at this location that I removed.

>> +  /* 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..

Fixed.

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

Fixed.

>> +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.

Done.