Re: [SeaBIOS] [RFC PATCH 1/2] serial console, output

[Kevin O'Connor]

On Fri, Jul 01, 2016 at 12:54:30PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann

Thanks.  See my comments below.

[...]
> --- a/src/misc.c
> +++ b/src/misc.c
> @@ -11,6 +11,7 @@
>  #include "output.h" // debug_enter
>  #include "stacks.h" // call16_int
>  #include "string.h" // memset
> +#include "util.h" // serial_10
>
> #define PORT_MATH_CLEAR        0x00f0
>
> @@ -57,6 +58,7 @@ handle_10(struct bregs *regs)
>  {
>      debug_enter(regs, DEBUG_HDL_10);
>      // don't do anything, since the VGA BIOS handles int10h requests
> sercon_10(regs);
>
> }

Might as well remove handle_10 and call sercon_10 directly from
romlayout.S.

[...]
> --- a/src/serial.c
> +++ b/src/serial.c
> @@ -315,3 +315,343 @@ handle_17(struct bregs *regs)
>      default:   handle_17XX(regs); break;
>      }
>  }
>
> +/****************************************************************
> serial console output
> ****************************************************************/

I think this code should go in a new c file and not modify serial.c.

[...]
> +VARLOW u8 sercon_mode;
> +VARLOW u8 sercon_cols;
> +VARLOW u8 sercon_rows;
>
> +VARLOW u8 sercon_row;
> +VARLOW u8 sercon_col;

I think the code should use the BDA equivalents of the above instead
of declaring them in the private VARLOW space.  Some old programs may
rely on the equivalents in the BDA being updated.
> +/*
> We have a small output buffer here, for lazy output.  That allows
> to avoid a whole bunch of control sequences for pointless cursor
> moves, so when logging the output it'll be *alot* less cluttered.
> sercon_char/attr  is the actual output buffer.
> sercon_col_lazy   is the column of the terminal's cursor, typically
>               a few positions left of sercon_col.
> sercon_attr_last  is the most recent attribute sent to the terminal.
> */
>
> +VARLOW u8 sercon_attr_last;
> +VARLOW u8 sercon_col_lazy;
> +VARLOW u8 sercon_char[8];
> +VARLOW u8 sercon_attr[8];
>
> +VARLOW u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' };

For constant data (sercon_cmap) it would be preferable to use "static
VAR16" (see kbd.c:scan_to_keycode[] and mouse.c:sample_rates[] as
examples) and use GET_GLOBAL() instead of GET_LOW().

[...]
> +static void sercon_print_char(u8 chr, u8 attr)
> +{
> if (chr != 0x00) {
>    sercon_set_attr(attr);
>    sercon_putchar(chr);
> } else {
>    /* move cursor right */
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('C');
> }
>
> +}
>
> +static void sercon_flush_lazy(void)
> +{
> u8 count = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
> u8 pos = 0;
> if (!count && !GET_LOW(sercon_attr[0]))
>    return;
> while (count) {
>    sercon_print_char(GET_LOW(sercon_char[pos]),
>                      GET_LOW(sercon_attr[pos]));
>    count--;
>    pos++;
> }
> if (pos < ARRAY_SIZE(sercon_char) && GET_LOW(sercon_char[pos])) {
>    sercon_print_char(GET_LOW(sercon_char[pos]),
>                      GET_LOW(sercon_attr[pos]));
>    /* move cursor left */
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('D');
> }
> SET_LOW(sercon_col_lazy, GET_LOW(sercon_col));
> for (pos = 0; pos < ARRAY_SIZE(sercon_char); pos++) {
>    SET_LOW(sercon_attr[pos], 0x07);
>    SET_LOW(sercon_char[pos], 0x00);
> }
>
> +}

So, if I understand the above correctly, it's a buffer of recent
updates used to reduce serial bandwidth (and unclutter serial logs) in
the case where the application code issues a bunch of cursor moves /
cell updates that are mostly redundant.
> +/* Set video mode */
> +static void sercon_1000(struct bregs *regs)
> +{
> SET_LOW(sercon_mode, regs->al);
> switch (regs->al) {
> case 0x03:
> default:
>    SET_LOW(sercon_cols, 80);
>    SET_LOW(sercon_rows, 25);
>    regs->al = 0x30;
> }
> SET_LOW(sercon_row, 0);
> SET_LOW(sercon_col, 0);
> SET_LOW(sercon_col_lazy, 0);
> sercon_init();
>
> +}

FYI, the screen should only be cleared if the high bit of the mode is
not set.  I do wonder if more of the vgabios interface code from
vgasrc/vgabios.c could be reused here.
> +/* Set cursor position */
> +static void sercon_1002(struct bregs *regs)
> +{
> u8 row = regs->dh;
> u8 col = regs->dl;
> if (row == GET_LOW(sercon_row) &&
>    col >= GET_LOW(sercon_col_lazy) &&
>    col <  GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char)) {
>    SET_LOW(sercon_col, col);
>    if (col+1 == GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char))
>        sercon_flush_lazy();
> } else {
>    sercon_flush_lazy();
>    if (row == GET_LOW(sercon_row) && col == 0) {
>        sercon_putchar('\r');
>    } else {
>        sercon_cursor_goto(row, col);
>    }
>    SET_LOW(sercon_row, row);
>    SET_LOW(sercon_col, col);
>    SET_LOW(sercon_col_lazy, col);
> }
>
> +}
>
> +/* Get cursor position */
> +static void sercon_1003(struct bregs *regs)
> +{
> regs->ax = 0;
> regs->ch = 0;
> regs->cl = 7;
> regs->dh = GET_LOW(sercon_row);
> regs->dl = GET_LOW(sercon_col);
>
> +}
>
> +/* Scroll up window */
> +static void sercon_1006(struct bregs *regs)
> +{
> sercon_flush_lazy();
> sercon_putchar('\r');
> sercon_putchar('\n');
>
> +}
>
> +/* Read character and attribute at cursor position */
> +static void sercon_1008(struct bregs *regs)
> +{
> regs->ah = 0x07;
> regs->bh = ' ';
>
> +}

FYI, the sgabios code seems to indicate that sercon_1008() needs to be
implemented for some programs to work properly.  The sgabios code even
implements a cache of recent writes to try to get it to work.  It's
ugly.
> +/* Write character and attribute at cursor position */
> +static void sercon_1009(struct bregs *regs)
> +{
> u16 count = regs->cx;
> u8 pos;
> if (count == 1) {
>    pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
>    if (pos < ARRAY_SIZE(sercon_char)) {
>        sercon_char[pos] = regs->al;
>        sercon_attr[pos] = regs->bl;
>    }
> } else if (regs->al == 0x20 &&
>           GET_LOW(sercon_rows) * GET_LOW(sercon_cols) == count &&
>           GET_LOW(sercon_row) == 0 &&
>           GET_LOW(sercon_col) == 0) {
>    /* override everything with spaces -> this is clear screen */
>    sercon_flush_lazy();
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('2');
>    sercon_putchar('J');
> } else {
>    sercon_flush_lazy();
>    sercon_set_attr(regs->bl);
>    while (count) {
>        sercon_putchar(regs->al);
>        count--;
>    }
>    sercon_cursor_goto(GET_LOW(sercon_row),
>                       GET_LOW(sercon_col));
> }
>
> +}
>
> +/* Teletype output */
> +static void sercon_100e(struct bregs *regs)
> +{
> u8 pos, row, col;
> switch (regs->al) {
> case 7:
>    // beep
>    break;
> case 8:
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    col = GET_LOW(sercon_col);
>    if (col > 0) {
>        col--;
>        SET_LOW(sercon_col, col);
>        SET_LOW(sercon_col_lazy, col);
>    }
>    break;
> case '\r':
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    SET_LOW(sercon_col, 0);
>    SET_LOW(sercon_col_lazy, 0);
>    break;
> case '\n':
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    row = GET_LOW(sercon_row);
>    row++;
>    if (row >= GET_LOW(sercon_rows))
>        row = GET_LOW(sercon_rows)-1;
>    SET_LOW(sercon_row, row);
>    break;
> default:
>    pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
>    sercon_char[pos] = regs->al;
>    SET_LOW(sercon_col, GET_LOW(sercon_col) + 1);
>    if (pos+1 == ARRAY_SIZE(sercon_char))
>        sercon_flush_lazy();
>    break;
> }
>
> +}

I'm finding it hard to understand how the "uncluttering" works in the
above.  I think it would be easier to understand if the vgabios
interface code was separated from the "uncluttering" code.

In particular, I wonder if it would be simpler if the interface code
was more similar to vgasrc/vgabios.c and it just translated requests
to set_cursor_pos(cursorpos) and write_char(cursorpos, charattr) type
calls.  Then the write_char() code could check if the position was
near previously written characters and buffer it if so - flushing if
not.  Thus the "uncluttering" could be mostly done in write_char().
The set_cursor_pos() implementation could be very lazy - it only needs
to update the BDA with the new position.  The sercon_check_event()
code could send an explicit cursor move for the case where the cursor
is idling at some position not after the last written character.

[...]
> +void VISIBLE16
> +sercon_10(struct bregs *regs)
> +{
> if (!GET_LOW(sercon_port))
>    return;
> switch (regs->ah) {
> case 0x00: sercon_1000(regs); break;
> case 0x02: sercon_1002(regs); break;
> case 0x03: sercon_1003(regs); break;
> case 0x06: sercon_1006(regs); break;
> case 0x08: sercon_1008(regs); break;
> case 0x09: sercon_1009(regs); break;
> case 0x0e: sercon_100e(regs); break;
> case 0x0f: sercon_100f(regs); break;
> case 0x4f: sercon_104f(regs); break;
> default:
>    dprintf(1, "%s: ah 0x%02x, not implemented\n",
>            __func__, regs->ah);

There is a warn_unimplemented(regs) for this.  Also, would be nice to
implement a sercon_10XX(regs) to match other areas of the code.

-Kevin