[PATCH 0/2] hw/display/vmware_vga: supress debug output and fix

[PATCH 0/2] hw/display/vmware_vga: supress debug output and fix

[Carwyn Ellis]

Two patches addressing the following in the vmware vga display code

  - only show debug output if DEBUG is explicitly enabled

  - do not discard display updates

    This prevents an issue that can cause garbled display output when
    a high number of screen updates are being requested.

    The FIFO queue size has been increased and all display update events
    are now processed ensuring correct display output even during
    periods of high activity.

Carwyn Ellis (2):
  hw/display/vmware_vga: only show debug output if DEBUG enabled
  hw/display/vmware_vga: do not discard screen updates

 hw/display/vmware_vga.c | 50 +++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/display/vmware_vga: only show debug output if DEBUG enabled

[Carwyn Ellis]

Debug output was always being sent to STDERR. This has been replaced by
a define that will only show this output when DEBUG is set to true.

Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
---
 hw/display/vmware_vga.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index e2969a6c81..8080e085d1 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -43,6 +43,12 @@
 
 /* See http://vmware-svga.sf.net/ for some documentation on VMWare SVGA */
 
+#ifdef DEBUG
+#define VMWARE_VGA_DEBUG(...) { (void) fprintf(stdout, __VA_ARGS__); }
+#else
+#define VMWARE_VGA_DEBUG(...) ((void) 0)
+#endif
+
 struct vmsvga_state_s {
     VGACommonState vga;
 
@@ -297,45 +303,45 @@ static inline bool vmsvga_verify_rect(DisplaySurface *surface,
                                       int x, int y, int w, int h)
 {
     if (x < 0) {
-        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
+        VMWARE_VGA_DEBUG("%s: x was < 0 (%d)\n", name, x);
         return false;
     }
     if (x > SVGA_MAX_WIDTH) {
-        fprintf(stderr, "%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
+        VMWARE_VGA_DEBUG("%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
         return false;
     }
     if (w < 0) {
-        fprintf(stderr, "%s: w was < 0 (%d)\n", name, w);
+        VMWARE_VGA_DEBUG("%s: w was < 0 (%d)\n", name, w);
         return false;
     }
     if (w > SVGA_MAX_WIDTH) {
-        fprintf(stderr, "%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
+        VMWARE_VGA_DEBUG("%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
         return false;
     }
     if (x + w > surface_width(surface)) {
-        fprintf(stderr, "%s: width was > %d (x: %d, w: %d)\n",
+        VMWARE_VGA_DEBUG("%s: width was > %d (x: %d, w: %d)\n",
                 name, surface_width(surface), x, w);
         return false;
     }
 
     if (y < 0) {
-        fprintf(stderr, "%s: y was < 0 (%d)\n", name, y);
+        VMWARE_VGA_DEBUG("%s: y was < 0 (%d)\n", name, y);
         return false;
     }
     if (y > SVGA_MAX_HEIGHT) {
-        fprintf(stderr, "%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
+        VMWARE_VGA_DEBUG("%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
         return false;
     }
     if (h < 0) {
-        fprintf(stderr, "%s: h was < 0 (%d)\n", name, h);
+        VMWARE_VGA_DEBUG("%s: h was < 0 (%d)\n", name, h);
         return false;
     }
     if (h > SVGA_MAX_HEIGHT) {
-        fprintf(stderr, "%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
+        VMWARE_VGA_DEBUG("%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
         return false;
     }
     if (y + h > surface_height(surface)) {
-        fprintf(stderr, "%s: update height > %d (y: %d, h: %d)\n",
+        VMWARE_VGA_DEBUG("%s: update height > %d (y: %d, h: %d)\n",
                 name, surface_height(surface), y, h);
         return false;
     }
-- 
2.34.1

Re: [PATCH 1/2] hw/display/vmware_vga: only show debug output if DEBUG enabled

[Laurent Vivier]

Le 04/01/2022 à 10:11, Carwyn Ellis a écrit :
> Debug output was always being sent to STDERR. This has been replaced by
> a define that will only show this output when DEBUG is set to true.
> 
> Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
> ---
>   hw/display/vmware_vga.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index e2969a6c81..8080e085d1 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -43,6 +43,12 @@
>   
>   /* See http://vmware-svga.sf.net/ for some documentation on VMWare SVGA */
>   
> +#ifdef DEBUG
> +#define VMWARE_VGA_DEBUG(...) { (void) fprintf(stdout, __VA_ARGS__); }
> +#else
> +#define VMWARE_VGA_DEBUG(...) ((void) 0)
> +#endif
> +

Could you replace this macro by adding some trace-events instead.

See https://qemu-project.gitlab.io/qemu/devel/tracing.html#using-trace-events

Thanks,
Laurent

>   struct vmsvga_state_s {
>       VGACommonState vga;
>   
> @@ -297,45 +303,45 @@ static inline bool vmsvga_verify_rect(DisplaySurface *surface,
>                                         int x, int y, int w, int h)
>   {
>       if (x < 0) {
> -        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
> +        VMWARE_VGA_DEBUG("%s: x was < 0 (%d)\n", name, x);
>           return false;
>       }
>       if (x > SVGA_MAX_WIDTH) {
> -        fprintf(stderr, "%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
> +        VMWARE_VGA_DEBUG("%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
>           return false;
>       }
>       if (w < 0) {
> -        fprintf(stderr, "%s: w was < 0 (%d)\n", name, w);
> +        VMWARE_VGA_DEBUG("%s: w was < 0 (%d)\n", name, w);
>           return false;
>       }
>       if (w > SVGA_MAX_WIDTH) {
> -        fprintf(stderr, "%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
> +        VMWARE_VGA_DEBUG("%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
>           return false;
>       }
>       if (x + w > surface_width(surface)) {
> -        fprintf(stderr, "%s: width was > %d (x: %d, w: %d)\n",
> +        VMWARE_VGA_DEBUG("%s: width was > %d (x: %d, w: %d)\n",
>                   name, surface_width(surface), x, w);
>           return false;
>       }
>   
>       if (y < 0) {
> -        fprintf(stderr, "%s: y was < 0 (%d)\n", name, y);
> +        VMWARE_VGA_DEBUG("%s: y was < 0 (%d)\n", name, y);
>           return false;
>       }
>       if (y > SVGA_MAX_HEIGHT) {
> -        fprintf(stderr, "%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
> +        VMWARE_VGA_DEBUG("%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
>           return false;
>       }
>       if (h < 0) {
> -        fprintf(stderr, "%s: h was < 0 (%d)\n", name, h);
> +        VMWARE_VGA_DEBUG("%s: h was < 0 (%d)\n", name, h);
>           return false;
>       }
>       if (h > SVGA_MAX_HEIGHT) {
> -        fprintf(stderr, "%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
> +        VMWARE_VGA_DEBUG("%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
>           return false;
>       }
>       if (y + h > surface_height(surface)) {
> -        fprintf(stderr, "%s: update height > %d (y: %d, h: %d)\n",
> +        VMWARE_VGA_DEBUG("%s: update height > %d (y: %d, h: %d)\n",
>                   name, surface_height(surface), y, h);
>           return false;
>       }

Re: [PATCH 1/2] hw/display/vmware_vga: only show debug output if DEBUG enabled

[Carwyn Ellis]

Hey,

Thanks for getting back to me.

Yeah will take a look and update when I have a mo.

Cheers
Carwyn

> On 4 Jan 2022, at 09:18, Laurent Vivier <laurent@vivier.eu> wrote:
> 
> Le 04/01/2022 à 10:11, Carwyn Ellis a écrit :
>> Debug output was always being sent to STDERR. This has been replaced by
>> a define that will only show this output when DEBUG is set to true.
>> Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
>> ---
>>  hw/display/vmware_vga.c | 26 ++++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>> index e2969a6c81..8080e085d1 100644
>> --- a/hw/display/vmware_vga.c
>> +++ b/hw/display/vmware_vga.c
>> @@ -43,6 +43,12 @@
>>    /* See http://vmware-svga.sf.net/ for some documentation on VMWare SVGA */
>>  +#ifdef DEBUG
>> +#define VMWARE_VGA_DEBUG(...) { (void) fprintf(stdout, __VA_ARGS__); }
>> +#else
>> +#define VMWARE_VGA_DEBUG(...) ((void) 0)
>> +#endif
>> +
> 
> Could you replace this macro by adding some trace-events instead.
> 
> See https://qemu-project.gitlab.io/qemu/devel/tracing.html#using-trace-events
> 
> Thanks,
> Laurent
> 
>>  struct vmsvga_state_s {
>>      VGACommonState vga;
>>  @@ -297,45 +303,45 @@ static inline bool vmsvga_verify_rect(DisplaySurface *surface,
>>                                        int x, int y, int w, int h)
>>  {
>>      if (x < 0) {
>> -        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
>> +        VMWARE_VGA_DEBUG("%s: x was < 0 (%d)\n", name, x);
>>          return false;
>>      }
>>      if (x > SVGA_MAX_WIDTH) {
>> -        fprintf(stderr, "%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
>> +        VMWARE_VGA_DEBUG("%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
>>          return false;
>>      }
>>      if (w < 0) {
>> -        fprintf(stderr, "%s: w was < 0 (%d)\n", name, w);
>> +        VMWARE_VGA_DEBUG("%s: w was < 0 (%d)\n", name, w);
>>          return false;
>>      }
>>      if (w > SVGA_MAX_WIDTH) {
>> -        fprintf(stderr, "%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
>> +        VMWARE_VGA_DEBUG("%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
>>          return false;
>>      }
>>      if (x + w > surface_width(surface)) {
>> -        fprintf(stderr, "%s: width was > %d (x: %d, w: %d)\n",
>> +        VMWARE_VGA_DEBUG("%s: width was > %d (x: %d, w: %d)\n",
>>                  name, surface_width(surface), x, w);
>>          return false;
>>      }
>>        if (y < 0) {
>> -        fprintf(stderr, "%s: y was < 0 (%d)\n", name, y);
>> +        VMWARE_VGA_DEBUG("%s: y was < 0 (%d)\n", name, y);
>>          return false;
>>      }
>>      if (y > SVGA_MAX_HEIGHT) {
>> -        fprintf(stderr, "%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
>> +        VMWARE_VGA_DEBUG("%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
>>          return false;
>>      }
>>      if (h < 0) {
>> -        fprintf(stderr, "%s: h was < 0 (%d)\n", name, h);
>> +        VMWARE_VGA_DEBUG("%s: h was < 0 (%d)\n", name, h);
>>          return false;
>>      }
>>      if (h > SVGA_MAX_HEIGHT) {
>> -        fprintf(stderr, "%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
>> +        VMWARE_VGA_DEBUG("%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
>>          return false;
>>      }
>>      if (y + h > surface_height(surface)) {
>> -        fprintf(stderr, "%s: update height > %d (y: %d, h: %d)\n",
>> +        VMWARE_VGA_DEBUG("%s: update height > %d (y: %d, h: %d)\n",
>>                  name, surface_height(surface), y, h);
>>          return false;
>>      }
> 

Re: [PATCH 1/2] hw/display/vmware_vga: only show debug output if DEBUG enabled

[Laurent Vivier]

Le 04/01/2022 à 10:20, Carwyn Ellis a écrit :
> Hey,
> 
> Thanks for getting back to me.
> 
> Yeah will take a look and update when I have a mo.

It's really easy to do, see below for an example:

...
>>>   @@ -297,45 +303,45 @@ static inline bool vmsvga_verify_rect(DisplaySurface *surface,
>>>                                         int x, int y, int w, int h)
>>>   {
>>>       if (x < 0) {
>>> -        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
>>> +        VMWARE_VGA_DEBUG("%s: x was < 0 (%d)\n", name, x);

replace it by:

     trace_vmsvga_verify_rect_check_neg(name, x);

and in hw/display/trace-events you add:

     vmsvga_verify_rect_check_neg(const char *name, int x) "%s: x was < 0 (%d)"

Thanks,
Laurent

Re: [PATCH 1/2] hw/display/vmware_vga: only show debug output if DEBUG enabled

[Carwyn Ellis]

Ok cool.

Thanks for the info!

> On 4 Jan 2022, at 09:27, Laurent Vivier <laurent@vivier.eu> wrote:
> 
> Le 04/01/2022 à 10:20, Carwyn Ellis a écrit :
>> Hey,
>> Thanks for getting back to me.
>> Yeah will take a look and update when I have a mo.
> 
> It's really easy to do, see below for an example:
> 
> ...
>>>>  @@ -297,45 +303,45 @@ static inline bool vmsvga_verify_rect(DisplaySurface *surface,
>>>>                                        int x, int y, int w, int h)
>>>>  {
>>>>      if (x < 0) {
>>>> -        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
>>>> +        VMWARE_VGA_DEBUG("%s: x was < 0 (%d)\n", name, x);
> 
> replace it by:
> 
>    trace_vmsvga_verify_rect_check_neg(name, x);
> 
> and in hw/display/trace-events you add:
> 
>    vmsvga_verify_rect_check_neg(const char *name, int x) "%s: x was < 0 (%d)"
> 
> Thanks,
> Laurent

[PATCH 2/2] hw/display/vmware_vga: do not discard screen updates

[Carwyn Ellis]

In certain circumstances, typically when there is lots changing on the
screen, updates will be discarded resulting in garbled output.

This change firstly increases the screen update FIFO size to ensure it's
large enough to accomodate all updates deferred in a given screen
refresh cycle.

When updating the screen all updates are applied to ensure the display
output is rendered correctly.

Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
---
 hw/display/vmware_vga.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 8080e085d1..28556f39c6 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -82,7 +82,7 @@ struct vmsvga_state_s {
     uint32_t fifo_next;
     uint32_t fifo_stop;
 
-#define REDRAW_FIFO_LEN  512
+#define REDRAW_FIFO_LEN  8192
     struct vmsvga_rect_s {
         int x, y, w, h;
     } redraw_fifo[REDRAW_FIFO_LEN];
@@ -385,7 +385,14 @@ static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
 {
     struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
 
-    s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
+    if (s->redraw_fifo_last >= REDRAW_FIFO_LEN) {
+        VMWARE_VGA_DEBUG("%s: Discarding updates - FIFO length %d exceeded\n",
+            "vmsvga_update_rect_delayed",
+            REDRAW_FIFO_LEN
+        );
+        s->redraw_fifo_last = REDRAW_FIFO_LEN - 1;
+    }
+
     rect->x = x;
     rect->y = y;
     rect->w = w;
@@ -402,11 +409,13 @@ static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
     }
     /* Overlapping region updates can be optimised out here - if someone
      * knows a smart algorithm to do that, please share.  */
-    while (s->redraw_fifo_first != s->redraw_fifo_last) {
-        rect = &s->redraw_fifo[s->redraw_fifo_first++];
-        s->redraw_fifo_first &= REDRAW_FIFO_LEN - 1;
+    for (int i = 0; i < s->redraw_fifo_last; i++) {
+        rect = &s->redraw_fifo[i];
         vmsvga_update_rect(s, rect->x, rect->y, rect->w, rect->h);
     }
+
+    s->redraw_fifo_first = 0;
+    s->redraw_fifo_last = 0;
 }
 
 #ifdef HW_RECT_ACCEL
@@ -607,13 +616,14 @@ static inline uint32_t vmsvga_fifo_read(struct vmsvga_state_s *s)
 static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 {
     uint32_t cmd, colour;
-    int args, len, maxloop = 1024;
+    int args, len = 1024;
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
 
     len = vmsvga_fifo_length(s);
-    while (len > 0 && --maxloop > 0) {
+
+    while (len > 0) {
         /* May need to go back to the start of the command if incomplete */
         cmd_start = s->fifo_stop;
 
-- 
2.34.1

Re: [PATCH 2/2] hw/display/vmware_vga: do not discard screen updates

[Gerd Hoffmann]

  Hi,

> This change firstly increases the screen update FIFO size to ensure it's
> large enough to accomodate all updates deferred in a given screen
> refresh cycle.

How do you know it's large enough?

> @@ -385,7 +385,14 @@ static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
>  {
>      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
>  
> -    s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> +    if (s->redraw_fifo_last >= REDRAW_FIFO_LEN) {
> +        VMWARE_VGA_DEBUG("%s: Discarding updates - FIFO length %d exceeded\n",
> +            "vmsvga_update_rect_delayed",
> +            REDRAW_FIFO_LEN

Hmm, apparently you don't know ;)

How about just calling vmsvga_update_rect_flush()
when the fifo is (almost) full?

Which guest do you use btw?  I'm kind-of surprised this is still being
used even though it hasn't seen any development (beside fixing a bug now
and then) for a decade or so and the feature gap to recent vmware is
huge ...

take care,
  Gerd

Re: [PATCH 2/2] hw/display/vmware_vga: do not discard screen updates

[Carwyn Ellis]

> On 4 Jan 2022, at 12:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>> This change firstly increases the screen update FIFO size to ensure it's
>> large enough to accomodate all updates deferred in a given screen
>> refresh cycle.
> 
> How do you know it's large enough?
> 
>> @@ -385,7 +385,14 @@ static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
>> {
>>     struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
>> 
>> -    s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
>> +    if (s->redraw_fifo_last >= REDRAW_FIFO_LEN) {
>> +        VMWARE_VGA_DEBUG("%s: Discarding updates - FIFO length %d exceeded\n",
>> +            "vmsvga_update_rect_delayed",
>> +            REDRAW_FIFO_LEN
> 
> Hmm, apparently you don't know ;)
> 
> How about just calling vmsvga_update_rect_flush()
> when the fifo is (almost) full?

Yeah will give that a shot. Wasn’t sure how it’d play so did the simplest thing possible.

> 
> Which guest do you use btw?  I'm kind-of surprised this is still being
> used even though it hasn't seen any development (beside fixing a bug now
> and then) for a decade or so and the feature gap to recent vmware is
> huge ...
> 

This is an old vmware vm that rarely gets used. Figured I’d see if I could get it working over the holidays after making the move off an intel mac to m1, and noticed the issue with the output. In this case using the already configured vmware drivers was the least worst option.

> take care,
>  Gerd
> 

Cheers
Carwyn