Fixes: f9b925fd41 vga: introduce VGADisplayParams
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/vga.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/vga.c b/hw/display/vga.c
index ee7d97b5c2..26039f0020 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1662,7 +1662,7 @@ 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],
+ width, height, v, s->params.line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
#endif
addr1 = (s->params.start_addr * 4);
--
2.41.3BALATON Zoltan <balaton@eik.bme.hu> writes:
> Fixes: f9b925fd41 vga: introduce VGADisplayParams
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/vga.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index ee7d97b5c2..26039f0020 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1662,7 +1662,7 @@ 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],
> + width, height, v, s->params.line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
> s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
> #endif
> addr1 = (s->params.start_addr * 4);
Really it would be worth just deleting the #if 0 code - if it's really
useful for debugging you could turn it into a tracepoint.
There are a bunch of other #if 0/#if DEBUG_* statements that could also
be dumped/modernised.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On Mon, 30 Mar 2026, Alex Bennée wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>
>> Fixes: f9b925fd41 vga: introduce VGADisplayParams
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/display/vga.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/vga.c b/hw/display/vga.c
>> index ee7d97b5c2..26039f0020 100644
>> --- a/hw/display/vga.c
>> +++ b/hw/display/vga.c
>> @@ -1662,7 +1662,7 @@ 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],
>> + width, height, v, s->params.line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
>> s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
>> #endif
>> addr1 = (s->params.start_addr * 4);
>
> Really it would be worth just deleting the #if 0 code - if it's really
> useful for debugging you could turn it into a tracepoint.
>
> There are a bunch of other #if 0/#if DEBUG_* statements that could also
> be dumped/modernised.
Yes, it's useful for debugging (I used it to test frame buffer endianness)
so probably should not be deleted. No, I don't want to overhaul and
modernise the whole device and convert everything to trace points because
as you say there's a bunch of those. So for now I just left it as it is
and fixed the typo in it. I don't intend to do more work on this.
Regards,
BALATON Zoltan
Remove dead code, replace debug prints with trace points.
Link: https://patchew.org/QEMU/20260330140656.46FE55969F2@zero.eik.bme.hu/
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
As an experiment I prompted Gemini with the thread contents and asked
it to make the changes. It cost about 9p in inference and I would
argue this barely counts as copyrightable as it is a simple
transformation of existing patterns.
---
hw/display/vga.c | 89 ++++++++---------------------------------
hw/display/trace-events | 11 +++++
2 files changed, 27 insertions(+), 73 deletions(-)
diff --git a/hw/display/vga.c b/hw/display/vga.c
index ee7d97b5c21..081a3b2576c 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -40,9 +40,6 @@
#include "migration/vmstate.h"
#include "trace.h"
-//#define DEBUG_VGA_MEM
-//#define DEBUG_VGA_REG
-
bool have_vga = true;
/* 16 state changes per vertical frame @60 Hz */
@@ -201,9 +198,6 @@ static void vga_precise_update_retrace_info(VGACommonState *s)
int vretr_end_line;
int dots;
-#if 0
- int div2, sldiv2;
-#endif
int clocking_mode;
int clock_sel;
const int clk_hz[] = {25175000, 28322000, 25175000, 25175000};
@@ -245,40 +239,9 @@ static void vga_precise_update_retrace_info(VGACommonState *s)
r->hend = r->hstart + hretr_end_char + 1;
r->htotal = htotal_chars;
-#if 0
- div2 = (s->cr[VGA_CRTC_MODE] >> 2) & 1;
- sldiv2 = (s->cr[VGA_CRTC_MODE] >> 3) & 1;
- printf (
- "hz=%f\n"
- "htotal = %d\n"
- "hretr_start = %d\n"
- "hretr_skew = %d\n"
- "hretr_end = %d\n"
- "vtotal = %d\n"
- "vretr_start = %d\n"
- "vretr_end = %d\n"
- "div2 = %d sldiv2 = %d\n"
- "clocking_mode = %d\n"
- "clock_sel = %d %d\n"
- "dots = %d\n"
- "ticks/char = %" PRId64 "\n"
- "\n",
- (double) NANOSECONDS_PER_SECOND / (r->ticks_per_char * r->total_chars),
- htotal_chars,
- hretr_start_char,
- hretr_skew_chars,
- hretr_end_char,
- vtotal_lines,
- vretr_start_line,
- vretr_end_line,
- div2, sldiv2,
- clocking_mode,
- clock_sel,
- clk_hz[clock_sel],
- dots,
- r->ticks_per_char
- );
-#endif
+ trace_vga_update_retrace_info(htotal_chars, vtotal_lines,
+ vretr_start_line, vretr_end_line,
+ dots, r->ticks_per_char);
}
static uint8_t vga_precise_retrace(VGACommonState *s)
@@ -358,9 +321,7 @@ uint32_t vga_ioport_read(void *opaque, uint32_t addr)
break;
case VGA_SEQ_D:
val = s->sr[s->sr_index];
-#ifdef DEBUG_VGA_REG
- printf("vga: read SR%x = 0x%02x\n", s->sr_index, val);
-#endif
+ trace_vga_read_sr(s->sr_index, val);
break;
case VGA_PEL_IR:
val = s->dac_state;
@@ -386,9 +347,7 @@ uint32_t vga_ioport_read(void *opaque, uint32_t addr)
break;
case VGA_GFX_D:
val = s->gr[s->gr_index];
-#ifdef DEBUG_VGA_REG
- printf("vga: read GR%x = 0x%02x\n", s->gr_index, val);
-#endif
+ trace_vga_read_gr(s->gr_index, val);
break;
case VGA_CRT_IM:
case VGA_CRT_IC:
@@ -397,9 +356,7 @@ uint32_t vga_ioport_read(void *opaque, uint32_t addr)
case VGA_CRT_DM:
case VGA_CRT_DC:
val = s->cr[s->cr_index];
-#ifdef DEBUG_VGA_REG
- printf("vga: read CR%x = 0x%02x\n", s->cr_index, val);
-#endif
+ trace_vga_read_cr(s->cr_index, val);
break;
case VGA_IS1_RM:
case VGA_IS1_RC:
@@ -467,9 +424,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
s->sr_index = val & 7;
break;
case VGA_SEQ_D:
-#ifdef DEBUG_VGA_REG
- printf("vga: write SR%x = 0x%02x\n", s->sr_index, val);
-#endif
+ trace_vga_write_sr(s->sr_index, val);
s->sr[s->sr_index] = val & sr_mask[s->sr_index];
if (s->sr_index == VGA_SEQ_CLOCK_MODE) {
s->update_retrace_info(s);
@@ -498,9 +453,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
s->gr_index = val & 0x0f;
break;
case VGA_GFX_D:
-#ifdef DEBUG_VGA_REG
- printf("vga: write GR%x = 0x%02x\n", s->gr_index, val);
-#endif
+ trace_vga_write_gr(s->gr_index, val);
s->gr[s->gr_index] = val & gr_mask[s->gr_index];
vbe_update_vgaregs(s);
vga_update_memory_access(s);
@@ -511,9 +464,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
break;
case VGA_CRT_DM:
case VGA_CRT_DC:
-#ifdef DEBUG_VGA_REG
- printf("vga: write CR%x = 0x%02x\n", s->cr_index, val);
-#endif
+ trace_vga_write_cr(s->cr_index, val);
/* handle CR0-7 protection */
if ((s->cr[VGA_CRTC_V_SYNC_END] & VGA_CR11_LOCK_CR0_CR7) &&
s->cr_index <= VGA_CRTC_OVERFLOW) {
@@ -873,9 +824,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
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);
-#endif
+ trace_vga_vram_write(addr, val);
/* convert to VGA memory offset */
memory_map_mode = (s->gr[VGA_GFX_MISC] >> 2) & 3;
addr &= 0x1ffff;
@@ -945,9 +894,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
if (mask) {
s->vram_ptr[(addr << 2) | plane] = val;
-#ifdef DEBUG_VGA_MEM
- printf("vga: chain4: [0x" HWADDR_FMT_plx "]\n", addr);
-#endif
+ trace_vga_vram_write((addr << 2) | plane, val);
s->plane_updated |= mask; /* only used to detect font change */
memory_region_set_dirty(&s->vram, addr, 1);
}
@@ -1021,10 +968,7 @@ do_write:
((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
+ trace_vga_vram_latch_write(addr << 2, write_mask, val);
memory_region_set_dirty(&s->vram, addr << 2, sizeof(uint32_t));
}
@@ -1660,11 +1604,10 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
s->cursor_invalidate(s);
}
-#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->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
-#endif
+ trace_vga_draw_graphic(width, height, v, s->params.line_offset,
+ s->cr[9], s->cr[VGA_CRTC_MODE],
+ s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
+
addr1 = (s->params.start_addr * 4);
y_start = -1;
d = surface_data(surface);
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 4bfc457fbac..c6591c90ca8 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -130,6 +130,17 @@ vga_std_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
vga_std_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
vga_vbe_read(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
vga_vbe_write(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_vram_read(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%x"
+vga_vram_write(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%x"
+vga_vram_latch_write(uint64_t addr, uint32_t mask, uint32_t val) "addr 0x%"PRIx64" mask 0x%08x val 0x%08x"
+vga_read_sr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_write_sr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_read_gr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_write_gr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_read_cr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_write_cr(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
+vga_draw_graphic(uint32_t w, uint32_t h, uint32_t v, uint32_t line_offset, uint32_t cr09, uint32_t cr17, uint32_t linecmp, uint32_t sr01) "w=%u h=%u v=%u line_offset=%u cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%u sr[0x01]=0x%02x"
+vga_update_retrace_info(uint32_t htotal, uint32_t vtotal, uint32_t vretr_start, uint32_t vretr_end, uint32_t dots, uint64_t ticks_per_char) "htotal %u, vtotal %u, vretr_start %u, vretr_end %u, dots %u, ticks/char %"PRIu64
# cirrus_vga.c
vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
--
2.47.3
On Mon, 30 Mar 2026, Alex Bennée wrote:
> @@ -1660,11 +1604,10 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
> s->cursor_invalidate(s);
> }
>
> -#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->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
> -#endif
> + trace_vga_draw_graphic(width, height, v, s->params.line_offset,
> + s->cr[9], s->cr[VGA_CRTC_MODE],
> + s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
> +
> addr1 = (s->params.start_addr * 4);
> y_start = -1;
> d = surface_data(surface);
I've found moving this to line 1678 into an else branch of if
(!full_update) so it's only logged once when parameters change would
probably be better. I haven't looked at the other logs as I only needed
this one at this time. The point is that it probably needs more thought
than automatic conversion so unless somebody wants to do that it's easier
to leave it as it is or convert them one by one.
Regards,
BALATON Zoltan
On 31/3/26 00:28, Alex Bennée wrote:
> Remove dead code, replace debug prints with trace points.
>
> Link: https://patchew.org/QEMU/20260330140656.46FE55969F2@zero.eik.bme.hu/
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> As an experiment I prompted Gemini with the thread contents and asked
> it to make the changes. It cost about 9p in inference and I would
> argue this barely counts as copyrightable as it is a simple
> transformation of existing patterns.
> ---
> hw/display/vga.c | 89 ++++++++---------------------------------
> hw/display/trace-events | 11 +++++
> 2 files changed, 27 insertions(+), 73 deletions(-)
> @@ -873,9 +824,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
> 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);
> -#endif
> + trace_vga_vram_write(addr, val);
OK, ...
> /* convert to VGA memory offset */
> memory_map_mode = (s->gr[VGA_GFX_MISC] >> 2) & 3;
> addr &= 0x1ffff;
> @@ -945,9 +894,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
> if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
> if (mask) {
> s->vram_ptr[(addr << 2) | plane] = val;
> -#ifdef DEBUG_VGA_MEM
> - printf("vga: chain4: [0x" HWADDR_FMT_plx "]\n", addr);
> -#endif
> + trace_vga_vram_write((addr << 2) | plane, val);
... but why print the index instead of the address here?
> s->plane_updated |= mask; /* only used to detect font change */
> memory_region_set_dirty(&s->vram, addr, 1);
> }
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 31/3/26 00:28, Alex Bennée wrote:
>> Remove dead code, replace debug prints with trace points.
>> Link:
>> https://patchew.org/QEMU/20260330140656.46FE55969F2@zero.eik.bme.hu/
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> As an experiment I prompted Gemini with the thread contents and asked
>> it to make the changes. It cost about 9p in inference and I would
>> argue this barely counts as copyrightable as it is a simple
>> transformation of existing patterns.
>> ---
>> hw/display/vga.c | 89 ++++++++---------------------------------
>> hw/display/trace-events | 11 +++++
>> 2 files changed, 27 insertions(+), 73 deletions(-)
>
>
>> @@ -873,9 +824,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
>> 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);
>> -#endif
>> + trace_vga_vram_write(addr, val);
>
> OK, ...
>
>> /* convert to VGA memory offset */
>> memory_map_mode = (s->gr[VGA_GFX_MISC] >> 2) & 3;
>> addr &= 0x1ffff;
>> @@ -945,9 +894,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
>> if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) {
>> if (mask) {
>> s->vram_ptr[(addr << 2) | plane] = val;
>> -#ifdef DEBUG_VGA_MEM
>> - printf("vga: chain4: [0x" HWADDR_FMT_plx "]\n", addr);
>> -#endif
>> + trace_vga_vram_write((addr << 2) | plane, val);
>
> ... but why print the index instead of the address here?
I expect it makes the trace data more closely match the actual access.
As a human I probably would have written:
if (mask) {
size_t idx = (addr << 2) | plane;
s->vram_ptr[idx] = val;
trace_vga_vram_write(idx, val);
...
}
to avoid the double calculation (even though I'm sure the compiler would
have clocked it).
>
>> s->plane_updated |= mask; /* only used to detect font change */
>> memory_region_set_dirty(&s->vram, addr, 1);
>> }
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
© 2016 - 2026 Red Hat, Inc.