From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44782) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJz3Z-0002TV-Tt for qemu-devel@nongnu.org; Mon, 04 Jul 2016 04:16:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bJz3W-0004Ly-My for qemu-devel@nongnu.org; Mon, 04 Jul 2016 04:16:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48690) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJz3W-0004Ln-FQ for qemu-devel@nongnu.org; Mon, 04 Jul 2016 04:16:38 -0400 Message-ID: <1467620195.15123.133.camel@redhat.com> From: Gerd Hoffmann Date: Mon, 04 Jul 2016 10:16:35 +0200 In-Reply-To: <20160701154732.GA11200@morn.lan> References: <1467370471-20554-1-git-send-email-kraxel@redhat.com> <1467370471-20554-2-git-send-email-kraxel@redhat.com> <20160701154732.GA11200@morn.lan> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 Subject: Re: [Qemu-devel] [SeaBIOS] [RFC PATCH 1/2] serial console, output List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: seabios@seabios.org, qemu-devel@nongnu.org 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); > > } >=20 > 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 > > + ****************************************************************/ >=20 > 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; >=20 > 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] =3D { '0', '4', '2', '6', '1', '5', '3', '7' = }; >=20 > 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 =3D 0; pos < ARRAY_SIZE(sercon_char); pos++) { > > + SET_LOW(sercon_attr[pos], 0x07); > > + SET_LOW(sercon_char[pos], 0x00); > > + } > > +} >=20 > 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 =3D 0x07; > > + regs->bh =3D ' '; > > +} >=20 > 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. >=20 > 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); >=20 > 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