If emulating text, return a foreground and background attribute from gfx_read_char(). Also, prefer returning a space character (instead of null) on blank cells. This may help with some users of "cbvga" seavgabios.
This also returns the foreground color (instead of always returning zero) for regular graphics mode gfx_read_char() calls. This seems fine as tests show other vgabios implementations also return various values here.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- vgasrc/vgafb.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/vgasrc/vgafb.c b/vgasrc/vgafb.c index 4bbbc16..b1d7aef 100644 --- a/vgasrc/vgafb.c +++ b/vgasrc/vgafb.c @@ -469,23 +469,35 @@ gfx_read_char(struct vgamode_s *vmode_g, struct cursorpos cp) op.op = GO_READ8; op.x = cp.x * 8; op.y = cp.y * cheight; + int car = 0; + u8 fgattr = 0x00, bgattr = 0x00; + if (vga_emulate_text()) { + // Read bottom right pixel of the cell to guess bg color + op.y += cheight-1; + handle_gfx_op(&op); + op.y -= cheight-1; + bgattr = op.pixels[7]; + fgattr = bgattr ^ 0x7; + car = 1; + } u8 i, j; for (i=0; i<cheight; i++, op.y++) { u8 line = 0; handle_gfx_op(&op); for (j=0; j<8; j++) - if (op.pixels[j]) + if (op.pixels[j] != bgattr) { line |= 0x80 >> j; + fgattr = op.pixels[j]; + } lines[i] = line; }
// Determine font - u16 car; - for (car=0; car<256; car++) { + for (; car<256; car++) { struct segoff_s font = get_font_data(car); if (memcmp_far(GET_SEG(SS), lines , font.seg, (void*)(font.offset+0), cheight) == 0) - return (struct carattr){car, 0, 0}; + return (struct carattr){car, fgattr | (bgattr << 4), 0}; } fail: return (struct carattr){0, 0, 0};
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
Paolo
On 06/01/2015 20:59, Paolo Bonzini wrote:
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
And a bunch of others (I checked the bottom line instead of just the bottom right). But the top right has no false positive for ASCII characters.
Paolo
On 06/01/2015 20:59, Paolo Bonzini wrote:
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
And a bunch of others (I checked the bottom line instead of just the bottom right). But the top right has no false positive for ASCII characters.
Paolo
On Tue, Jan 06, 2015 at 08:59:09PM +0100, Paolo Bonzini wrote:
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
Hrmm. How did you test? The script I wrote shows the bottom right as the least number of false positives. With the 8x16 font, there are six false positives, but all six are mirror images of other characters so there is no way to avoid them.
-Kevin
$ python ./scripts/countbits.py < vgasrc/vgafonts.c 256 49 83 107 129 113 82 51 16 58 101 110 98 100 103 61 19 78 146 139 132 146 132 72 29 92 136 122 140 154 122 63 17 108 159 142 139 154 147 88 44 81 135 124 116 134 132 59 14 66 123 132 152 134 101 66 13 22 31 42 57 44 30 28 11
256 6 8 21 41 23 21 19 8 14 24 40 57 41 32 26 7 44 78 102 125 110 102 65 13 48 93 92 92 93 110 76 12 51 90 92 99 95 99 81 15 80 146 129 132 138 141 89 26 85 133 109 134 133 140 95 18 99 151 121 134 138 149 114 39 86 129 88 111 120 130 92 13 80 116 84 105 113 134 90 10 47 109 126 148 122 136 83 10 12 16 25 39 30 31 26 6 9 16 28 47 30 27 22 9 6 7 20 33 20 20 20 6
256 6 8 21 41 23 21 19 8 15 24 40 57 41 33 27 7 41 72 95 118 100 89 59 20 43 88 90 85 82 93 75 20 51 83 79 92 81 91 81 21 81 140 126 134 138 134 94 31 79 130 98 122 131 131 89 25 110 153 119 140 146 142 120 52 82 130 91 107 111 120 92 24 85 127 87 103 112 125 95 17 87 117 71 100 105 130 96 18 49 115 134 154 122 136 90 15 9 13 23 40 28 31 23 8 8 13 25 38 29 30 25 8 8 11 23 41 25 22 19 8 6 7 20 33 20 20 20 6
==================================== countbits.py script
import sys
count = 0 counts = [] for line in sys.stdin: if 'vgafont' in line: if counts: print count for i in range(len(counts)): print "%3d" % counts[i], if i % 8 == 7: print print count = 0 counts = [] try: parts = [int(p.strip(), 0) for p in line.split(',') if p.strip()] except ValueError: continue if not parts: continue count += 1 out = [0] * (len(parts) * 8) for i in range(len(parts)): p = parts[i] for j in range(8): if (1<<(7-j)) & p: out[i*8 + j] += 1 if not counts: counts = out else: for i in range(len(out)): counts[i] += out[i]
On 06/01/2015 21:22, Kevin O'Connor wrote:
On Tue, Jan 06, 2015 at 08:59:09PM +0100, Paolo Bonzini wrote:
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
Hrmm. How did you test? The script I wrote shows the bottom right as the least number of false positives. With the 8x16 font, there are six false positives, but all six are mirror images of other characters so there is no way to avoid them.
I used the 8x8 font. I suspect that my new favorite testcase (gwbasic, what else) will mess up PRINT "_" if you use the bottom right.
Paolo
-Kevin
$ python ./scripts/countbits.py < vgasrc/vgafonts.c 256 49 83 107 129 113 82 51 16 58 101 110 98 100 103 61 19 78 146 139 132 146 132 72 29 92 136 122 140 154 122 63 17 108 159 142 139 154 147 88 44 81 135 124 116 134 132 59 14 66 123 132 152 134 101 66 13 22 31 42 57 44 30 28 11
256 6 8 21 41 23 21 19 8 14 24 40 57 41 32 26 7 44 78 102 125 110 102 65 13 48 93 92 92 93 110 76 12 51 90 92 99 95 99 81 15 80 146 129 132 138 141 89 26 85 133 109 134 133 140 95 18 99 151 121 134 138 149 114 39 86 129 88 111 120 130 92 13 80 116 84 105 113 134 90 10 47 109 126 148 122 136 83 10 12 16 25 39 30 31 26 6 9 16 28 47 30 27 22 9 6 7 20 33 20 20 20 6
256 6 8 21 41 23 21 19 8 15 24 40 57 41 33 27 7 41 72 95 118 100 89 59 20 43 88 90 85 82 93 75 20 51 83 79 92 81 91 81 21 81 140 126 134 138 134 94 31 79 130 98 122 131 131 89 25 110 153 119 140 146 142 120 52 82 130 91 107 111 120 92 24 85 127 87 103 112 125 95 17 87 117 71 100 105 130 96 18 49 115 134 154 122 136 90 15 9 13 23 40 28 31 23 8 8 13 25 38 29 30 25 8 8 11 23 41 25 22 19 8 6 7 20 33 20 20 20 6
==================================== countbits.py script
import sys
count = 0 counts = [] for line in sys.stdin: if 'vgafont' in line: if counts: print count for i in range(len(counts)): print "%3d" % counts[i], if i % 8 == 7: print print count = 0 counts = [] try: parts = [int(p.strip(), 0) for p in line.split(',') if p.strip()] except ValueError: continue if not parts: continue count += 1 out = [0] * (len(parts) * 8) for i in range(len(parts)): p = parts[i] for j in range(8): if (1<<(7-j)) & p: out[i*8 + j] += 1 if not counts: counts = out else: for i in range(len(out)): counts[i] += out[i]
On Wed, Jan 07, 2015 at 07:16:16AM +0100, Paolo Bonzini wrote:
On 06/01/2015 21:22, Kevin O'Connor wrote:
On Tue, Jan 06, 2015 at 08:59:09PM +0100, Paolo Bonzini wrote:
On 06/01/2015 16:49, Kevin O'Connor wrote:
- if (vga_emulate_text()) {
// Read bottom right pixel of the cell to guess bg colorop.y += cheight-1;handle_gfx_op(&op);op.y -= cheight-1;bgattr = op.pixels[7];fgattr = bgattr ^ 0x7;car = 1;- }
It's better to use the top right pixel. The bottom right has a false positive for characters 23 and 95.
Hrmm. How did you test? The script I wrote shows the bottom right as the least number of false positives. With the 8x16 font, there are six false positives, but all six are mirror images of other characters so there is no way to avoid them.
I used the 8x8 font. I suspect that my new favorite testcase (gwbasic, what else) will mess up PRINT "_" if you use the bottom right.
The patch only changes behavior when vga_emulate_text() is true though. That only happens for the "coreboot framebuffer" seavgabios, and it is only activated when the application thinks it's in text mode (mode 0x03).
Basically, cbvga seavgabios is used when coreboot knows how to initialize an onboard vga adapter, and the user doesn't have a real vgabios (or doesn't want to run it). The cbvga code emulates text mode character accesses by translating them to a graphical framebuffer (setup by coreboot). So, although the video adapter is technically in a graphical mode, we want the vgabios responses to look like text mode responses. The cbvga code only enables the 8x16 font, and it wouldn't really make sense for it to use an 8x8 font (given the size of modern lcd screens).
That said, it would be nice to try to avoid conflicts even with the 8x8 font - it's why I chose lower right instead of upper left or lower left. Unfortunately, upper right has additional conflicts for the 8x16 font.
So, unless I've missed something, I don't think it impacts gwbasic.
-Kevin
On 07/01/2015 07:43, Kevin O'Connor wrote:
So, unless I've missed something, I don't think it impacts gwbasic.
Yeah, I don't know how I missed "if (vga_emulate_text())". I guess I was confused by the reference to other video cards in the commit messages.
Paolo
On Wed, Jan 07, 2015 at 08:50:07AM +0100, Paolo Bonzini wrote:
On 07/01/2015 07:43, Kevin O'Connor wrote:
So, unless I've missed something, I don't think it impacts gwbasic.
Yeah, I don't know how I missed "if (vga_emulate_text())". I guess I was confused by the reference to other video cards in the commit messages.
The commit message could have been worded better. I've updated it to the below.
-Kevin
commit e638f97da08bd03c2d5be88325950d93d36911b1 Author: Kevin O'Connor kevin@koconnor.net Date: Tue Jan 6 09:36:41 2015 -0500
vgabios: Support emulated text in gfx_read_char()
When emulating text mode on "coreboot framebuffer" SeaVGABIOS, return a foreground and background attribute from gfx_read_char() and prefer returning a space character (instead of null) on blank cells.
This also returns the foreground color (instead of always returning zero) for regular graphics mode gfx_read_char() calls. This seems fine as tests show other vgabios implementations also return various values here.
Signed-off-by: Kevin O'Connor kevin@koconnor.net
diff --git a/vgasrc/vgafb.c b/vgasrc/vgafb.c index 4bbbc16..1a94fcf 100644 --- a/vgasrc/vgafb.c +++ b/vgasrc/vgafb.c @@ -367,7 +367,7 @@ gfx_move_chars(struct vgamode_s *vmode_g, struct cursorpos dest handle_gfx_op(&op); }
-// Clear are of screen in graphics mode. +// Clear area of screen in graphics mode. static void gfx_clear_chars(struct vgamode_s *vmode_g, struct cursorpos dest , struct carattr ca, struct cursorpos clearsize) @@ -469,23 +469,36 @@ gfx_read_char(struct vgamode_s *vmode_g, struct cursorpos cp) op.op = GO_READ8; op.x = cp.x * 8; op.y = cp.y * cheight; + int car = 0; + u8 fgattr = 0x00, bgattr = 0x00; + if (vga_emulate_text()) { + // Read bottom right pixel of the cell to guess bg color + op.y += cheight-1; + handle_gfx_op(&op); + op.y -= cheight-1; + bgattr = op.pixels[7]; + fgattr = bgattr ^ 0x7; + // Report space character for blank cells (skip null character check) + car = 1; + } u8 i, j; for (i=0; i<cheight; i++, op.y++) { u8 line = 0; handle_gfx_op(&op); for (j=0; j<8; j++) - if (op.pixels[j]) + if (op.pixels[j] != bgattr) { line |= 0x80 >> j; + fgattr = op.pixels[j]; + } lines[i] = line; }
// Determine font - u16 car; - for (car=0; car<256; car++) { + for (; car<256; car++) { struct segoff_s font = get_font_data(car); if (memcmp_far(GET_SEG(SS), lines , font.seg, (void*)(font.offset+0), cheight) == 0) - return (struct carattr){car, 0, 0}; + return (struct carattr){car, fgattr | (bgattr << 4), 0}; } fail: return (struct carattr){0, 0, 0}; @@ -594,7 +607,7 @@ vgafb_move_chars(struct cursorpos dest , movesize.x * 2, stride, movesize.y); }
-// Clear are of screen. +// Clear area of screen. void vgafb_clear_chars(struct cursorpos dest , struct carattr ca, struct cursorpos clearsize)
On Tue, Jan 06, 2015 at 10:49:27AM -0500, Kevin O'Connor wrote:
If emulating text, return a foreground and background attribute from gfx_read_char(). Also, prefer returning a space character (instead of null) on blank cells. This may help with some users of "cbvga" seavgabios.
This also returns the foreground color (instead of always returning zero) for regular graphics mode gfx_read_char() calls. This seems fine as tests show other vgabios implementations also return various values here.
Signed-off-by: Kevin O'Connor kevin@koconnor.net
FYI, I pushed this (slightly modified) patch to the main repo.
-Kevin
-
Kevin O'Connor -
Paolo Bonzini