The macfb VRAM memory region was configured with coalescing rather than dirty
memory logging enabled, causing some areas of the screen not to redraw after
a full screen update.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 8ac919a065 ("hw/m68k: add Nubus macfb video card")
---
 hw/display/macfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 277d3e6633..4bd7c3ad6a 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -661,9 +661,9 @@ static bool macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
 
     memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
                            MACFB_VRAM_SIZE, &error_abort);
+    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
     s->vram = memory_region_get_ram_ptr(&s->mem_vram);
     s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
-    memory_region_set_coalescing(&s->mem_vram);
 
     s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
     macfb_update_mode(s);
-- 
2.20.1

Le 08/01/2022 à 17:41, Mark Cave-Ayland a écrit :
> The macfb VRAM memory region was configured with coalescing rather than dirty
> memory logging enabled, causing some areas of the screen not to redraw after
> a full screen update.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 8ac919a065 ("hw/m68k: add Nubus macfb video card")
> ---
>   hw/display/macfb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 277d3e6633..4bd7c3ad6a 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -661,9 +661,9 @@ static bool macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>   
>       memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
>                              MACFB_VRAM_SIZE, &error_abort);
> +    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
>       s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>       s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
> -    memory_region_set_coalescing(&s->mem_vram);
>   
>       s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
>       macfb_update_mode(s);

I understant why you add memory_region_set_log() but I don't understand why you remove 
memory_region_set_coalescing().

Thanks,
Laurent

On 08/01/2022 16:53, Laurent Vivier wrote:

> Le 08/01/2022 à 17:41, Mark Cave-Ayland a écrit :
>> The macfb VRAM memory region was configured with coalescing rather than dirty
>> memory logging enabled, causing some areas of the screen not to redraw after
>> a full screen update.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: 8ac919a065 ("hw/m68k: add Nubus macfb video card")
>> ---
>>   hw/display/macfb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index 277d3e6633..4bd7c3ad6a 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -661,9 +661,9 @@ static bool macfb_common_realize(DeviceState *dev, MacfbState 
>> *s, Error **errp)
>>       memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
>>                              MACFB_VRAM_SIZE, &error_abort);
>> +    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
>>       s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>>       s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>> -    memory_region_set_coalescing(&s->mem_vram);
>>       s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
>>       macfb_update_mode(s);
> 
> I understant why you add memory_region_set_log() but I don't understand why you 
> remove memory_region_set_coalescing().

Looking at the other display devices, only VGA and cirrus use 
memory_region_set_coalescing() and that's on the IO ports rather than the VRAM.

Based upon this my suspicion is that this is mainly a vmexit optimisation when using 
KVM which isn't relevant here for macfb.


ATB,

Mark.

Le 08/01/2022 à 18:15, Mark Cave-Ayland a écrit :
> On 08/01/2022 16:53, Laurent Vivier wrote:
> 
>> Le 08/01/2022 à 17:41, Mark Cave-Ayland a écrit :
>>> The macfb VRAM memory region was configured with coalescing rather than dirty
>>> memory logging enabled, causing some areas of the screen not to redraw after
>>> a full screen update.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Fixes: 8ac919a065 ("hw/m68k: add Nubus macfb video card")
>>> ---
>>>   hw/display/macfb.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>>> index 277d3e6633..4bd7c3ad6a 100644
>>> --- a/hw/display/macfb.c
>>> +++ b/hw/display/macfb.c
>>> @@ -661,9 +661,9 @@ static bool macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
>>>       memory_region_init_ram(&s->mem_vram, OBJECT(dev), "macfb-vram",
>>>                              MACFB_VRAM_SIZE, &error_abort);
>>> +    memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA);
>>>       s->vram = memory_region_get_ram_ptr(&s->mem_vram);
>>>       s->vram_bit_mask = MACFB_VRAM_SIZE - 1;
>>> -    memory_region_set_coalescing(&s->mem_vram);
>>>       s->vbl_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, macfb_vbl_timer, s);
>>>       macfb_update_mode(s);
>>
>> I understant why you add memory_region_set_log() but I don't understand why you remove 
>> memory_region_set_coalescing().
> 
> Looking at the other display devices, only VGA and cirrus use memory_region_set_coalescing() and 
> that's on the IO ports rather than the VRAM.
 >
> Based upon this my suspicion is that this is mainly a vmexit optimisation when using KVM which isn't 
> relevant here for macfb.

You're right.

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Thanks,
Laurent