Re: [PATCH] hw/display/tcx: Avoid clearing dirty bitmap in DeviceReset()

[Philippe Mathieu-Daudé]

On 1/26/22 08:23, Mark Cave-Ayland wrote:
> On 22/01/2022 00:07, Philippe Mathieu-Daudé via wrote:
> 
>> Commit 2dd285b5f3 ("tcx: make display updates thread safe")
>> converted this model to use the DirtyBitmapSnapshot API,
>> resetting the dirty bitmap in tcx_update_display(). There
>> is no need to do it again in the DeviceReset handler.
>>
>> See more details in commit fec5e8c92b ("vga: make display
>> updates thread safe.").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/display/tcx.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>> index d4d09d0df8..22b0ae4761 100644
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -371,8 +371,6 @@ static void tcx_reset(DeviceState *d)
>>       s->r[258] = s->g[258] = s->b[258] = 255;
>>       update_palette_entries(s, 0, 260);
>>       memset(s->vram, 0, MAXX*MAXY);
>> -    memory_region_reset_dirty(&s->vram_mem, 0, MAXX * MAXY * (1 + 4 +
>> 4),
>> -                              DIRTY_MEMORY_VGA);
>>       s->dac_index = 0;
>>       s->dac_state = 0;
>>       s->cursx = 0xf000; /* Put cursor off screen */
> 
> I think the issue here is that tcx_reset() should be marking the entire
> VRAM dirty due to the memset() i.e. we should be setting the dirty
> bitmap rather than resetting it. Perhaps memory_region_reset_dirty()
> should be replaced with tcx_invalidate_display() instead?

Yeah I was not sure, and your suggestion sounds right. I'll respin.