On 05/02/2022 15:39, Peter Maydell wrote:
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.
That makes sense to me. My guess is that MAXX and MAXY were originally used for
calculations on the original 8-bit framebuffer and the size calculation wasn't
updated when 24-bit support was added.
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...
(goes and looks)
It does look a bit odd certainly. Without Blue Swirl being around all I can only
guess as to why everything is configured to use an alias onto a single VRAM memory
region :/
As for exposing the vram24 and cplane parts, I don't think it matters since 24-bit
mode is clearly designed to be backwards compatible with 8-bit mode. During
initialisation OpenBIOS reads the colour depth using the fw_cfg interface and adds
the registers for that mode into the DT as required so the correct information is
exposed to the guest.
ATB,
Mark.
