Re: [SeaBIOS] [PATCH 2/2] vgabios: implement read char in graphics mode

[Kevin O'Connor]

On Thu, Jan 01, 2015 at 09:27:47PM +0100, Paolo Bonzini wrote:
> GWBasic relies on this, so implement it to enable some serious retrocomputing.

Heh.

[...]
> I couldn't find documentation for what to return as the attribute, but
> experimenting with DOSBox suggests 0 (and GWBasic accepts it).

Just for reference, for the "cbvga" text mode emulation, I grab the
background attribute by assuming the lower right pixel of the cell is
the background, and then assume the foreground attribute is the
background attribute xor'd with 0x7 (see gfx_write_char()).  I'm not
sure if that hack applies here though.  :-)

One thing I'm curious about is what should happen when the cell
contains a space character (all pixels off) - your patch returns 0
right now, where I'd think a space (32) would be more probable.  Maybe
callers already know how to handle this though.  I might run a quick
test under coreboot with a proprietary vgabios to see what it does.  :-)

[...]
> +// Read a character to the screen in graphics mode.
> +static int
> +gfx_read_char(struct vgamode_s *vmode_g
>            , struct cursorpos cp)
>
> +{
> if (cp.x >= GET_BDA(video_cols))
>    return 0;
> struct gfx_op op;
> init_gfx_op(&op, vmode_g);
> op.op = GO_READ8;
> op.x = cp.x * 8;
> op.y = -1;
> int cheight = GET_BDA(char_height);
> u16 basey = cp.y * cheight;
> u16 car;
> u8 i, j;
> for (car = 0; car < 256; car++) {
>    struct segoff_s font = get_font_data(car);
>    for (i = 0; i < cheight; i++) {
>        if (op.y != basey + i) {
>            op.y = basey + i;
>            handle_gfx_op(&op);
>        }
>        u8 fontline = GET_FARVAR(font.seg, *(u8*)(font.offset+i));
>        for (j = 0; j < 8; j++, fontline <<= 1)
>            if (!!op.pixels[j] != !!(fontline & 0x80))
>                goto trynext;
>    }
>    return car;
>
> +trynext:;
> }
> return 0;
>
> +}
>
> // Write a character to the screen in graphics mode.
>  static void
>  gfx_write_char(struct vgamode_s *vmode_g
> @@ -561,9 +596,9 @@ vgafb_read_char(struct cursorpos cp)
>          goto fail;
>  if (GET_GLOBAL(vmode_g->memmodel) != MM_TEXT) {
>    // FIXME gfx mode
>    dprintf(1, "Read char in graphics mode\n");
>    goto fail;
>    u8 v = gfx_read_char(vmode_g, cp);
>    struct carattr ca = {v, 0, 0};
>    return ca;

Any reason not to just "return gfx_read_char(vmode_g, cp);" here and
have gfx_read_char() return a struct carattr?

Patch looks good to me - thanks.
-Kevin