[SeaBIOS] [RFC PATCH 1/2] serial console, output
Gerd Hoffmann
kraxel at redhat.com
Mon Jul 4 10:16:35 CEST 2016
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
More information about the SeaBIOS
mailing list