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

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

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

Well, I expect this will change when adding support for parallel output
to both vga and serial console.

> > +/****************************************************************
> > + * serial console output
> > + ****************************************************************/
> 
> I think this code should go in a new c file and not modify serial.c.

Agree.  Was thinking about that already as I saw the code grow ;)

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

Figured that meanwhile.  syslinux is one example.

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

OK.

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

Yep.  Typically happens for colored output.  Apps use int10/09 to set
character and attribute at the cursor position (but without moving the
cursor).  Then they move the cursor either using int10/02 (explicit set
cursor position) or by printing the same character again using int10/0e
(teletype, which prints character without updating attribute and moves
the cursor forward).

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

Didn't run into any issues yet, but also tested linux bootloaders only.

Maybe we can reuse the output buffer which we have anyway.  Logic needs
reworked a bit.  We can't just clear characters after printing them out
if we want be able to read them later, so we need a separate
pending-updates bit.  Also should be bigger I guess, maybe 80 chars so
it can cover a complete line.

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

I'll have a look.

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

Ok.

cheers,
  Gerd