[PATCH v3 0/2] use trace events and fix garbled output

[PATCH v3 0/2] use trace events and fix garbled output

[Carwyn Ellis]

This patchset supersedes earlier submissions and incorporates feedback
from Laurent Vivier, Gerd Hoffmann and Philippe Mathieu-Daudé.

There are two patches addressing the following in the vmware vga display
code

 - use of fprintf to log debug output to STDERR

   This has been replaced with trace events.

 - garbled display due to lost display updates

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

   The queue is now flushed when it reaches capacity.

   The code traversing the queue when updates are being applied to the
   display has also been simplified, since we always start the traversal
   at the beginning of the queue to ensure that all updates are applied.

Carwyn Ellis (2):
  hw/display/vmware_vga: replace fprintf calls with trace events
  hw/display/vmware_vga: do not discard screen updates

 hw/display/trace-events |  4 +++
 hw/display/vmware_vga.c | 71 ++++++++++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 30 deletions(-)

-- 
2.35.1

[PATCH v3 1/2] hw/display/vmware_vga: replace fprintf calls with trace events

[Carwyn Ellis]

Debug output was always being sent to STDERR.

This has been replaced with trace events.

Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
---
 hw/display/trace-events |  3 +++
 hw/display/vmware_vga.c | 30 ++++++++++++++++++------------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index 4a687d1b8e..91efc88f04 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -21,6 +21,9 @@ vmware_palette_write(uint32_t index, uint32_t value) "index %d, value 0x%x"
 vmware_scratch_read(uint32_t index, uint32_t value) "index %d, value 0x%x"
 vmware_scratch_write(uint32_t index, uint32_t value) "index %d, value 0x%x"
 vmware_setmode(uint32_t w, uint32_t h, uint32_t bpp) "%dx%d @ %d bpp"
+vmware_verify_rect_less_than_zero(const char *name, const char *param, int x) "%s: %s was < 0 (%d)"
+vmware_verify_rect_greater_than_bound(const char *name, const char *param, int bound, int x) "%s: %s was > %d (%d)"
+vmware_verify_rect_surface_bound_exceeded(const char *name, const char *component, int bound, const char *param1, int value1, const char *param2, int value2) "%s: %s > %d (%s: %d, %s: %d)"
 
 # virtio-gpu-base.c
 virtio_gpu_features(bool virgl) "virgl %d"
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index e2969a6c81..0cc43a1f15 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -297,46 +297,52 @@ 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);
+        trace_vmware_verify_rect_less_than_zero(name, "x", x);
         return false;
     }
     if (x > SVGA_MAX_WIDTH) {
-        fprintf(stderr, "%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
+        trace_vmware_verify_rect_greater_than_bound(name, "x", SVGA_MAX_WIDTH,
+                                                    x);
         return false;
     }
     if (w < 0) {
-        fprintf(stderr, "%s: w was < 0 (%d)\n", name, w);
+        trace_vmware_verify_rect_less_than_zero(name, "w", w);
         return false;
     }
     if (w > SVGA_MAX_WIDTH) {
-        fprintf(stderr, "%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
+        trace_vmware_verify_rect_greater_than_bound(name, "w", SVGA_MAX_WIDTH,
+                                                    w);
         return false;
     }
     if (x + w > surface_width(surface)) {
-        fprintf(stderr, "%s: width was > %d (x: %d, w: %d)\n",
-                name, surface_width(surface), x, w);
+        trace_vmware_verify_rect_surface_bound_exceeded(name, "width",
+                                                        surface_width(surface),
+                                                        "x", x, "w", w);
         return false;
     }
 
     if (y < 0) {
-        fprintf(stderr, "%s: y was < 0 (%d)\n", name, y);
+        trace_vmware_verify_rect_less_than_zero(name, "y", y);
         return false;
     }
     if (y > SVGA_MAX_HEIGHT) {
-        fprintf(stderr, "%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
+        trace_vmware_verify_rect_greater_than_bound(name, "y", SVGA_MAX_HEIGHT,
+                                                    y);
         return false;
     }
     if (h < 0) {
-        fprintf(stderr, "%s: h was < 0 (%d)\n", name, h);
+        trace_vmware_verify_rect_less_than_zero(name, "h", h);
         return false;
     }
     if (h > SVGA_MAX_HEIGHT) {
-        fprintf(stderr, "%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
+        trace_vmware_verify_rect_greater_than_bound(name, "y", SVGA_MAX_HEIGHT,
+                                                    y);
         return false;
     }
     if (y + h > surface_height(surface)) {
-        fprintf(stderr, "%s: update height > %d (y: %d, h: %d)\n",
-                name, surface_height(surface), y, h);
+        trace_vmware_verify_rect_surface_bound_exceeded(name, "height",
+                                                        surface_height(surface),
+                                                        "y", y, "h", h);
         return false;
     }
 
-- 
2.35.1

[PATCH v3 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 simplifies the traversal of the display update FIFO queue
when applying updates. We just track the queue length and iterate up to
the end of the queue.

Additionally when adding updates to the queue, if the buffer reaches
capacity we force a flush before accepting further events.

Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
---
 hw/display/trace-events |  1 +
 hw/display/vmware_vga.c | 41 +++++++++++++++++++++++------------------
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index 91efc88f04..0c0ffcbe42 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -24,6 +24,7 @@ vmware_setmode(uint32_t w, uint32_t h, uint32_t bpp) "%dx%d @ %d bpp"
 vmware_verify_rect_less_than_zero(const char *name, const char *param, int x) "%s: %s was < 0 (%d)"
 vmware_verify_rect_greater_than_bound(const char *name, const char *param, int bound, int x) "%s: %s was > %d (%d)"
 vmware_verify_rect_surface_bound_exceeded(const char *name, const char *component, int bound, const char *param1, int value1, const char *param2, int value2) "%s: %s > %d (%s: %d, %s: %d)"
+vmware_update_rect_delayed_flush(void) "display update FIFO full - forcing flush"
 
 # virtio-gpu-base.c
 virtio_gpu_features(bool virgl) "virgl %d"
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0cc43a1f15..8a3c3cb8f0 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -80,7 +80,7 @@ struct vmsvga_state_s {
     struct vmsvga_rect_s {
         int x, y, w, h;
     } redraw_fifo[REDRAW_FIFO_LEN];
-    int redraw_fifo_first, redraw_fifo_last;
+    int redraw_fifo_last;
 };
 
 #define TYPE_VMWARE_SVGA "vmware-svga"
@@ -380,33 +380,39 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
     dpy_gfx_update(s->vga.con, x, y, w, h);
 }
 
-static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
-                int x, int y, int w, int h)
-{
-    struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
-
-    s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
-    rect->x = x;
-    rect->y = y;
-    rect->w = w;
-    rect->h = h;
-}
-
 static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
 {
     struct vmsvga_rect_s *rect;
 
     if (s->invalidated) {
-        s->redraw_fifo_first = s->redraw_fifo_last;
+        s->redraw_fifo_last = 0;
         return;
     }
     /* 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_last = 0;
+}
+
+static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
+                int x, int y, int w, int h)
+{
+
+    if (s->redraw_fifo_last >= REDRAW_FIFO_LEN) {
+        trace_vmware_update_rect_delayed_flush();
+        vmsvga_update_rect_flush(s);
+    }
+
+    struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
+
+    rect->x = x;
+    rect->y = y;
+    rect->w = w;
+    rect->h = h;
 }
 
 #ifdef HW_RECT_ACCEL
@@ -1159,7 +1165,6 @@ static void vmsvga_reset(DeviceState *dev)
     s->config = 0;
     s->svgaid = SVGA_ID;
     s->cursor.on = 0;
-    s->redraw_fifo_first = 0;
     s->redraw_fifo_last = 0;
     s->syncing = 0;
 
-- 
2.35.1

Re: [PATCH v3 1/2] hw/display/vmware_vga: replace fprintf calls with trace events

[Philippe Mathieu-Daudé via]

On 6/2/22 19:39, Carwyn Ellis wrote:
> Debug output was always being sent to STDERR.
> 
> This has been replaced with trace events.
> 
> Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
> ---
>   hw/display/trace-events |  3 +++
>   hw/display/vmware_vga.c | 30 ++++++++++++++++++------------
>   2 files changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Carwyn Ellis]

ping

https://patchew.org/QEMU/20220206183956.10694-1-carwynellis@gmail.com/20220206183956.10694-3-carwynellis@gmail.com/

Originally submitted as one of two patches, the first patch to use trace events has been merged, however the patch that fixes garbled output hasn’t been reviewed yet.

Do let me know if you think there’s anything else that needs changing here and I’ll resubmit if so. FWIW I’ve been using this fix for a couple of months now without any issues.

Thanks
Carwyn

> On 6 Feb 2022, at 18:39, Carwyn Ellis <carwynellis@gmail.com> wrote:
> 
> In certain circumstances, typically when there is lots changing on the
> screen, updates will be discarded resulting in garbled output.
> 
> This change simplifies the traversal of the display update FIFO queue
> when applying updates. We just track the queue length and iterate up to
> the end of the queue.
> 
> Additionally when adding updates to the queue, if the buffer reaches
> capacity we force a flush before accepting further events.
> 
> Signed-off-by: Carwyn Ellis <carwynellis@gmail.com>
> ---
> hw/display/trace-events |  1 +
> hw/display/vmware_vga.c | 41 +++++++++++++++++++++++------------------
> 2 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 91efc88f04..0c0ffcbe42 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -24,6 +24,7 @@ vmware_setmode(uint32_t w, uint32_t h, uint32_t bpp) "%dx%d @ %d bpp"
> vmware_verify_rect_less_than_zero(const char *name, const char *param, int x) "%s: %s was < 0 (%d)"
> vmware_verify_rect_greater_than_bound(const char *name, const char *param, int bound, int x) "%s: %s was > %d (%d)"
> vmware_verify_rect_surface_bound_exceeded(const char *name, const char *component, int bound, const char *param1, int value1, const char *param2, int value2) "%s: %s > %d (%s: %d, %s: %d)"
> +vmware_update_rect_delayed_flush(void) "display update FIFO full - forcing flush"
> 
> # virtio-gpu-base.c
> virtio_gpu_features(bool virgl) "virgl %d"
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 0cc43a1f15..8a3c3cb8f0 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -80,7 +80,7 @@ struct vmsvga_state_s {
>     struct vmsvga_rect_s {
>         int x, y, w, h;
>     } redraw_fifo[REDRAW_FIFO_LEN];
> -    int redraw_fifo_first, redraw_fifo_last;
> +    int redraw_fifo_last;
> };
> 
> #define TYPE_VMWARE_SVGA "vmware-svga"
> @@ -380,33 +380,39 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
>     dpy_gfx_update(s->vga.con, x, y, w, h);
> }
> 
> -static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
> -                int x, int y, int w, int h)
> -{
> -    struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
> -
> -    s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> -    rect->x = x;
> -    rect->y = y;
> -    rect->w = w;
> -    rect->h = h;
> -}
> -
> static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
> {
>     struct vmsvga_rect_s *rect;
> 
>     if (s->invalidated) {
> -        s->redraw_fifo_first = s->redraw_fifo_last;
> +        s->redraw_fifo_last = 0;
>         return;
>     }
>     /* 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_last = 0;
> +}
> +
> +static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
> +                int x, int y, int w, int h)
> +{
> +
> +    if (s->redraw_fifo_last >= REDRAW_FIFO_LEN) {
> +        trace_vmware_update_rect_delayed_flush();
> +        vmsvga_update_rect_flush(s);
> +    }
> +
> +    struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last++];
> +
> +    rect->x = x;
> +    rect->y = y;
> +    rect->w = w;
> +    rect->h = h;
> }
> 
> #ifdef HW_RECT_ACCEL
> @@ -1159,7 +1165,6 @@ static void vmsvga_reset(DeviceState *dev)
>     s->config = 0;
>     s->svgaid = SVGA_ID;
>     s->cursor.on = 0;
> -    s->redraw_fifo_first = 0;
>     s->redraw_fifo_last = 0;
>     s->syncing = 0;
> 
> -- 
> 2.35.1
> 

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

[Gerd Hoffmann]

On Sun, Apr 10, 2022 at 05:49:30PM +0100, Carwyn Ellis wrote:
> ping
> 
> https://patchew.org/QEMU/20220206183956.10694-1-carwynellis@gmail.com/20220206183956.10694-3-carwynellis@gmail.com/
> 
> Originally submitted as one of two patches, the first patch to use trace events has been merged, however the patch that fixes garbled output hasn’t been reviewed yet.

Hmm, slipped through for some reason, IIRC I cherry-picked the trace
events patch from v2 series and probably simply missed v3.  Queued now.

thanks,
  Gerd

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

[Carwyn Ellis]

Awesome! Thanks! 

> On 22 Apr 2022, at 11:40, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> On Sun, Apr 10, 2022 at 05:49:30PM +0100, Carwyn Ellis wrote:
>> ping
>> 
>> https://patchew.org/QEMU/20220206183956.10694-1-carwynellis@gmail.com/20220206183956.10694-3-carwynellis@gmail.com/
>> 
>> Originally submitted as one of two patches, the first patch to use trace events has been merged, however the patch that fixes garbled output hasn’t been reviewed yet.
> 
> Hmm, slipped through for some reason, IIRC I cherry-picked the trace
> events patch from v2 series and probably simply missed v3.  Queued now.
> 
> thanks,
>  Gerd
>