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

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

Posted by Philippe Mathieu-Daudé via 2 years, 3 months ago

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 */
-- 
2.34.1

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

Posted by Mark Cave-Ayland 2 years, 2 months ago

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?


ATB,

Mark.

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

Posted by Philippe Mathieu-Daudé via 2 years, 2 months ago

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.