New patch for video subsystem...

[Marco Gerards]

Vesa Jääskeläinen <***@nic.fi> 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

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".
insmod font
font /boot/grub/unifont.pff
insmod terminal
insmod vbe
insmod videoterm
set video_mode=1024x768
terminal videoterm
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

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

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

[Vesa Jääskeläinen]

Hi,

Here is the newest modifications to video subsystem.

In short:
This patch adds support for video driver interface, example driver for
VBE, new video terminal (framebuffer terminal) using this video subsystem.

Longer description:
This patch implements most of the functionality described in grub's wiki
(http://grub.enbug.org/VideoSubsystem). There are still some issues left
to be implemented, but in my opinion it would be more important in this
point to get feedback about it and test how well it works on other
systems. Currently speed has not been target, only the functionality, so
some operations will take more time than would be nice. There is support
for index color and RGB modes. Video driver interface consist of video
module (video/video.c) that will forward all queries to active driver
and then interface definition (include/grub/video.h).

There is an example driver that implements VESA Bios Extension 2.0
support, only to framebuffer modes (video/i386/pc/vbe.c). It also
contains some support functionality for MultiBoot standard as it needs
some VBE fields.

There is implementaion for video terminal (term/videoterm.c) that has
basic support for rendering graphical terminal with two layers (actually
background layer is just a plain color, but other source could also be
used). Enhancing functionality the videoterm at this point is quite
easy, we just need to know what we want.

At this stage I think it would be Ok, to start to write drivers for
other architectures so we can have common video terminal.

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:

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"

It would tell driver to only look for RGB modes in that size.

Thanks,
Vesa JÀÀskelÀinen

[Johan Rydberg]

Post by Vesa Jääskeläinen
Here is the newest modifications to video subsystem.

Hi Vesa. What a suitable name :)

Sorry for the delay, and lack of earlier comments, but I've been out
of the GRUB loop for a while. But I have a few comments on the video
subsystem;

Correct me if I am wrong here, but what you call "render target" seems
to be what other video systems call "surface" or "drawable".

Why use the concept of a "active" render target? Why not instead let
all functions that operate on the active render target take a pointer
to a specific render target? Poking through your patch, it seems that
there a lot of the following (forgive my pseudo-code)

grub_video_set_active_render_target (target);
// .. fill it with something ..
grub_video_fill_rect (color, 0, 0, width, height);

grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY);
grub_video_blit_render_target (target, 0, 0, 0, 0, width, height);

I would feel more comfortable with the following workflow:

grub_video_fill_rect (target, color, 0, 0, width, height);
grub_video_blit_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY,
target, 0, 0, 0, 0, width, height);

Also, I think it is important the user can get hold of a pointer to
the render targets data, and an exact pixel format, to do private
rendering. It would be hard to make a perfect gradient using fill_rect.

I'm not sure the concept of "viewports" are needed at all; instead let
the 'application' (e.g, the terminal) render into a render target, and
blit that to the screen at the desired position. To minimize memory,
the videport-render target can be a sub-render target of the main
render target (ie, they share the same buffer) unless the user wants
any fancy stuff like a background picture.

Thanks,
Johan

[Vesa Jääskeläinen]

Post by Johan Rydberg

Post by Vesa Jääskeläinen
Here is the newest modifications to video subsystem.

Hi Vesa. What a suitable name :)

:)

Post by Johan Rydberg
Sorry for the delay, and lack of earlier comments, but I've been out
of the GRUB loop for a while. But I have a few comments on the video
subsystem;
Correct me if I am wrong here, but what you call "render target" seems
to be what other video systems call "surface" or "drawable".

Yes they are called by different names and they have a bit different
semantics depending on a place they are used.

Post by Johan Rydberg
Why use the concept of a "active" render target? Why not instead let
all functions that operate on the active render target take a pointer
to a specific render target? Poking through your patch, it seems that
there a lot of the following (forgive my pseudo-code)
grub_video_set_active_render_target (target);
// .. fill it with something ..
grub_video_fill_rect (color, 0, 0, width, height);
grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY);
grub_video_blit_render_target (target, 0, 0, 0, 0, width, height);
grub_video_fill_rect (target, color, 0, 0, width, height);
grub_video_blit_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY,
target, 0, 0, 0, 0, width, height);

Actually that patch is already on CVS :]... so you might find it easier
to read from there.

But the idea here was to make it easier to make function that operates
on different render targets without knowing about it. eg:

set_target a
call common_func
set target b
call common_func
set target c

With your proposal this would require to pass this render target pointer
all around and would make video function calls longer.

There is also another problem in your proposal. What happens if video
driver get's changed and you still have pointers to render targets (or
to raw memory)?

Post by Johan Rydberg
Also, I think it is important the user can get hold of a pointer to
the render targets data, and an exact pixel format, to do private
rendering. It would be hard to make a perfect gradient using fill_rect.

That might not be possible on all architectures. That's the reason there
is a bitmap support (which I am working on currently). Bitmaps are
located on host memory so they can be modified.

Render targets can be located on host memory or on video memory
depending on arch. If they are located on video memory then there is
only need to have video-video blit functionality and some archs might
even provide hardware accelerated helper functions.

One idea of the render targets are that they hide the real bitmap data
format. So one could make a 24 bit RGB picture and then just blit it to
render target and underlying video driver would convert it to correct
format. This frees API user from thinking all possible formats and can
concentrate to make one working implementation.

If one needs to modify render target on all platforms there should be
support to direct access to memory or ability to read contents of frame
buffer to host memory and then write it back or requirement to store
data on host memory and then do blit from there to video memory, but
that is a bit costy on performance.

Post by Johan Rydberg
I'm not sure the concept of "viewports" are needed at all; instead let
the 'application' (e.g, the terminal) render into a render target, and
blit that to the screen at the desired position. To minimize memory,
the videport-render target can be a sub-render target of the main
render target (ie, they share the same buffer) unless the user wants
any fancy stuff like a background picture.

Dropping support for viewports would indeed make driver code a bit
simplier. It were originally designed to problems like this:

Let's assume you have window on your screen. Then you have scroll bars
there so the actual window contents are bigger than what is shown on
screen. Now you just set viewport on target render target and it's easy
to render new contents and you don't need to worry that it would be
rendered out of bounds of target rectangle. And as coordinates are now
related to viewport you don't need to take account that.

But now that I think, one could just give screen coordinates and size
and then use offset to scroll within that area.

Now that I have really used the api for gfxterm and working on bitmap
support I have noticed that it might be better to handle those
operations on one render target and then blit the changed data to
visible render target.

I am not fond to your sub-render-target idea. You can always use
coordinates and offsets to play with it. And the background image or
color would most likely to be present.

Post by Johan Rydberg
Thanks,
Johan

Thanks for your comments,
Vesa Jääskeläinen

[Johan Rydberg]

Post by Vesa Jääskeläinen
But the idea here was to make it easier to make function that operates
set_target a
call common_func
set target b
call common_func
set target c
With your proposal this would require to pass this render target pointer
all around and would make video function calls longer.

I do not see a problem with that. The overhead for passing of an
extra argument is probably less than the overhead of the function
calls (esp since set_active_render_target is an indirect call)

Post by Vesa Jääskeläinen
There is also another problem in your proposal. What happens if video
driver get's changed and you still have pointers to render targets (or
to raw memory)?

Possibly the same thing as with the current method; I do not see how
this would make things different?

Post by Vesa Jääskeläinen

Post by Johan Rydberg
Also, I think it is important the user can get hold of a pointer to
the render targets data, and an exact pixel format, to do private
rendering. It would be hard to make a perfect gradient using fill_rect.

That might not be possible on all architectures. That's the reason there
is a bitmap support (which I am working on currently). Bitmaps are
located on host memory so they can be modified.

I'm not sure "bitmap" is a good word here; maybe "image" would be better.

Post by Vesa Jääskeläinen
Render targets can be located on host memory or on video memory
depending on arch. If they are located on video memory then there is
only need to have video-video blit functionality and some archs might
even provide hardware accelerated helper functions.

If the architecture does not support direct access to the video
memory, it just falls back on returning a render target with
malloc()ed memory.

Post by Vesa Jääskeläinen
One idea of the render targets are that they hide the real bitmap data
format. So one could make a 24 bit RGB picture and then just blit it to
render target and underlying video driver would convert it to correct
format. This frees API user from thinking all possible formats and can
concentrate to make one working implementation.

Iter-format blits can of course also be supported if you have direct
access to the render target pixel data.

Post by Vesa Jääskeläinen
If one needs to modify render target on all platforms there should be
support to direct access to memory or ability to read contents of frame
buffer to host memory and then write it back or requirement to store
data on host memory and then do blit from there to video memory, but
that is a bit costy on performance.

I do not see why that should be needed.

Have you looked at for example DirectFB's API? Even through their
coding style is fucked up, the overall design of the API is quite
nice.

I'm just afraid that we will see the exact same thing that happened to
legacy GRUB; a lot of distribution vendors hack up something of their
own. Therefor it is important that we get this right.

~j

[Vesa Jääskeläinen]

Post by Marco Gerards
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.

Post by Marco Gerards
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.

Thanks,
Vesa Jääskeläinen

Post by Marco Gerards

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".
insmod font
font /boot/grub/unifont.pff
insmod terminal
insmod vbe
insmod videoterm
set video_mode=1024x768
terminal videoterm
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

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.

Post by Marco Gerards
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.

Post by Marco Gerards

width=<value>
height=<value>
index
rgb
<width>x<height>x<depth>
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.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

+ (grub_vbe_get_controller_info): Renamed to
+ grub_vbe_bios_get_controller_info.

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

Ok. I left out additional description as it would have made huge entries.

Post by Marco Gerards

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

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

Ok.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

`New file.' again.

Ok.

Post by Marco Gerards

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

Post by Marco Gerards

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

Post by Marco Gerards

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?

Sure.

Post by Marco Gerards

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

And this one.

Ok.

Post by Marco Gerards

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

Oh, yes... Fixed.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Changed to sizeof (grub_uint32_t).

Post by Marco Gerards

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

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

Sure. I must have forgotten this one.

Post by Marco Gerards

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

Yep ;)

Post by Marco Gerards

+typedef grub_uint32_t grub_color_t;

I think grub_video_color_t is better.

It is a long one, but changed.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

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

Ok.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Typo.
Can you change grub_uint32_t to ints inside this struct?

Fixed. Ok.

Post by Marco Gerards

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

Typo: seure->sure

Fixed.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Typo: Cleare->Create (right?)

Fixed.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Post by Marco Gerards

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

I think the opposite, but changed.

Post by Marco Gerards

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.

Oh. Fixed.

Post by Marco Gerards

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

Post by Marco Gerards

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

Typo.

Fixed.

Post by Marco Gerards

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

Please add a space after `grub_abs'.

Ok.

Post by Marco Gerards

+ // TODO: PROOFREAD THIS!.

Ehm? :-)

Whups :).

Post by Marco Gerards

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

Post by Marco Gerards

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

Please use sizeof here.

I made different change :).

Post by Marco Gerards

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

In most cases I would agree, but this one is something that should not
be modified. But if that is a real problem, then of course I will change it.

Post by Marco Gerards
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.

Post by Marco Gerards

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

Ok.

Post by Marco Gerards

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

Moved as new module.

[Yoshinori K. Okuji]

Post by Vesa Jääskeläinen
Nowadays 1024x768 is a normal standard resolution.

Even if it is very popular, since the boot loader is a critical part of an
operation system, the default should be set to something really safe. So I
prefer 640x480. It is up to the user (or the vendor shipping GRUB) to change
it to something more comfortable.

Okuji

[Vesa Jääskeläinen]

Post by Yoshinori K. Okuji

Post by Vesa Jääskeläinen
Nowadays 1024x768 is a normal standard resolution.

Even if it is very popular, since the boot loader is a critical part of an
operation system, the default should be set to something really safe. So I
prefer 640x480. It is up to the user (or the vendor shipping GRUB) to change
it to something more comfortable.

Ok. I will change the default to that. I will only make new patch if
there are issues to be solved before commit.

[Vesa Jääskeläinen]

Post by Vesa Jääskeläinen

Post by Yoshinori K. Okuji

Post by Vesa Jääskeläinen
Nowadays 1024x768 is a normal standard resolution.

Even if it is very popular, since the boot loader is a critical part of an
operation system, the default should be set to something really safe. So I
prefer 640x480. It is up to the user (or the vendor shipping GRUB) to change
it to something more comfortable.

Ok. I will change the default to that. I will only make new patch if
there are issues to be solved before commit.

While waiting for more comments or approval I have made some changes,
but I will commit them after we have initial set of patches in.

1. Added simple caching to font manager. Implemented as a "pageing", in
initial page there is 128 entries to hold glyphs, if it there is need
for more glyphs new page will be allocated to and added as a next page.
When finding for cached glyph, it first scans root page and then
advances to next pages.

2. Added simple buffering to videoterm. If there is enough memory for
buffer layer it will allocate it and use it. When rendering, all
operations in videoterm goes first to buffer layer, and when all layers
has been rendered to it, then it renders buffer to display. If it is not
available, we will notice some flickering.

I think the next step here would be to either add bitmap loading support
(and implement rendering for it) or add more optimal rendering
possibilities to VBE driver. Is there a nice way to split vbe driver to
more source files without causing generation of more modules. I was
thinking that "optimized" versions of some blitting operations would be
good to place on some other file as source might grow to quite large.

I am open to proposals what to implement next (it could even be
something not listed here).

Also we need to decide should there be functional changes when user has
chosen indexed color modes instead of RGB mode. (Both work now, but
indexed color mode more slowly as it tries to find best indexes for RGB
values for EVERY drawn pixel :)

Thanks,
Vesa Jääskeläinen

[Vesa Jääskeläinen]

Post by Vesa Jääskeläinen
Is there a nice way to split vbe driver to
more source files without causing generation of more modules. I was
thinking that "optimized" versions of some blitting operations would be
good to place on some other file as source might grow to quite large.

I should have sleeped before asking this question :) conf/*.rmk and just
adding source file to vbe.mod defs....

[Marco Gerards]

Vesa Jääskeläinen <***@nic.fi> 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

Post by Vesa Jääskeläinen

Post by Marco Gerards
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.

:-)

Post by Vesa Jääskeläinen

Post by Marco Gerards
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.

Post by Vesa Jääskeläinen

Post by Marco Gerards

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".
insmod font
font /boot/grub/unifont.pff
insmod terminal
insmod vbe
insmod videoterm
set video_mode=1024x768
terminal videoterm
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

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.

Post by Vesa Jääskeläinen

Post by Marco Gerards
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. :-)

Post by Vesa Jääskeläinen

Post by Marco Gerards

width=<value>
height=<value>
index
rgb
<width>x<height>x<depth>
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.

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Vesa Jääskeläinen

Post by Marco Gerards
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.


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

Post by Vesa Jääskeläinen
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 @@
+
+ * 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.

Post by Vesa Jääskeläinen
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?

Post by Vesa Jääskeläinen
+ 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.

Post by Vesa Jääskeläinen
+ grub_video_set_viewport (x + 150, y + 150, width - 150*2, height - 150*2);

Spaces around the `*'.

Post by Vesa Jääskeläinen
+ color = grub_video_map_rgb (77,33,77);

Spaces after the `,'.

Post by Vesa Jääskeläinen
+ color = grub_video_map_rgb (255,255,255);

Likewise.

Post by Vesa Jääskeläinen
+ grub_video_blit_glyph (&glyph, color, 16*2, 16);

And here.

Post by Vesa Jääskeläinen
+ for (i = 0; i < 16; i++)
+ {
+ grub_printf("color %d: %08x\n", i, palette[i]);
+ }

Braces.

Post by Vesa Jääskeläinen
+ glyph->height = 16;
+ glyph->baseline = (16*3)/4;

Spaces.

Post by Vesa Jääskeläinen
- 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.

Post by Vesa Jääskeläinen
+ /* 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.

Post by Vesa Jääskeläinen
+struct grub_video_mode_info {

Can you move the brace to the next line?

Post by Vesa Jääskeläinen
+ /* Text buffer for virual screen. Contains (columns * rows) number
+ of entries. */

Typo.

Post by Vesa Jääskeläinen
+ /* Intialize user requested mode. */

Typo.

Post by Vesa Jääskeläinen
+ /* Create virtual screen. */
+ if (grub_virtual_screen_setup (10,
+ 10,
+ width,
+ height) != GRUB_ERR_NONE)

This fits on less lines, I think?

Post by Vesa Jääskeläinen
+static void
+redraw_screen_rect (unsigned int x,
+ unsigned int y,
+ unsigned int width,
+ unsigned int height)

Doesn't this fit on two lines?

Post by Vesa Jääskeläinen
+ grub_video_blit_render_target (text_layer,
+ x,
+ y,
+ x - virtual_screen.offset_x,
+ y - virtual_screen.offset_y,
+ width,
+ height);
+}

Same here.

Post by Vesa Jääskeläinen
+ 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.

Post by Vesa Jääskeläinen
+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).

Post by Vesa Jääskeläinen
+ - Implement mode optimized operations and automatically select best
+
+*/

Better move this to the wiki.

Post by Vesa Jääskeläinen
+ /* 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?

Post by Vesa Jääskeläinen
+ /* 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..

Post by Vesa Jääskeläinen
+GRUB_MOD_INIT(video_i386_pc_vbe)
+{
+ grub_video_register (&grub_video_vbe_adapter);
+}
+
+GRUB_MOD_FINI(video_i386_pc_ovbe)

Typo.

Post by Vesa Jääskeläinen
+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.

[Vesa Jääskeläinen]

Post by Marco Gerards
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
- 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.

Post by Marco Gerards
- 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.

Post by Marco Gerards
- 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

Post by Marco Gerards

Post by Vesa Jääskeläinen
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.

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.

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards
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.

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards

width=<value>
height=<value>
index
rgb
<width>x<height>x<depth>
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.

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards

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

Post by Marco Gerards

Post by Vesa Jääskeläinen

Post by Marco Gerards
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)

Post by Marco Gerards
---------------------------------------------------------------------

Post by Vesa Jääskeläinen
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 @@
+
+ * 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ grub_video_set_viewport (x + 150, y + 150, width - 150*2, height - 150*2);

Spaces around the `*'.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ color = grub_video_map_rgb (77,33,77);

Spaces after the `,'.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ color = grub_video_map_rgb (255,255,255);

Likewise.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ grub_video_blit_glyph (&glyph, color, 16*2, 16);

And here.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ for (i = 0; i < 16; i++)
+ {
+ grub_printf("color %d: %08x\n", i, palette[i]);
+ }

Braces.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ glyph->height = 16;
+ glyph->baseline = (16*3)/4;

Spaces.

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
- 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+struct grub_video_mode_info {

Can you move the brace to the next line?

Ok.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* Text buffer for virual screen. Contains (columns * rows) number
+ of entries. */

Typo.

Fixed.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* Intialize user requested mode. */

Typo.

Fixed.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ - 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+ /* 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.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+GRUB_MOD_INIT(video_i386_pc_vbe)
+{
+ grub_video_register (&grub_video_vbe_adapter);
+}
+
+GRUB_MOD_FINI(video_i386_pc_ovbe)

Typo.

Fixed.

Post by Marco Gerards

Post by Vesa Jääskeläinen
+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.

[Vesa Jääskeläinen]

Hi,

Here is hopefully the last patch modification :)

Thanks,
Vesa Jääskeläinen

[Marco Gerards]

Post by Vesa Jääskeläinen

Post by Marco Gerards
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.

One more thing... Can you check if the endianess when loading fonts is
converted to the machine endianess? I had some serious doubts that
this was done correctly. This should work, otherwise your code will
seriously break on some archs...

--
Marco