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);
- 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 */
--
2.34.1
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);
> - 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 update_palette_entries()
here e.g.
tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
ATB,
Mark.
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
On Sat, 5 Feb 2022 at 14:24, BALATON Zoltan <balaton@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-ayland@ilande.co.uk>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@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
On 05/02/2022 15:39, Peter Maydell wrote: > On Sat, 5 Feb 2022 at 14:24, BALATON Zoltan <balaton@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-ayland@ilande.co.uk> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@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.
On Sun, 6 Feb 2022 at 09:30, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 05/02/2022 15:39, Peter Maydell wrote: > > 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. A guest won't notice if we expose stuff to it that it doesn't expect to be there -- but if the 8-bit-only device is not supposed to have those memory regions we shouldn't be creating them... -- PMM
© 2016 - 2024 Red Hat, Inc.