[PATCH 0/8] vga: improve emulation fidelity

[PATCH 0/8] vga: improve emulation fidelity

[Paolo Bonzini]

This implements horizontal pel panning, which is used by games such as
the Commander Keen series, and also reimplements word and odd/even modes
so that they work in graphics modes; this mostly fixes Jazz Jackrabbit's
graphics.

There are still some issues with Cirrus VGA, and also Keen expects the
display parameters to be latched in ways that I don't fully understand
and that seem to differ between Keen 1 (EGA) and 4 (VGA).  So there is
still a bit of tearing, but I have been sitting on these patches since
the Christmas holidays of 2014, so let's flush this first part.

Paolo

Paolo Bonzini (8):
  vga: remove unused macros
  vga: introduce VGADisplayParams
  vga: mask addresses in non-VESA modes to 256k
  vga: implement horizontal pel panning in graphics modes
  vga: optimize horizontal pel panning in 256-color modes
  vga: reindent memory access code
  vga: use latches in odd/even mode too
  vga: sort-of implement word and double-word access modes

 hw/display/cirrus_vga.c  |  28 +--
 hw/display/vga-helpers.h | 121 +++++++++---
 hw/display/vga.c         | 418 +++++++++++++++++++++------------------
 hw/display/vga_int.h     |  18 +-
 hw/display/vga_regs.h    |   4 +
 5 files changed, 347 insertions(+), 242 deletions(-)

-- 
2.43.0

[PATCH 1/8] vga: remove unused macros

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 37557c3442a..18d966ecd3e 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -103,12 +103,6 @@ const uint8_t gr_mask[16] = {
 #define PAT(x) (x)
 #endif
 
-#if HOST_BIG_ENDIAN
-#define BIG 1
-#else
-#define BIG 0
-#endif
-
 #if HOST_BIG_ENDIAN
 #define GET_PLANE(data, p) (((data) >> (24 - (p) * 8)) & 0xff)
 #else
@@ -134,14 +128,6 @@ static const uint32_t mask16[16] = {
     PAT(0xffffffff),
 };
 
-#undef PAT
-
-#if HOST_BIG_ENDIAN
-#define PAT(x) (x)
-#else
-#define PAT(x) cbswap_32(x)
-#endif
-
 static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
-- 
2.43.0

Re: [PATCH 1/8] vga: remove unused macros

[BALATON Zoltan]

On Sun, 31 Dec 2023, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/display/vga.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 37557c3442a..18d966ecd3e 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -103,12 +103,6 @@ const uint8_t gr_mask[16] = {
> #define PAT(x) (x)
> #endif

While at it you could also move this definiton dows to where it's used so 
it's clear where it's needed.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> -#if HOST_BIG_ENDIAN
> -#define BIG 1
> -#else
> -#define BIG 0
> -#endif
> -
> #if HOST_BIG_ENDIAN
> #define GET_PLANE(data, p) (((data) >> (24 - (p) * 8)) & 0xff)
> #else
> @@ -134,14 +128,6 @@ static const uint32_t mask16[16] = {
>     PAT(0xffffffff),
> };
>
> -#undef PAT
> -
> -#if HOST_BIG_ENDIAN
> -#define PAT(x) (x)
> -#else
> -#define PAT(x) cbswap_32(x)
> -#endif
> -
> static uint32_t expand4[256];
> static uint16_t expand2[256];
> static uint8_t expand4to8[16];
>

Re: [PATCH 1/8] vga: remove unused macros

[Paolo Bonzini]

On Sun, Dec 31, 2023 at 5:01 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sun, 31 Dec 2023, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > hw/display/vga.c | 14 --------------
> > 1 file changed, 14 deletions(-)
> >
> > diff --git a/hw/display/vga.c b/hw/display/vga.c
> > index 37557c3442a..18d966ecd3e 100644
> > --- a/hw/display/vga.c
> > +++ b/hw/display/vga.c
> > @@ -103,12 +103,6 @@ const uint8_t gr_mask[16] = {
> > #define PAT(x) (x)
> > #endif
>
> While at it you could also move this definiton dows to where it's used so
> it's clear where it's needed.

In fact PAT() can be replaced with const_le32(), and GET_PLANE() can
use a byte swap, which is just as fast as tweaking the shift count these
days[1]:

#define GET_PLANE(data, p) ((cpu_to_le32(data) >> ((p) * 8)) & 0xff)

and it can even be inlined

Paolo

[1] see following instructions, only sparc is missing:

arm/aarch64 - rev
i386 - bswapl
loongarch - revb.2w
mips - wsbh + rotr
ppc - brw
riscv - rev8 ( + srli on riscv64)
s390 - lrvgr

[PATCH 2/8] vga: introduce VGADisplayParams

[Paolo Bonzini]

The next patches will introduce more parameters that cause a full
refresh.  Instead of adding arguments to get_offsets and lines to
update_basic_params, do everything through a struct.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cirrus_vga.c | 24 +++++-------
 hw/display/vga.c        | 82 +++++++++++++++++------------------------
 hw/display/vga_int.h    | 15 ++++----
 3 files changed, 52 insertions(+), 69 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index b80f98b6c4c..e6d2581d4eb 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -798,9 +798,9 @@ static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
     if (blit_is_unsafe(s, false))
         return 0;
 
-    return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
-            s->cirrus_blt_srcaddr - s->vga.start_addr,
-            s->cirrus_blt_width, s->cirrus_blt_height);
+    return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.params.start_addr,
+                          s->cirrus_blt_srcaddr - s->vga.params.start_addr,
+                          s->cirrus_blt_width, s->cirrus_blt_height);
 }
 
 /***************************************
@@ -1101,30 +1101,26 @@ static void cirrus_write_bitblt(CirrusVGAState * s, unsigned reg_value)
  *
  ***************************************/
 
-static void cirrus_get_offsets(VGACommonState *s1,
-                               uint32_t *pline_offset,
-                               uint32_t *pstart_addr,
-                               uint32_t *pline_compare)
+static void cirrus_get_params(VGACommonState *s1,
+                              VGADisplayParams *params)
 {
     CirrusVGAState * s = container_of(s1, CirrusVGAState, vga);
-    uint32_t start_addr, line_offset, line_compare;
+    uint32_t line_offset;
 
     line_offset = s->vga.cr[0x13]
         | ((s->vga.cr[0x1b] & 0x10) << 4);
     line_offset <<= 3;
-    *pline_offset = line_offset;
+    params->line_offset = line_offset;
 
-    start_addr = (s->vga.cr[0x0c] << 8)
+    params->start_addr = (s->vga.cr[0x0c] << 8)
         | s->vga.cr[0x0d]
         | ((s->vga.cr[0x1b] & 0x01) << 16)
         | ((s->vga.cr[0x1b] & 0x0c) << 15)
         | ((s->vga.cr[0x1d] & 0x80) << 12);
-    *pstart_addr = start_addr;
 
-    line_compare = s->vga.cr[0x18] |
+    params->line_compare = s->vga.cr[0x18] |
         ((s->vga.cr[0x07] & 0x10) << 4) |
         ((s->vga.cr[0x09] & 0x40) << 3);
-    *pline_compare = line_compare;
 }
 
 static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)
@@ -2925,7 +2921,7 @@ void cirrus_init_common(CirrusVGAState *s, Object *owner,
     s->linear_mmio_mask = s->real_vram_size - 256;
 
     s->vga.get_bpp = cirrus_get_bpp;
-    s->vga.get_offsets = cirrus_get_offsets;
+    s->vga.get_params = cirrus_get_params;
     s->vga.get_resolution = cirrus_get_resolution;
     s->vga.cursor_invalidate = cirrus_cursor_invalidate;
     s->vga.cursor_draw_line = cirrus_cursor_draw_line;
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 18d966ecd3e..3bd357d72c2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1059,52 +1059,40 @@ static int update_palette256(VGACommonState *s)
     return full_update;
 }
 
-static void vga_get_offsets(VGACommonState *s,
-                            uint32_t *pline_offset,
-                            uint32_t *pstart_addr,
-                            uint32_t *pline_compare)
+static void vga_get_params(VGACommonState *s,
+                           VGADisplayParams *params)
 {
-    uint32_t start_addr, line_offset, line_compare;
-
     if (vbe_enabled(s)) {
-        line_offset = s->vbe_line_offset;
-        start_addr = s->vbe_start_addr;
-        line_compare = 65535;
+        params->line_offset = s->vbe_line_offset;
+        params->start_addr = s->vbe_start_addr;
+        params->line_compare = 65535;
     } else {
         /* compute line_offset in bytes */
-        line_offset = s->cr[VGA_CRTC_OFFSET];
-        line_offset <<= 3;
+        params->line_offset = s->cr[VGA_CRTC_OFFSET] << 3;
 
         /* starting address */
-        start_addr = s->cr[VGA_CRTC_START_LO] |
+        params->start_addr = s->cr[VGA_CRTC_START_LO] |
             (s->cr[VGA_CRTC_START_HI] << 8);
 
         /* line compare */
-        line_compare = s->cr[VGA_CRTC_LINE_COMPARE] |
+        params->line_compare = s->cr[VGA_CRTC_LINE_COMPARE] |
             ((s->cr[VGA_CRTC_OVERFLOW] & 0x10) << 4) |
             ((s->cr[VGA_CRTC_MAX_SCAN] & 0x40) << 3);
     }
-    *pline_offset = line_offset;
-    *pstart_addr = start_addr;
-    *pline_compare = line_compare;
 }
 
 /* update start_addr and line_offset. Return TRUE if modified */
 static int update_basic_params(VGACommonState *s)
 {
     int full_update;
-    uint32_t start_addr, line_offset, line_compare;
+    VGADisplayParams current;
 
     full_update = 0;
 
-    s->get_offsets(s, &line_offset, &start_addr, &line_compare);
+    s->get_params(s, &current);
 
-    if (line_offset != s->line_offset ||
-        start_addr != s->start_addr ||
-        line_compare != s->line_compare) {
-        s->line_offset = line_offset;
-        s->start_addr = start_addr;
-        s->line_compare = line_compare;
+    if (memcmp(&current, &s->params, sizeof(current))) {
+        s->params = current;
         full_update = 1;
     }
     return full_update;
@@ -1205,7 +1193,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
     }
     full_update |= update_basic_params(s);
 
-    line_offset = s->line_offset;
+    line_offset = s->params.line_offset;
 
     vga_get_text_resolution(s, &width, &height, &cw, &cheight);
     if ((height * width) <= 1) {
@@ -1244,7 +1232,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
     }
 
     cursor_offset = ((s->cr[VGA_CRTC_CURSOR_HI] << 8) |
-                     s->cr[VGA_CRTC_CURSOR_LO]) - s->start_addr;
+                     s->cr[VGA_CRTC_CURSOR_LO]) - s->params.start_addr;
     if (cursor_offset != s->cursor_offset ||
         s->cr[VGA_CRTC_CURSOR_START] != s->cursor_start ||
         s->cr[VGA_CRTC_CURSOR_END] != s->cursor_end) {
@@ -1258,7 +1246,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
         s->cursor_start = s->cr[VGA_CRTC_CURSOR_START];
         s->cursor_end = s->cr[VGA_CRTC_CURSOR_END];
     }
-    cursor_ptr = s->vram_ptr + (s->start_addr + cursor_offset) * 4;
+    cursor_ptr = s->vram_ptr + (s->params.start_addr + cursor_offset) * 4;
     if (now >= s->cursor_blink_time) {
         s->cursor_blink_time = now + VGA_TEXT_CURSOR_PERIOD_MS / 2;
         s->cursor_visible_phase = !s->cursor_visible_phase;
@@ -1268,7 +1256,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
     linesize = surface_stride(surface);
     ch_attr_ptr = s->last_ch_attr;
     line = 0;
-    offset = s->start_addr * 4;
+    offset = s->params.start_addr * 4;
     for(cy = 0; cy < height; cy++) {
         d1 = dest;
         src = s->vram_ptr + offset;
@@ -1348,7 +1336,7 @@ static void vga_draw_text(VGACommonState *s, int full_update)
         dest += linesize * cheight;
         line1 = line + cheight;
         offset += line_offset;
-        if (line < s->line_compare && line1 >= s->line_compare) {
+        if (line < s->params.line_compare && line1 >= s->params.line_compare) {
             offset = 0;
         }
         line = line1;
@@ -1478,10 +1466,10 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     disp_width = width;
     depth = s->get_bpp(s);
 
-    region_start = (s->start_addr * 4);
-    region_end = region_start + (ram_addr_t)s->line_offset * height;
+    region_start = (s->params.start_addr * 4);
+    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
     region_end += width * depth / 8; /* scanline length */
-    region_end -= s->line_offset;
+    region_end -= s->params.line_offset;
     if (region_end > s->vbe_size || depth == 0 || depth == 15) {
         /*
          * We land here on:
@@ -1546,7 +1534,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         share_surface = false;
     }
 
-    if (s->line_offset != s->last_line_offset ||
+    if (s->params.line_offset != s->last_line_offset ||
         disp_width != s->last_width ||
         height != s->last_height ||
         s->last_depth != depth ||
@@ -1557,12 +1545,12 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         s->last_scr_height = height;
         s->last_width = disp_width;
         s->last_height = height;
-        s->last_line_offset = s->line_offset;
+        s->last_line_offset = s->params.line_offset;
         s->last_depth = depth;
         s->last_byteswap = byteswap;
         full_update = 1;
     }
-    if (surface_data(surface) != s->vram_ptr + (s->start_addr * 4)
+    if (surface_data(surface) != s->vram_ptr + (s->params.start_addr * 4)
         && is_buffer_shared(surface)) {
         /* base address changed (page flip) -> shared display surfaces
          * must be updated with the new base address */
@@ -1572,8 +1560,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     if (full_update) {
         if (share_surface) {
             surface = qemu_create_displaysurface_from(disp_width,
-                    height, format, s->line_offset,
-                    s->vram_ptr + (s->start_addr * 4));
+                    height, format, s->params.line_offset,
+                    s->vram_ptr + (s->params.start_addr * 4));
             dpy_gfx_replace_surface(s->con, surface);
         } else {
             qemu_console_resize(s->con, disp_width, height);
@@ -1637,9 +1625,9 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
 #if 0
     printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n",
            width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
-           s->line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
+           s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
 #endif
-    addr1 = (s->start_addr * 4);
+    addr1 = (s->params.start_addr * 4);
     bwidth = DIV_ROUND_UP(width * bits, 8);
     y_start = -1;
     d = surface_data(surface);
@@ -1647,7 +1635,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     y1 = 0;
 
     if (!full_update) {
-        if (s->line_compare < height) {
+        if (s->params.line_compare < height) {
             /* split screen mode */
             region_start = 0;
         }
@@ -1703,14 +1691,14 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         if (!multi_run) {
             mask = (s->cr[VGA_CRTC_MODE] & 3) ^ 3;
             if ((y1 & mask) == mask)
-                addr1 += s->line_offset;
+                addr1 += s->params.line_offset;
             y1++;
             multi_run = multi_scan;
         } else {
             multi_run--;
         }
         /* line compare acts on the displayed lines */
-        if (y == s->line_compare)
+        if (y == s->params.line_compare)
             addr1 = 0;
         d += linesize;
     }
@@ -1827,9 +1815,7 @@ void vga_common_reset(VGACommonState *s)
     s->graphic_mode = -1; /* force full update */
     s->shift_control = 0;
     s->double_scan = 0;
-    s->line_offset = 0;
-    s->line_compare = 0;
-    s->start_addr = 0;
+    memset(&s->params, '\0', sizeof(s->params));
     s->plane_updated = 0;
     s->last_cw = 0;
     s->last_ch = 0;
@@ -1951,7 +1937,7 @@ static void vga_update_text(void *opaque, console_ch_t *chardata)
 
         /* Update "hardware" cursor */
         cursor_offset = ((s->cr[VGA_CRTC_CURSOR_HI] << 8) |
-                         s->cr[VGA_CRTC_CURSOR_LO]) - s->start_addr;
+                         s->cr[VGA_CRTC_CURSOR_LO]) - s->params.start_addr;
         if (cursor_offset != s->cursor_offset ||
             s->cr[VGA_CRTC_CURSOR_START] != s->cursor_start ||
             s->cr[VGA_CRTC_CURSOR_END] != s->cursor_end || full_update) {
@@ -1967,7 +1953,7 @@ static void vga_update_text(void *opaque, console_ch_t *chardata)
             s->cursor_end = s->cr[VGA_CRTC_CURSOR_END];
         }
 
-        src = (uint32_t *) s->vram_ptr + s->start_addr;
+        src = (uint32_t *) s->vram_ptr + s->params.start_addr;
         dst = chardata;
 
         if (full_update) {
@@ -2212,7 +2198,7 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
     xen_register_framebuffer(&s->vram);
     s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
     s->get_bpp = vga_get_bpp;
-    s->get_offsets = vga_get_offsets;
+    s->get_params = vga_get_params;
     s->get_resolution = vga_get_resolution;
     s->hw_ops = &vga_ops;
     switch (vga_retrace_method) {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 7cf0d11201a..6be61e04284 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -56,6 +56,12 @@ struct VGACommonState;
 typedef uint8_t (* vga_retrace_fn)(struct VGACommonState *s);
 typedef void (* vga_update_retrace_info_fn)(struct VGACommonState *s);
 
+typedef struct VGADisplayParams {
+    uint32_t line_offset;
+    uint32_t start_addr;
+    uint32_t line_compare;
+} VGADisplayParams;
+
 typedef struct VGACommonState {
     MemoryRegion *legacy_address_space;
     uint8_t *vram_ptr;
@@ -90,10 +96,7 @@ typedef struct VGACommonState {
     uint8_t palette[768];
     int32_t bank_offset;
     int (*get_bpp)(struct VGACommonState *s);
-    void (*get_offsets)(struct VGACommonState *s,
-                        uint32_t *pline_offset,
-                        uint32_t *pstart_addr,
-                        uint32_t *pline_compare);
+    void (*get_params)(struct VGACommonState *s, VGADisplayParams *params);
     void (*get_resolution)(struct VGACommonState *s,
                         int *pwidth,
                         int *pheight);
@@ -111,9 +114,7 @@ typedef struct VGACommonState {
     int graphic_mode;
     uint8_t shift_control;
     uint8_t double_scan;
-    uint32_t line_offset;
-    uint32_t line_compare;
-    uint32_t start_addr;
+    VGADisplayParams params;
     uint32_t plane_updated;
     uint32_t last_line_offset;
     uint8_t last_cw, last_ch;
-- 
2.43.0

Re: [PATCH 2/8] vga: introduce VGADisplayParams

[Philippe Mathieu-Daudé]

On 31/12/23 10:39, Paolo Bonzini wrote:
> The next patches will introduce more parameters that cause a full
> refresh.  Instead of adding arguments to get_offsets and lines to
> update_basic_params, do everything through a struct.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/display/cirrus_vga.c | 24 +++++-------
>   hw/display/vga.c        | 82 +++++++++++++++++------------------------
>   hw/display/vga_int.h    | 15 ++++----
>   3 files changed, 52 insertions(+), 69 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

(Please enable scripts/git.orderfile)

[PATCH 3/8] vga: mask addresses in non-VESA modes to 256k

[Paolo Bonzini]

This allows setting the start address to a high value, and reading the
bottom of the screen from the beginning of VRAM.  Commander Keen 4
("Goodbye, Galaxy!") relies on this behavior.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga-helpers.h | 9 +++++----
 hw/display/vga.c         | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
index 10e9cfd40a0..83b9a15604a 100644
--- a/hw/display/vga-helpers.h
+++ b/hw/display/vga-helpers.h
@@ -108,7 +108,7 @@ static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
     width >>= 3;
     for(x = 0; x < width; x++) {
-        data = vga_read_dword_le(vga, addr);
+        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
         data &= plane_mask;
         v = expand2[GET_PLANE(data, 0)];
         v |= expand2[GET_PLANE(data, 2)] << 2;
@@ -144,7 +144,7 @@ static void vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
     width >>= 3;
     for(x = 0; x < width; x++) {
-        data = vga_read_dword_le(vga, addr);
+        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
         data &= plane_mask;
         v = expand2[GET_PLANE(data, 0)];
         v |= expand2[GET_PLANE(data, 2)] << 2;
@@ -177,7 +177,7 @@ static void vga_draw_line4(VGACommonState *vga, uint8_t *d,
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
     width >>= 3;
     for(x = 0; x < width; x++) {
-        data = vga_read_dword_le(vga, addr);
+        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
         data &= plane_mask;
         v = expand4[GET_PLANE(data, 0)];
         v |= expand4[GET_PLANE(data, 1)] << 1;
@@ -209,7 +209,7 @@ static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
     width >>= 3;
     for(x = 0; x < width; x++) {
-        data = vga_read_dword_le(vga, addr);
+        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
         data &= plane_mask;
         v = expand4[GET_PLANE(data, 0)];
         v |= expand4[GET_PLANE(data, 1)] << 1;
@@ -242,6 +242,7 @@ static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
     palette = vga->last_palette;
     width >>= 3;
     for(x = 0; x < width; x++) {
+        addr &= VGA_VRAM_SIZE - 1;
         PUT_PIXEL2(d, 0, palette[vga_read_byte(vga, addr + 0)]);
         PUT_PIXEL2(d, 1, palette[vga_read_byte(vga, addr + 1)]);
         PUT_PIXEL2(d, 2, palette[vga_read_byte(vga, addr + 2)]);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 3bd357d72c2..b1660bdde67 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -47,6 +47,9 @@ bool have_vga = true;
 /* 16 state changes per vertical frame @60 Hz */
 #define VGA_TEXT_CURSOR_PERIOD_MS       (1000 * 2 * 16 / 60)
 
+/* Address mask for non-VESA modes.  */
+#define VGA_VRAM_SIZE                   262144
+
 /*
  * Video Graphics Array (VGA)
  *
-- 
2.43.0

Re: [PATCH 3/8] vga: mask addresses in non-VESA modes to 256k

[BALATON Zoltan]

On Sun, 31 Dec 2023, Paolo Bonzini wrote:
> This allows setting the start address to a high value, and reading the
> bottom of the screen from the beginning of VRAM.  Commander Keen 4
> ("Goodbye, Galaxy!") relies on this behavior.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/display/vga-helpers.h | 9 +++++----
> hw/display/vga.c         | 3 +++
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
> index 10e9cfd40a0..83b9a15604a 100644
> --- a/hw/display/vga-helpers.h
> +++ b/hw/display/vga-helpers.h
> @@ -108,7 +108,7 @@ static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
>     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
>     width >>= 3;
>     for(x = 0; x < width; x++) {
> -        data = vga_read_dword_le(vga, addr);
> +        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
>         data &= plane_mask;
>         v = expand2[GET_PLANE(data, 0)];
>         v |= expand2[GET_PLANE(data, 2)] << 2;
> @@ -144,7 +144,7 @@ static void vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
>     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
>     width >>= 3;
>     for(x = 0; x < width; x++) {
> -        data = vga_read_dword_le(vga, addr);
> +        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
>         data &= plane_mask;
>         v = expand2[GET_PLANE(data, 0)];
>         v |= expand2[GET_PLANE(data, 2)] << 2;
> @@ -177,7 +177,7 @@ static void vga_draw_line4(VGACommonState *vga, uint8_t *d,
>     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
>     width >>= 3;
>     for(x = 0; x < width; x++) {
> -        data = vga_read_dword_le(vga, addr);
> +        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
>         data &= plane_mask;
>         v = expand4[GET_PLANE(data, 0)];
>         v |= expand4[GET_PLANE(data, 1)] << 1;
> @@ -209,7 +209,7 @@ static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
>     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
>     width >>= 3;
>     for(x = 0; x < width; x++) {
> -        data = vga_read_dword_le(vga, addr);
> +        data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
>         data &= plane_mask;
>         v = expand4[GET_PLANE(data, 0)];
>         v |= expand4[GET_PLANE(data, 1)] << 1;
> @@ -242,6 +242,7 @@ static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
>     palette = vga->last_palette;
>     width >>= 3;
>     for(x = 0; x < width; x++) {
> +        addr &= VGA_VRAM_SIZE - 1;
>         PUT_PIXEL2(d, 0, palette[vga_read_byte(vga, addr + 0)]);
>         PUT_PIXEL2(d, 1, palette[vga_read_byte(vga, addr + 1)]);
>         PUT_PIXEL2(d, 2, palette[vga_read_byte(vga, addr + 2)]);
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 3bd357d72c2..b1660bdde67 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -47,6 +47,9 @@ bool have_vga = true;
> /* 16 state changes per vertical frame @60 Hz */
> #define VGA_TEXT_CURSOR_PERIOD_MS       (1000 * 2 * 16 / 60)
>
> +/* Address mask for non-VESA modes.  */
> +#define VGA_VRAM_SIZE                   262144

(256 * KiB) maybe?

> +
> /*
>  * Video Graphics Array (VGA)
>  *
>

[PATCH 4/8] vga: implement horizontal pel panning in graphics modes

[Paolo Bonzini]

This implements smooth scrolling, as used for example by Commander Keen
and Second Reality.

Unfortunately, this is not enough to avoid tearing in Commander Keen,
because sometimes the wrong start address is used for a frame.
On real EGA, the panning register is sampled on every line, while
the display start is latched for the next frame at the start of the
vertical retrace.  On real VGA, the panning register is also latched,
but at the end of the vertical retrace.  It looks like Keen exploits
this by only waiting for horizontal retrace when setting the display
start, but implementing it breaks the 256-color Keen games...

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cirrus_vga.c  |   4 ++
 hw/display/vga-helpers.h | 100 ++++++++++++++++++++++++++++-----------
 hw/display/vga.c         |  36 ++++++++++++--
 hw/display/vga_int.h     |   3 ++
 4 files changed, 111 insertions(+), 32 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index e6d2581d4eb..849eff43898 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -43,6 +43,7 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "ui/pixel_ops.h"
+#include "vga_regs.h"
 #include "cirrus_vga_internal.h"
 #include "qom/object.h"
 #include "ui/console.h"
@@ -1121,6 +1122,9 @@ static void cirrus_get_params(VGACommonState *s1,
     params->line_compare = s->vga.cr[0x18] |
         ((s->vga.cr[0x07] & 0x10) << 4) |
         ((s->vga.cr[0x09] & 0x40) << 3);
+
+    params->hpel = s->vga.ar[VGA_ATC_PEL];
+    params->hpel_split = s->vga.ar[VGA_ATC_MODE] & 0x20;
 }
 
 static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)
diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
index 83b9a15604a..29933562c45 100644
--- a/hw/display/vga-helpers.h
+++ b/hw/display/vga-helpers.h
@@ -98,14 +98,19 @@ static void vga_draw_glyph9(uint8_t *d, int linesize,
 /*
  * 4 color mode
  */
-static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
-                           uint32_t addr, int width)
+static void *vga_draw_line2(VGACommonState *vga, uint8_t *d,
+                            uint32_t addr, int width, int hpel)
 {
     uint32_t plane_mask, *palette, data, v;
     int x;
 
     palette = vga->last_palette;
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+    hpel &= 7;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
@@ -126,6 +131,7 @@ static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
         d += 32;
         addr += 4;
     }
+    return hpel ? vga->panning_buf + 4 * hpel : NULL;
 }
 
 #define PUT_PIXEL2(d, n, v) \
@@ -134,14 +140,19 @@ static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
 /*
  * 4 color mode, dup2 horizontal
  */
-static void vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
-                             uint32_t addr, int width)
+static void *vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
+                              uint32_t addr, int width, int hpel)
 {
     uint32_t plane_mask, *palette, data, v;
     int x;
 
     palette = vga->last_palette;
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+    hpel &= 7;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
@@ -162,19 +173,25 @@ static void vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
         d += 64;
         addr += 4;
     }
+    return hpel ? vga->panning_buf + 8 * hpel : NULL;
 }
 
 /*
  * 16 color mode
  */
-static void vga_draw_line4(VGACommonState *vga, uint8_t *d,
-                           uint32_t addr, int width)
+static void *vga_draw_line4(VGACommonState *vga, uint8_t *d,
+                            uint32_t addr, int width, int hpel)
 {
     uint32_t plane_mask, data, v, *palette;
     int x;
 
     palette = vga->last_palette;
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+    hpel &= 7;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
@@ -194,19 +211,25 @@ static void vga_draw_line4(VGACommonState *vga, uint8_t *d,
         d += 32;
         addr += 4;
     }
+    return hpel ? vga->panning_buf + 4 * hpel : NULL;
 }
 
 /*
  * 16 color mode, dup2 horizontal
  */
-static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
-                             uint32_t addr, int width)
+static void *vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
+                              uint32_t addr, int width, int hpel)
 {
     uint32_t plane_mask, data, v, *palette;
     int x;
 
     palette = vga->last_palette;
     plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+    hpel &= 7;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         data = vga_read_dword_le(vga, addr & (VGA_VRAM_SIZE - 1));
@@ -226,6 +249,7 @@ static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
         d += 64;
         addr += 4;
     }
+    return hpel ? vga->panning_buf + 8 * hpel : NULL;
 }
 
 /*
@@ -233,13 +257,18 @@ static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
  *
  * XXX: add plane_mask support (never used in standard VGA modes)
  */
-static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
-                             uint32_t addr, int width)
+static void *vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
+                              uint32_t addr, int width, int hpel)
 {
     uint32_t *palette;
     int x;
 
     palette = vga->last_palette;
+    hpel = (hpel >> 1) & 3;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         addr &= VGA_VRAM_SIZE - 1;
@@ -250,6 +279,7 @@ static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
         d += 32;
         addr += 4;
     }
+    return hpel ? vga->panning_buf + 8 * hpel : NULL;
 }
 
 /*
@@ -257,13 +287,18 @@ static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
  *
  * XXX: add plane_mask support (never used in standard VGA modes)
  */
-static void vga_draw_line8(VGACommonState *vga, uint8_t *d,
-                           uint32_t addr, int width)
+static void *vga_draw_line8(VGACommonState *vga, uint8_t *d,
+                            uint32_t addr, int width, int hpel)
 {
     uint32_t *palette;
     int x;
 
     palette = vga->last_palette;
+    hpel = (hpel >> 1) & 3;
+    if (hpel) {
+        width += 8;
+        d = vga->panning_buf;
+    }
     width >>= 3;
     for(x = 0; x < width; x++) {
         ((uint32_t *)d)[0] = palette[vga_read_byte(vga, addr + 0)];
@@ -277,13 +312,14 @@ static void vga_draw_line8(VGACommonState *vga, uint8_t *d,
         d += 32;
         addr += 8;
     }
+    return hpel ? vga->panning_buf + 4 * hpel : NULL;
 }
 
 /*
  * 15 bit color
  */
-static void vga_draw_line15_le(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line15_le(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t v, r, g, b;
@@ -298,10 +334,11 @@ static void vga_draw_line15_le(VGACommonState *vga, uint8_t *d,
         addr += 2;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
-static void vga_draw_line15_be(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line15_be(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t v, r, g, b;
@@ -316,13 +353,14 @@ static void vga_draw_line15_be(VGACommonState *vga, uint8_t *d,
         addr += 2;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
 /*
  * 16 bit color
  */
-static void vga_draw_line16_le(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line16_le(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t v, r, g, b;
@@ -337,10 +375,11 @@ static void vga_draw_line16_le(VGACommonState *vga, uint8_t *d,
         addr += 2;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
-static void vga_draw_line16_be(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line16_be(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t v, r, g, b;
@@ -355,13 +394,14 @@ static void vga_draw_line16_be(VGACommonState *vga, uint8_t *d,
         addr += 2;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
 /*
  * 24 bit color
  */
-static void vga_draw_line24_le(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line24_le(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t r, g, b;
@@ -375,10 +415,11 @@ static void vga_draw_line24_le(VGACommonState *vga, uint8_t *d,
         addr += 3;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
-static void vga_draw_line24_be(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line24_be(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t r, g, b;
@@ -392,13 +433,14 @@ static void vga_draw_line24_be(VGACommonState *vga, uint8_t *d,
         addr += 3;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
 /*
  * 32 bit color
  */
-static void vga_draw_line32_le(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line32_le(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t r, g, b;
@@ -412,10 +454,11 @@ static void vga_draw_line32_le(VGACommonState *vga, uint8_t *d,
         addr += 4;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
 
-static void vga_draw_line32_be(VGACommonState *vga, uint8_t *d,
-                               uint32_t addr, int width)
+static void *vga_draw_line32_be(VGACommonState *vga, uint8_t *d,
+                                uint32_t addr, int width, int hpel)
 {
     int w;
     uint32_t r, g, b;
@@ -429,4 +472,5 @@ static void vga_draw_line32_be(VGACommonState *vga, uint8_t *d,
         addr += 4;
         d += 4;
     } while (--w != 0);
+    return NULL;
 }
diff --git a/hw/display/vga.c b/hw/display/vga.c
index b1660bdde67..2467f3f6c65 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -50,6 +50,13 @@ bool have_vga = true;
 /* Address mask for non-VESA modes.  */
 #define VGA_VRAM_SIZE                   262144
 
+/* This value corresponds to a shift of zero pixels
+ * in 9-dot text mode.  In other modes, bit 3 is undefined;
+ * we just ignore it, so that 8 corresponds to zero pixels
+ * in all modes.
+ */
+#define VGA_HPEL_NEUTRAL		8
+
 /*
  * Video Graphics Array (VGA)
  *
@@ -1001,8 +1008,8 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
     }
 }
 
-typedef void vga_draw_line_func(VGACommonState *s1, uint8_t *d,
-                                uint32_t srcaddr, int width);
+typedef void *vga_draw_line_func(VGACommonState *s1, uint8_t *d,
+                                 uint32_t srcaddr, int width, int hpel);
 
 #include "vga-access.h"
 #include "vga-helpers.h"
@@ -1069,6 +1076,8 @@ static void vga_get_params(VGACommonState *s,
         params->line_offset = s->vbe_line_offset;
         params->start_addr = s->vbe_start_addr;
         params->line_compare = 65535;
+        params->hpel = VGA_HPEL_NEUTRAL;
+        params->hpel_split = false;
     } else {
         /* compute line_offset in bytes */
         params->line_offset = s->cr[VGA_CRTC_OFFSET] << 3;
@@ -1081,6 +1090,9 @@ static void vga_get_params(VGACommonState *s,
         params->line_compare = s->cr[VGA_CRTC_LINE_COMPARE] |
             ((s->cr[VGA_CRTC_OVERFLOW] & 0x10) << 4) |
             ((s->cr[VGA_CRTC_MAX_SCAN] & 0x40) << 3);
+
+        params->hpel = s->ar[VGA_ATC_PEL];
+        params->hpel_split = s->ar[VGA_ATC_MODE] & 0x20;
     }
 }
 
@@ -1452,6 +1464,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     ram_addr_t page0, page1, region_start, region_end;
     DirtyBitmapSnapshot *snap = NULL;
     int disp_width, multi_scan, multi_run;
+    int hpel;
     uint8_t *d;
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line = NULL;
@@ -1551,6 +1564,9 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         s->last_line_offset = s->params.line_offset;
         s->last_depth = depth;
         s->last_byteswap = byteswap;
+        /* 16 extra pixels are needed for double-width planar modes.  */
+        s->panning_buf = g_realloc(s->panning_buf,
+                                   (disp_width + 16) * sizeof(uint32_t));
         full_update = 1;
     }
     if (surface_data(surface) != s->vram_ptr + (s->params.start_addr * 4)
@@ -1630,8 +1646,12 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
            width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
            s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
 #endif
+    hpel = bits <= 8 ? s->params.hpel : 0;
     addr1 = (s->params.start_addr * 4);
     bwidth = DIV_ROUND_UP(width * bits, 8);
+    if (hpel) {
+        bwidth += 4;
+    }
     y_start = -1;
     d = surface_data(surface);
     linesize = surface_stride(surface);
@@ -1679,7 +1699,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             if (y_start < 0)
                 y_start = y;
             if (!(is_buffer_shared(surface))) {
-                vga_draw_line(s, d, addr, width);
+                uint8_t *p;
+                p = vga_draw_line(s, d, addr, width, hpel);
+                if (p) {
+                    memcpy(d, p, disp_width * sizeof(uint32_t));
+                }
                 if (s->cursor_draw_line)
                     s->cursor_draw_line(s, d, y);
             }
@@ -1701,8 +1725,12 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             multi_run--;
         }
         /* line compare acts on the displayed lines */
-        if (y == s->params.line_compare)
+        if (y == s->params.line_compare) {
+            if (s->params.hpel_split) {
+                hpel = VGA_HPEL_NEUTRAL;
+            }
             addr1 = 0;
+        }
         d += linesize;
     }
     if (y_start >= 0) {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 6be61e04284..876a1d3697b 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -60,6 +60,8 @@ typedef struct VGADisplayParams {
     uint32_t line_offset;
     uint32_t start_addr;
     uint32_t line_compare;
+    uint8_t  hpel;
+    bool     hpel_split;
 } VGADisplayParams;
 
 typedef struct VGACommonState {
@@ -111,6 +113,7 @@ typedef struct VGACommonState {
     /* display refresh support */
     QemuConsole *con;
     uint32_t font_offsets[2];
+    uint8_t *panning_buf;
     int graphic_mode;
     uint8_t shift_control;
     uint8_t double_scan;
-- 
2.43.0

[PATCH 5/8] vga: optimize horizontal pel panning in 256-color modes

[Paolo Bonzini]

Do not go through the panning buffer unless the address wraps in the middle
of the line.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga-helpers.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
index 29933562c45..60ddb27d946 100644
--- a/hw/display/vga-helpers.h
+++ b/hw/display/vga-helpers.h
@@ -265,6 +265,18 @@ static void *vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
 
     palette = vga->last_palette;
     hpel = (hpel >> 1) & 3;
+
+    /* For 256 color modes, we can adjust the source address and write directly
+     * to the destination, even if horizontal pel panning is active.  However,
+     * the loop below assumes that the address does not wrap in the middle of a
+     * plane.  If that happens...
+     */
+    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
+        addr += hpel * 4;
+        hpel = 0;
+    }
+
+    /* ... use the panning buffer as in planar modes.  */
     if (hpel) {
         width += 8;
         d = vga->panning_buf;
-- 
2.43.0

Re: [PATCH 5/8] vga: optimize horizontal pel panning in 256-color modes

[BALATON Zoltan]

On Sun, 31 Dec 2023, Paolo Bonzini wrote:
> Do not go through the panning buffer unless the address wraps in the middle
> of the line.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/display/vga-helpers.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
> index 29933562c45..60ddb27d946 100644
> --- a/hw/display/vga-helpers.h
> +++ b/hw/display/vga-helpers.h
> @@ -265,6 +265,18 @@ static void *vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
>
>     palette = vga->last_palette;
>     hpel = (hpel >> 1) & 3;
> +
> +    /* For 256 color modes, we can adjust the source address and write directly
> +     * to the destination, even if horizontal pel panning is active.  However,
> +     * the loop below assumes that the address does not wrap in the middle of a
> +     * plane.  If that happens...
> +     */
> +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
> +        addr += hpel * 4;
> +        hpel = 0;
> +    }
> +
> +    /* ... use the panning buffer as in planar modes.  */
>     if (hpel) {
>         width += 8;
>         d = vga->panning_buf;

Is it possible to do these checks once in vga.c and instead of changing 
the return value of the draw functions pass panning_buf as d if needed? 
Maybe that way the draw funcs could be left unchanged?

Regards,
BALATON Zoltan

Re: [PATCH 5/8] vga: optimize horizontal pel panning in 256-color modes

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

Il dom 31 dic 2023, 17:27 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> >     palette = vga->last_palette;
> >     hpel = (hpel >> 1) & 3;
> > +
> > +    /* For 256 color modes, we can adjust the source address and write
> directly
> > +     * to the destination, even if horizontal pel panning is active.
> However,
> > +     * the loop below assumes that the address does not wrap in the
> middle of a
> > +     * plane.  If that happens...
> > +     */
> > +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
> > +        addr += hpel * 4;
> > +        hpel = 0;
> > +    }
> > +
> > +    /* ... use the panning buffer as in planar modes.  */
> >     if (hpel) {
> >         width += 8;
> >         d = vga->panning_buf;
>
> Is it possible to do these checks once in vga.c and instead of changing
> the return value of the draw functions pass panning_buf as d if needed?
> Maybe that way the draw funcs could be left unchanged?
>

As of the previous patch it could, here however the logic for whether to
use the panning_buf depends on the drawing function; 8d2 is special and
different from the others.

I can remove the optimization, it's not super Important; but it's kind of
obvious to do it for the VGA 256-color modes, since they're the only ones
with the same bit alignment for all pixels.

Paolo


> Regards,
> BALATON Zoltan
>
>

[-- Attachment #2: Type: text/html, Size: 2155 bytes --]

Re: [PATCH 5/8] vga: optimize horizontal pel panning in 256-color modes

[BALATON Zoltan]

On Mon, 1 Jan 2024, Paolo Bonzini wrote:
> Il dom 31 dic 2023, 17:27 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>
>>>     palette = vga->last_palette;
>>>     hpel = (hpel >> 1) & 3;
>>> +
>>> +    /* For 256 color modes, we can adjust the source address and write
>> directly
>>> +     * to the destination, even if horizontal pel panning is active.
>> However,
>>> +     * the loop below assumes that the address does not wrap in the
>> middle of a
>>> +     * plane.  If that happens...
>>> +     */
>>> +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
>>> +        addr += hpel * 4;
>>> +        hpel = 0;
>>> +    }
>>> +
>>> +    /* ... use the panning buffer as in planar modes.  */
>>>     if (hpel) {
>>>         width += 8;
>>>         d = vga->panning_buf;
>>
>> Is it possible to do these checks once in vga.c and instead of changing
>> the return value of the draw functions pass panning_buf as d if needed?
>> Maybe that way the draw funcs could be left unchanged?
>>
>
> As of the previous patch it could, here however the logic for whether to
> use the panning_buf depends on the drawing function; 8d2 is special and
> different from the others.
>
> I can remove the optimization, it's not super Important; but it's kind of
> obvious to do it for the VGA 256-color modes, since they're the only ones
> with the same bit alignment for all pixels.

I guess that means it's not easy to do so my question is not so imprtant 
then.

Regards,
BALATON Zoltan

[PATCH 6/8] vga: reindent memory access code

[Paolo Bonzini]

The next patch will reuse latched memory access in text modes.  Start with
a patch that moves the latched access code out of the "if".

Best reviewed with "git diff -b".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 211 ++++++++++++++++++++++++-----------------------
 1 file changed, 110 insertions(+), 101 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 2467f3f6c65..08ba5bb1118 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -832,37 +832,41 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr)
     }
 
     if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
-        /* chain 4 mode : simplest access */
+        /* chain 4 mode : simplest access (but it should use the same
+         * algorithms as below; see e.g. vga_mem_writeb's plane mask check).
+         */
         assert(addr < s->vram_size);
-        ret = s->vram_ptr[addr];
-    } else if (s->gr[VGA_GFX_MODE] & 0x10) {
+        return s->vram_ptr[addr];
+    }
+
+    if (s->gr[VGA_GFX_MODE] & 0x10) {
         /* odd/even mode (aka text mode mapping) */
         plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
         addr = ((addr & ~1) << 1) | plane;
         if (addr >= s->vram_size) {
             return 0xff;
         }
-        ret = s->vram_ptr[addr];
-    } else {
-        /* standard VGA latched access */
-        if (addr * sizeof(uint32_t) >= s->vram_size) {
-            return 0xff;
-        }
-        s->latch = ((uint32_t *)s->vram_ptr)[addr];
-
-        if (!(s->gr[VGA_GFX_MODE] & 0x08)) {
-            /* read mode 0 */
-            plane = s->gr[VGA_GFX_PLANE_READ];
-            ret = GET_PLANE(s->latch, plane);
-        } else {
-            /* read mode 1 */
-            ret = (s->latch ^ mask16[s->gr[VGA_GFX_COMPARE_VALUE]]) &
-                mask16[s->gr[VGA_GFX_COMPARE_MASK]];
-            ret |= ret >> 16;
-            ret |= ret >> 8;
-            ret = (~ret) & 0xff;
-        }
+        return s->vram_ptr[addr];
     }
+
+    /* standard VGA latched access */
+    plane = s->gr[VGA_GFX_PLANE_READ];
+    if (addr * sizeof(uint32_t) >= s->vram_size) {
+        return 0xff;
+    }
+    s->latch = ((uint32_t *)s->vram_ptr)[addr];
+    if (!(s->gr[VGA_GFX_MODE] & 0x08)) {
+        /* read mode 0 */
+        ret = GET_PLANE(s->latch, plane);
+    } else {
+        /* read mode 1 */
+        ret = (s->latch ^ mask16[s->gr[VGA_GFX_COMPARE_VALUE]]) &
+            mask16[s->gr[VGA_GFX_COMPARE_MASK]];
+        ret |= ret >> 16;
+        ret |= ret >> 8;
+        ret = (~ret) & 0xff;
+    }
+
     return ret;
 }
 
@@ -912,7 +916,10 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
             s->plane_updated |= mask; /* only used to detect font change */
             memory_region_set_dirty(&s->vram, addr, 1);
         }
-    } else if (s->gr[VGA_GFX_MODE] & 0x10) {
+        return;
+    }
+
+    if (s->gr[VGA_GFX_MODE] & 0x10) {
         /* odd/even mode (aka text mode mapping) */
         plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
         mask = (1 << plane);
@@ -928,84 +935,86 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
             s->plane_updated |= mask; /* only used to detect font change */
             memory_region_set_dirty(&s->vram, addr, 1);
         }
-    } else {
-        /* standard VGA latched access */
-        write_mode = s->gr[VGA_GFX_MODE] & 3;
-        switch(write_mode) {
-        default:
-        case 0:
-            /* rotate */
-            b = s->gr[VGA_GFX_DATA_ROTATE] & 7;
-            val = ((val >> b) | (val << (8 - b))) & 0xff;
-            val |= val << 8;
-            val |= val << 16;
-
-            /* apply set/reset mask */
-            set_mask = mask16[s->gr[VGA_GFX_SR_ENABLE]];
-            val = (val & ~set_mask) |
-                (mask16[s->gr[VGA_GFX_SR_VALUE]] & set_mask);
-            bit_mask = s->gr[VGA_GFX_BIT_MASK];
-            break;
-        case 1:
-            val = s->latch;
-            goto do_write;
-        case 2:
-            val = mask16[val & 0x0f];
-            bit_mask = s->gr[VGA_GFX_BIT_MASK];
-            break;
-        case 3:
-            /* rotate */
-            b = s->gr[VGA_GFX_DATA_ROTATE] & 7;
-            val = (val >> b) | (val << (8 - b));
-
-            bit_mask = s->gr[VGA_GFX_BIT_MASK] & val;
-            val = mask16[s->gr[VGA_GFX_SR_VALUE]];
-            break;
-        }
-
-        /* apply logical operation */
-        func_select = s->gr[VGA_GFX_DATA_ROTATE] >> 3;
-        switch(func_select) {
-        case 0:
-        default:
-            /* nothing to do */
-            break;
-        case 1:
-            /* and */
-            val &= s->latch;
-            break;
-        case 2:
-            /* or */
-            val |= s->latch;
-            break;
-        case 3:
-            /* xor */
-            val ^= s->latch;
-            break;
-        }
-
-        /* apply bit mask */
-        bit_mask |= bit_mask << 8;
-        bit_mask |= bit_mask << 16;
-        val = (val & bit_mask) | (s->latch & ~bit_mask);
-
-    do_write:
-        /* mask data according to sr[2] */
-        mask = sr(s, VGA_SEQ_PLANE_WRITE);
-        s->plane_updated |= mask; /* only used to detect font change */
-        write_mask = mask16[mask];
-        if (addr * sizeof(uint32_t) >= s->vram_size) {
-            return;
-        }
-        ((uint32_t *)s->vram_ptr)[addr] =
-            (((uint32_t *)s->vram_ptr)[addr] & ~write_mask) |
-            (val & write_mask);
-#ifdef DEBUG_VGA_MEM
-        printf("vga: latch: [0x" HWADDR_FMT_plx "] mask=0x%08x val=0x%08x\n",
-               addr * 4, write_mask, val);
-#endif
-        memory_region_set_dirty(&s->vram, addr << 2, sizeof(uint32_t));
+        return;
     }
+
+    mask = sr(s, VGA_SEQ_PLANE_WRITE);
+
+    /* standard VGA latched access */
+    write_mode = s->gr[VGA_GFX_MODE] & 3;
+    switch(write_mode) {
+    default:
+    case 0:
+        /* rotate */
+        b = s->gr[VGA_GFX_DATA_ROTATE] & 7;
+        val = ((val >> b) | (val << (8 - b))) & 0xff;
+        val |= val << 8;
+        val |= val << 16;
+
+        /* apply set/reset mask */
+        set_mask = mask16[s->gr[VGA_GFX_SR_ENABLE]];
+        val = (val & ~set_mask) |
+            (mask16[s->gr[VGA_GFX_SR_VALUE]] & set_mask);
+        bit_mask = s->gr[VGA_GFX_BIT_MASK];
+        break;
+    case 1:
+        val = s->latch;
+        goto do_write;
+    case 2:
+        val = mask16[val & 0x0f];
+        bit_mask = s->gr[VGA_GFX_BIT_MASK];
+        break;
+    case 3:
+        /* rotate */
+        b = s->gr[VGA_GFX_DATA_ROTATE] & 7;
+        val = (val >> b) | (val << (8 - b));
+
+        bit_mask = s->gr[VGA_GFX_BIT_MASK] & val;
+        val = mask16[s->gr[VGA_GFX_SR_VALUE]];
+        break;
+    }
+
+    /* apply logical operation */
+    func_select = s->gr[VGA_GFX_DATA_ROTATE] >> 3;
+    switch(func_select) {
+    case 0:
+    default:
+        /* nothing to do */
+        break;
+    case 1:
+        /* and */
+        val &= s->latch;
+        break;
+    case 2:
+        /* or */
+        val |= s->latch;
+        break;
+    case 3:
+        /* xor */
+        val ^= s->latch;
+        break;
+    }
+
+    /* apply bit mask */
+    bit_mask |= bit_mask << 8;
+    bit_mask |= bit_mask << 16;
+    val = (val & bit_mask) | (s->latch & ~bit_mask);
+
+do_write:
+    /* mask data according to sr[2] */
+    s->plane_updated |= mask; /* only used to detect font change */
+    write_mask = mask16[mask];
+    if (addr * sizeof(uint32_t) >= s->vram_size) {
+        return;
+    }
+    ((uint32_t *)s->vram_ptr)[addr] =
+        (((uint32_t *)s->vram_ptr)[addr] & ~write_mask) |
+        (val & write_mask);
+#ifdef DEBUG_VGA_MEM
+    printf("vga: latch: [0x" HWADDR_FMT_plx "] mask=0x%08x val=0x%08x\n",
+           addr * 4, write_mask, val);
+#endif
+    memory_region_set_dirty(&s->vram, addr << 2, sizeof(uint32_t));
 }
 
 typedef void *vga_draw_line_func(VGACommonState *s1, uint8_t *d,
-- 
2.43.0

Re: [PATCH 6/8] vga: reindent memory access code

[Philippe Mathieu-Daudé]

On 31/12/23 10:39, Paolo Bonzini wrote:
> The next patch will reuse latched memory access in text modes.  Start with
> a patch that moves the latched access code out of the "if".
> 
> Best reviewed with "git diff -b".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/display/vga.c | 211 ++++++++++++++++++++++++-----------------------
>   1 file changed, 110 insertions(+), 101 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 7/8] vga: use latches in odd/even mode too

[Paolo Bonzini]

Jazz Jackrabbit uses odd/even mode with 256-color graphics.  This is
probably so that it can do very fast blitting with a decent resolution
(two pixels, compared to four pixels for "regular" mode X).

Accesses still use all planes (reads go to the latches and the game uses
read mode 1 so that the CPU always gets 0xFF; writes use the plane mask
register because the game sets bit 2 of the sequencer's memory mode
register).  For this to work, QEMU needs to use the code for latched
memory accesses in odd/even mode.  The only difference between odd/even
mode and "regular" planar mode is how the plane is computed in read mode
0, and how the planes are masked if the aforementioned bit 2 is reset.

It is almost enough to fix the game.  You also need to honor byte/word
mode selection, which is done in the next patch.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 08ba5bb1118..731501cb7af 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -842,15 +842,12 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr)
     if (s->gr[VGA_GFX_MODE] & 0x10) {
         /* odd/even mode (aka text mode mapping) */
         plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
-        addr = ((addr & ~1) << 1) | plane;
-        if (addr >= s->vram_size) {
-            return 0xff;
-        }
-        return s->vram_ptr[addr];
+        addr >>= 1;
+    } else {
+        /* standard VGA latched access */
+        plane = s->gr[VGA_GFX_PLANE_READ];
     }
 
-    /* standard VGA latched access */
-    plane = s->gr[VGA_GFX_PLANE_READ];
     if (addr * sizeof(uint32_t) >= s->vram_size) {
         return 0xff;
     }
@@ -903,11 +900,12 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         break;
     }
 
+    mask = sr(s, VGA_SEQ_PLANE_WRITE);
     if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
         /* chain 4 mode : simplest access */
         plane = addr & 3;
-        mask = (1 << plane);
-        if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) {
+        mask &= (1 << plane);
+        if (mask) {
             assert(addr < s->vram_size);
             s->vram_ptr[addr] = val;
 #ifdef DEBUG_VGA_MEM
@@ -919,26 +917,14 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         return;
     }
 
-    if (s->gr[VGA_GFX_MODE] & 0x10) {
-        /* odd/even mode (aka text mode mapping) */
-        plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
-        mask = (1 << plane);
-        if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) {
-            addr = ((addr & ~1) << 1) | plane;
-            if (addr >= s->vram_size) {
-                return;
-            }
-            s->vram_ptr[addr] = val;
-#ifdef DEBUG_VGA_MEM
-            printf("vga: odd/even: [0x" HWADDR_FMT_plx "]\n", addr);
-#endif
-            s->plane_updated |= mask; /* only used to detect font change */
-            memory_region_set_dirty(&s->vram, addr, 1);
-        }
-        return;
+    if ((sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_SEQ_MODE) == 0) {
+        mask &= (addr & 1) ? 0x0a : 0x05;
     }
 
-    mask = sr(s, VGA_SEQ_PLANE_WRITE);
+    if (s->gr[VGA_GFX_MODE] & 0x10) {
+        /* odd/even mode (aka text mode mapping) */
+        addr >>= 1;
+    }
 
     /* standard VGA latched access */
     write_mode = s->gr[VGA_GFX_MODE] & 3;
-- 
2.43.0

[PATCH 8/8] vga: sort-of implement word and double-word access modes

[Paolo Bonzini]

Jazz Jackrabbit has a very unusual VGA setup, where it uses odd/even mode
with 256-color graphics.  Probably, it wants to use fast VRAM-to-VRAM
copies without having to store 4 copies of the sprites as needed in mode
X, one for each mod-4 alignment; odd/even mode simplifies the code a
lot if it's okay to place on a 160-pixels horizontal grid.

At the same time, because it wants to use double buffering (a la "mode X")
it uses byte mode, not word mode as is the case in text modes.  In order
to implement the combination of odd/even mode (plane number comes from
bit 0 of the address) and byte mode (use all bytes of VRAM, whereas word
mode only uses bytes 0, 2, 4,... on each of the four planes), we need
to separate the effect on the plane number from the effect on the address.

Implementing the modes properly is a mess in QEMU, because it would
change the layout of VRAM and break migration.  As an approximation,
shift right when the CPU accesses memory instead of shifting left when
the CRT controller reads it.  A hack is needed in order to write font data
properly (see comment in the code), but it works well enough for the game.

Because doubleword and chain4 modes are now independent, chain4 does not
assert anymore that the address is in range.  Instead it just returns
all ones and discards writes, like other modes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c      | 90 +++++++++++++++++++++++++++++++------------
 hw/display/vga_regs.h |  4 ++
 2 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 731501cb7af..02b250ec0e6 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -832,25 +832,40 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr)
     }
 
     if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
-        /* chain 4 mode : simplest access (but it should use the same
-         * algorithms as below; see e.g. vga_mem_writeb's plane mask check).
-         */
-        assert(addr < s->vram_size);
-        return s->vram_ptr[addr];
-    }
-
-    if (s->gr[VGA_GFX_MODE] & 0x10) {
+        /* chain4 mode */
+        plane = addr & 3;
+        addr &= ~3;
+    } else if (s->gr[VGA_GFX_MODE] & VGA_GR05_HOST_ODD_EVEN) {
         /* odd/even mode (aka text mode mapping) */
         plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
-        addr >>= 1;
     } else {
         /* standard VGA latched access */
         plane = s->gr[VGA_GFX_PLANE_READ];
     }
 
+    if (s->gr[VGA_GFX_MISC] & VGA_GR06_CHAIN_ODD_EVEN) {
+        addr &= ~1;
+    }
+
+    /* Doubleword/word mode.  See comment in vga_mem_writeb */
+    if (s->cr[VGA_CRTC_UNDERLINE] & VGA_CR14_DW) {
+        addr >>= 2;
+    } else if ((s->gr[VGA_GFX_MODE] & VGA_GR05_HOST_ODD_EVEN) &&
+               (s->cr[VGA_CRTC_MODE] & VGA_CR17_WORD_BYTE) == 0) {
+        addr >>= 1;
+    }
+
     if (addr * sizeof(uint32_t) >= s->vram_size) {
         return 0xff;
     }
+
+    if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) {
+        /* chain 4 mode: simplified access (but it should use the same
+         * algorithms as below, see e.g. vga_mem_writeb's plane mask check).
+         */
+        return s->vram_ptr[(addr << 2) | plane];
+    }
+
     s->latch = ((uint32_t *)s->vram_ptr)[addr];
     if (!(s->gr[VGA_GFX_MODE] & 0x08)) {
         /* read mode 0 */
@@ -870,8 +885,9 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr)
 /* called for accesses between 0xa0000 and 0xc0000 */
 void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
 {
-    int memory_map_mode, plane, write_mode, b, func_select, mask;
+    int memory_map_mode, write_mode, b, func_select, mask;
     uint32_t write_mask, bit_mask, set_mask;
+    int plane = 0;
 
 #ifdef DEBUG_VGA_MEM
     printf("vga: [0x" HWADDR_FMT_plx "] = 0x%02x\n", addr, val);
@@ -905,9 +921,47 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         /* chain 4 mode : simplest access */
         plane = addr & 3;
         mask &= (1 << plane);
+        addr &= ~3;
+    } else {
+        if ((sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_SEQ_MODE) == 0) {
+            mask &= (addr & 1) ? 0x0a : 0x05;
+        }
+    }
+
+    if (s->gr[VGA_GFX_MISC] & VGA_GR06_CHAIN_ODD_EVEN) {
+        addr &= ~1;
+    }
+
+    /* Doubleword/word mode.  These should be honored when displaying,
+     * not when reading/writing to memory!  For example, chain4 modes
+     * use double-word mode and, on real hardware, would fetch bytes
+     * 0,1,2,3, 16,17,18,19, 32,33,34,35, etc.  Text modes use word
+     * mode and, on real hardware, would fetch bytes 0,1, 8,9, etc.
+     *
+     * QEMU instead shifted addresses on memory accesses because it
+     * allows more optimizations (e.g. chain4_alias) and simplifies
+     * the draw_line handlers. Unfortunately, there is one case where
+     * the difference shows.  When fetching font data, accesses are
+     * always in consecutive bytes, even if the text/attribute pairs
+     * are done in word mode.  Hence, doing a right shift when operating
+     * on font data is wrong.  So check the odd/even mode bits together with
+     * word mode bit.  The odd/even read bit is 0 when reading font data,
+     * and the odd/even write bit is 1 when writing it.
+     */
+    if (s->cr[VGA_CRTC_UNDERLINE] & VGA_CR14_DW) {
+        addr >>= 2;
+    } else if ((sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_SEQ_MODE) == 0 &&
+               (s->cr[VGA_CRTC_MODE] & VGA_CR17_WORD_BYTE) == 0) {
+        addr >>= 1;
+    }
+
+    if (addr * sizeof(uint32_t) >= s->vram_size) {
+        return;
+    }
+
+    if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
         if (mask) {
-            assert(addr < s->vram_size);
-            s->vram_ptr[addr] = val;
+            s->vram_ptr[(addr << 2) | plane] = val;
 #ifdef DEBUG_VGA_MEM
             printf("vga: chain4: [0x" HWADDR_FMT_plx "]\n", addr);
 #endif
@@ -917,15 +971,6 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         return;
     }
 
-    if ((sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_SEQ_MODE) == 0) {
-        mask &= (addr & 1) ? 0x0a : 0x05;
-    }
-
-    if (s->gr[VGA_GFX_MODE] & 0x10) {
-        /* odd/even mode (aka text mode mapping) */
-        addr >>= 1;
-    }
-
     /* standard VGA latched access */
     write_mode = s->gr[VGA_GFX_MODE] & 3;
     switch(write_mode) {
@@ -990,9 +1035,6 @@ do_write:
     /* mask data according to sr[2] */
     s->plane_updated |= mask; /* only used to detect font change */
     write_mask = mask16[mask];
-    if (addr * sizeof(uint32_t) >= s->vram_size) {
-        return;
-    }
     ((uint32_t *)s->vram_ptr)[addr] =
         (((uint32_t *)s->vram_ptr)[addr] & ~write_mask) |
         (val & write_mask);
diff --git a/hw/display/vga_regs.h b/hw/display/vga_regs.h
index 7fdba34b9b1..40e673f164d 100644
--- a/hw/display/vga_regs.h
+++ b/hw/display/vga_regs.h
@@ -100,7 +100,9 @@
 
 /* VGA CRT controller bit masks */
 #define VGA_CR11_LOCK_CR0_CR7   0x80 /* lock writes to CR0 - CR7 */
+#define VGA_CR14_DW             0x40
 #define VGA_CR17_H_V_SIGNALS_ENABLED 0x80
+#define VGA_CR17_WORD_BYTE      0x40
 
 /* VGA attribute controller register indices */
 #define VGA_ATC_PALETTE0        0x00
@@ -154,6 +156,8 @@
 #define VGA_GFX_BIT_MASK        0x08
 
 /* VGA graphics controller bit masks */
+#define VGA_GR05_HOST_ODD_EVEN  0x10
 #define VGA_GR06_GRAPHICS_MODE  0x01
+#define VGA_GR06_CHAIN_ODD_EVEN 0x02
 
 #endif /* HW_VGA_REGS_H */
-- 
2.43.0