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

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

[Philippe Mathieu-Daudé via]

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

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

[Mark Cave-Ayland]

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.

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

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

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

[Peter Maydell]

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

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

[Mark Cave-Ayland]

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.

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

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé via]

On Sun, Feb 6, 2022 at 11:44 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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...

I guess I'll go with your code suggestion as a first step, and we can figure
out whether this MR is pointless and remove it later.