On Sat, 5 Feb 2022 at 14:24, BALATON Zoltan <bala...@eik.bme.hu> wrote:
>
> 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-ayl...@ilande.co.uk>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> >> - 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.
The memset chosen size seems odd -- MAXX and MAXY are
constants, but the size of the memory block here is
s->vram_size * 9, which might be smaller than MAXX * MAXY...
> > 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?
I think we should clear the whole of the vram -- it ought to go
back to the same state as when the device was completed. If I'm
reading the sun4m board code correctly, the other parts of
the vram are mapped into the guest address space too.
So I would go for
memset(s->vram, 0, memory_region_size(&s->vram_mem));
memory_region_set_dirty(&s->vram_mem, 0, memory_region_size(&s->vram_mem),
DIRTY_MEMORY_VGA);
We can then delete the MAXX and MAXY defines, which were
previously used only in these two lines and which don't actually
have any relationship to the maximum size of the framebuffer.
The handling of the vram buffer seems weird in this device overall,
though -- the memory block is divided into three parts
* main vram, one byte per pixel
* vram24, four bytes per pixel
* cplane, four bytes per pixel
As far as I can see, only if depth=24 (fixed at device creation
time) do we use the vram24 and cplane parts. But we create the
memory block at the same size regardless of depth and we expose
the vram24 and cplane parts to the guest as sysbus MMIO regions
that are mapped into guest memory regardless of depth...
-- PMM
