Re: [RFC PATCH] hw/display: modernise the vga debug code (!!GenAI!!)

["Alex Bennée" <alex.bennee@linaro.org>]

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