Re: [PATCH v2] hw/display/tcx: Mark the VRAM dirty upon reset

[BALATON Zoltan]

On Sat, 5 Feb 2022, Mark Cave-Ayland wrote:
> On 03/02/2022 00:05, Philippe Mathieu-Daudé via wrote:
>
> > When resetting we don't want to *reset* the dirty bitmap,
> > we want to *set* it to mark the entire VRAM dirty due to
> > the memset() call.
> >
> > Replace memory_region_reset_dirty() by tcx_set_dirty()
> > which conveniently set the correct ranges dirty.
> >
> > Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > Supersedes: <20220122000707.82918-1-f4bug@amsat.org>
> > ---
> >   hw/display/tcx.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> > index d4d09d0df8..90e2975e35 100644
> > --- a/hw/display/tcx.c
> > +++ b/hw/display/tcx.c
> > @@ -371,8 +371,7 @@ 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);

Should checkpatch complain about missing spaces around * here?

> > -    memory_region_reset_dirty(&s->vram_mem, 0, MAXX * MAXY * (1 + 4 + 4),
> > -                              DIRTY_MEMORY_VGA);
> > +    tcx_set_dirty(s, 0, MAXX * MAXY);
> >       s->dac_index = 0;
> >       s->dac_state = 0;
> >       s->cursx = 0xf000; /* Put cursor off screen */
>
> I don't think the size calculation of MAXX * MAXY is correct when comparing 
> with above? I think it's easiest just to use the same approach as

Xonsidering that the memset has the same length it should be correct as 
that's what has been changed (assuming tcx_set_dirty works correctly), but 
maybe there's some trick here I don't know about.

> update_palette_entries() here e.g.
>
>    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));

This may be an overkill. Although probably does not matter but it's still 
cleaner to only set dirty what has been changed otherwise you've just 
disabled dirty tracking. On the other hand, if this is a reset routine do 
you only want to clear the displayable part of vram or the whole vram?

Regards,
BALATON Zoltan