[Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Gerd Hoffmann]

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

  Hi,

When using a shared display surface buffer some of the optimizations
done by the vnc server code don't work.  In shared buffer mode the guest
may update the screen while the vnc server looks at the framebuffer.
That in turn makes some code racy, the dirty bitmap walk through for
example, leading to screen corruption.  Right now this is visible with
xenfb only.  I expect simliar issues will show up for vga too once we
run the vcpus in threads.

please apply
  Gerd

[-- Attachment #2: 0001-vnc-shared-buffer-skip-some-optimizations.patch --]
[-- Type: text/plain, Size: 4346 bytes --]

>From eaff07da03b93fa59d6330a7e5e0e720d4573f3c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 11 Mar 2009 15:30:23 +0100
Subject: [PATCH] vnc: shared buffer: skip some optimizations.

When using a shared display surface buffer some of the optimizations
done by the vnc server code don't work.  In shared buffer mode the guest
may update the screen while the vnc server looks at the framebuffer.
That in turn makes some code racy, the dirty bitmap walk through for
example, leading to screen corruption.  Right now this is visible with
xenfb only.  I expect simliar issues will show up for vga too once we
run the vcpus in threads.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vnc.c |   78 +++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/vnc.c b/vnc.c
index 81c842a..66f8946 100644
--- a/vnc.c
+++ b/vnc.c
@@ -649,7 +649,8 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
     VncDisplay *vd = ds->opaque;
     VncState *vs = vd->clients;
     while (vs != NULL) {
-        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT))
+        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT) &&
+            !is_buffer_shared(ds->surface))
             vnc_copy(vs, src_x, src_y, dst_x, dst_y, w, h);
         else /* TODO */
             vnc_update(vs, dst_x, dst_y, w, h);
@@ -688,36 +689,51 @@ static void vnc_update_client(void *opaque)
 
         vnc_set_bits(width_mask, (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
 
-	/* Walk through the dirty map and eliminate tiles that
-	   really aren't dirty */
-	row = ds_get_data(vs->ds);
-	old_row = vs->old_data;
-
-	for (y = 0; y < ds_get_height(vs->ds); y++) {
-	    if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
-		int x;
-		uint8_t *ptr;
-		char *old_ptr;
-
-		ptr = row;
-		old_ptr = (char*)old_row;
-
-		for (x = 0; x < ds_get_width(vs->ds); x += 16) {
-		    if (memcmp(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds)) == 0) {
-			vnc_clear_bit(vs->dirty_row[y], (x / 16));
-		    } else {
-			has_dirty = 1;
-			memcpy(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds));
-		    }
-
-		    ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
-		    old_ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
-		}
-	    }
+        if (!is_buffer_shared(vs->ds->surface)) {
+            /* Walk through the dirty map and eliminate tiles that
+               really aren't dirty */
+            row = ds_get_data(vs->ds);
+            old_row = vs->old_data;
+
+            for (y = 0; y < ds_get_height(vs->ds); y++) {
+                if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
+                    int x;
+                    uint8_t *ptr;
+                    char *old_ptr;
+
+                    ptr = row;
+                    old_ptr = (char*)old_row;
+
+                    for (x = 0; x < ds_get_width(vs->ds); x += 16) {
+                        if (memcmp(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds)) == 0) {
+                            vnc_clear_bit(vs->dirty_row[y], (x / 16));
+                        } else {
+                            has_dirty = 1;
+                            memcpy(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds));
+                        }
+
+                        ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
+                        old_ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
+                    }
+                }
 
-	    row += ds_get_linesize(vs->ds);
-	    old_row += ds_get_linesize(vs->ds);
-	}
+                row += ds_get_linesize(vs->ds);
+                old_row += ds_get_linesize(vs->ds);
+            }
+        } else {
+            /*
+             * shared buffer:
+             *   -> verifying the dirty map is racy as the guest may
+             *      update the screen while we are looking at it
+             *   -> skip the check
+             */
+            for (y = 0; y < ds_get_height(vs->ds); y++) {
+                if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
+                    has_dirty = 1;
+                    break;
+                }
+            }
+        }
 
 	if (!has_dirty && !vs->audio_cap) {
 	    qemu_mod_timer(vs->timer, qemu_get_clock(rt_clock) + VNC_REFRESH_INTERVAL);
-- 
1.6.1.3

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Anthony Liguori]

Gerd Hoffmann wrote:
>   Hi,
>
> When using a shared display surface buffer some of the optimizations
> done by the vnc server code don't work.  In shared buffer mode the guest
> may update the screen while the vnc server looks at the framebuffer.
> That in turn makes some code racy, the dirty bitmap walk through for
> example, leading to screen corruption.  Right now this is visible with
> xenfb only.  I expect simliar issues will show up for vga too once we
> run the vcpus in threads.
>   

:-/  I'd rather disable the shared buffers.

The minimization optimization is extremely important for VNC 
performance.  It's a much bigger win than the memory copy that you lose 
when using shared buffers.

Regards,

Anthony Liguori

> please apply
>   Gerd
>   

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Gerd Hoffmann]

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

Anthony Liguori wrote:
> 
> :-/  I'd rather disable the shared buffers.
> 
> The minimization optimization is extremely important for VNC
> performance.  It's a much bigger win than the memory copy that you lose
> when using shared buffers.

Much improved version.  Not polished yet, to be done next week (will be
offline tomorrow + weekend).

Concept: The patch clearly separates between the guest visible surface
and the vnc server surface.  They both have a separate dirty bitmap.

Guest updates will go to the guest surface and will be marked in the
guest dirty map.

Server surface will be updated from the guest surface using the guest
dirty bitmap.  Only areas which are really updates will be tagged in the
server dirty map as such.  That is *not* an extra copy because the copy
was done before as well for book-keeping (to old_data).

Sending screen updates to the client will be done using the server
surface + dirty map only, so the guest updating the screen in parallel
can't cause trouble here.

The separate dirty bitmap also has the nice effect that forced screen
updates can be done cleanly by simply tagging the area in both guest and
server dirty map.  The old, hackish way was memset(old_data, 42, size)
to trick the code checking for screen changes.

enjoy,
  Gerd

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 16654 bytes --]

diff --git a/vnc.c b/vnc.c
index a67d23f..4945ab9 100644
--- a/vnc.c
+++ b/vnc.c
@@ -260,6 +260,7 @@ static inline int vnc_and_bits(const uint32_t *d1, const uint32_t *d2,
 
 static void vnc_update(VncState *vs, int x, int y, int w, int h)
 {
+    struct VncSurface *s = &vs->guest;
     int i;
 
     h += y;
@@ -271,14 +272,14 @@ static void vnc_update(VncState *vs, int x, int y, int w, int h)
     w += (x % 16);
     x -= (x % 16);
 
-    x = MIN(x, vs->serverds.width);
-    y = MIN(y, vs->serverds.height);
-    w = MIN(x + w, vs->serverds.width) - x;
-    h = MIN(h, vs->serverds.height);
+    x = MIN(x, s->ds.width);
+    y = MIN(y, s->ds.height);
+    w = MIN(x + w, s->ds.width) - x;
+    h = MIN(h, s->ds.height);
 
     for (; y < h; y++)
         for (i = 0; i < w; i += 16)
-            vnc_set_bit(vs->dirty_row[y], (x + i) / 16);
+            vnc_set_bit(s->dirty[y], (x + i) / 16);
 }
 
 static void vnc_dpy_update(DisplayState *ds, int x, int y, int w, int h)
@@ -338,22 +339,15 @@ void buffer_append(Buffer *buffer, const void *data, size_t len)
 static void vnc_resize(VncState *vs)
 {
     DisplayState *ds = vs->ds;
-
     int size_changed;
 
-    vs->old_data = qemu_realloc(vs->old_data, ds_get_linesize(ds) * ds_get_height(ds));
-
-    if (vs->old_data == NULL) {
-        fprintf(stderr, "vnc: memory allocation failed\n");
-        exit(1);
-    }
-
-    if (ds_get_bytes_per_pixel(ds) != vs->serverds.pf.bytes_per_pixel)
+    /* guest surface */
+    if (ds_get_bytes_per_pixel(ds) != vs->guest.ds.pf.bytes_per_pixel)
         console_color_init(ds);
     vnc_colordepth(vs);
-    size_changed = ds_get_width(ds) != vs->serverds.width ||
-                   ds_get_height(ds) != vs->serverds.height;
-    vs->serverds = *(ds->surface);
+    size_changed = ds_get_width(ds) != vs->guest.ds.width ||
+                   ds_get_height(ds) != vs->guest.ds.height;
+    vs->guest.ds = *(ds->surface);
     if (size_changed) {
         if (vs->csock != -1 && vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
             vnc_write_u8(vs, 0);  /* msg id */
@@ -364,9 +358,18 @@ static void vnc_resize(VncState *vs)
             vnc_flush(vs);
         }
     }
-
-    memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
-    memset(vs->old_data, 42, ds_get_linesize(vs->ds) * ds_get_height(vs->ds));
+    memset(vs->guest.dirty, 0xFF, sizeof(vs->guest.dirty));
+
+    /* server surface */
+    qemu_resize_displaysurface(&vs->server.ds,
+                               ds_get_width(ds), ds_get_height(ds),
+                               ds_get_bits_per_pixel(ds),
+                               ds_get_linesize(ds));
+    if (vs->server.ds.data == NULL) {
+        fprintf(stderr, "vnc: memory allocation failed\n");
+        exit(1);
+    }
+    memset(vs->server.dirty, 0xFF, sizeof(vs->guest.dirty));
 }
 
 static void vnc_dpy_resize(DisplayState *ds)
@@ -390,12 +393,12 @@ static void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
 {
     uint8_t r, g, b;
 
-    r = ((((v & vs->serverds.pf.rmask) >> vs->serverds.pf.rshift) << vs->clientds.pf.rbits) >>
-        vs->serverds.pf.rbits);
-    g = ((((v & vs->serverds.pf.gmask) >> vs->serverds.pf.gshift) << vs->clientds.pf.gbits) >>
-        vs->serverds.pf.gbits);
-    b = ((((v & vs->serverds.pf.bmask) >> vs->serverds.pf.bshift) << vs->clientds.pf.bbits) >>
-        vs->serverds.pf.bbits);
+    r = ((((v & vs->server.ds.pf.rmask) >> vs->server.ds.pf.rshift) << vs->clientds.pf.rbits) >>
+        vs->server.ds.pf.rbits);
+    g = ((((v & vs->server.ds.pf.gmask) >> vs->server.ds.pf.gshift) << vs->clientds.pf.gbits) >>
+        vs->server.ds.pf.gbits);
+    b = ((((v & vs->server.ds.pf.bmask) >> vs->server.ds.pf.bshift) << vs->clientds.pf.bbits) >>
+        vs->server.ds.pf.bbits);
     v = (r << vs->clientds.pf.rshift) |
         (g << vs->clientds.pf.gshift) |
         (b << vs->clientds.pf.bshift);
@@ -433,7 +436,7 @@ static void vnc_write_pixels_generic(VncState *vs, void *pixels1, int size)
 {
     uint8_t buf[4];
 
-    if (vs->serverds.pf.bytes_per_pixel == 4) {
+    if (vs->server.ds.pf.bytes_per_pixel == 4) {
         uint32_t *pixels = pixels1;
         int n, i;
         n = size >> 2;
@@ -441,7 +444,7 @@ static void vnc_write_pixels_generic(VncState *vs, void *pixels1, int size)
             vnc_convert_pixel(vs, buf, pixels[i]);
             vnc_write(vs, buf, vs->clientds.pf.bytes_per_pixel);
         }
-    } else if (vs->serverds.pf.bytes_per_pixel == 2) {
+    } else if (vs->server.ds.pf.bytes_per_pixel == 2) {
         uint16_t *pixels = pixels1;
         int n, i;
         n = size >> 1;
@@ -449,7 +452,7 @@ static void vnc_write_pixels_generic(VncState *vs, void *pixels1, int size)
             vnc_convert_pixel(vs, buf, pixels[i]);
             vnc_write(vs, buf, vs->clientds.pf.bytes_per_pixel);
         }
-    } else if (vs->serverds.pf.bytes_per_pixel == 1) {
+    } else if (vs->server.ds.pf.bytes_per_pixel == 1) {
         uint8_t *pixels = pixels1;
         int n, i;
         n = size;
@@ -467,7 +470,7 @@ static void send_framebuffer_update_raw(VncState *vs, int x, int y, int w, int h
     int i;
     uint8_t *row;
 
-    row = ds_get_data(vs->ds) + y * ds_get_linesize(vs->ds) + x * ds_get_bytes_per_pixel(vs->ds);
+    row = vs->server.ds.data + y * ds_get_linesize(vs->ds) + x * ds_get_bytes_per_pixel(vs->ds);
     for (i = 0; i < h; i++) {
         vs->write_pixels(vs, row, w * ds_get_bytes_per_pixel(vs->ds));
         row += ds_get_linesize(vs->ds);
@@ -516,8 +519,8 @@ static void send_framebuffer_update_hextile(VncState *vs, int x, int y, int w, i
     int has_fg, has_bg;
     uint8_t *last_fg, *last_bg;
 
-    last_fg = (uint8_t *) qemu_malloc(vs->serverds.pf.bytes_per_pixel);
-    last_bg = (uint8_t *) qemu_malloc(vs->serverds.pf.bytes_per_pixel);
+    last_fg = (uint8_t *) qemu_malloc(vs->server.ds.pf.bytes_per_pixel);
+    last_bg = (uint8_t *) qemu_malloc(vs->server.ds.pf.bytes_per_pixel);
     has_fg = has_bg = 0;
     for (j = y; j < (y + h); j += 16) {
         for (i = x; i < (x + w); i += 16) {
@@ -670,16 +673,17 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
     }
 }
 
-static int find_dirty_height(VncState *vs, int y, int last_x, int x)
+static int find_and_clear_dirty_height(struct VncSurface *s,
+                                       int y, int last_x, int x)
 {
     int h;
 
-    for (h = 1; h < (vs->serverds.height - y); h++) {
+    for (h = 1; h < (s->ds.height - y) && h < 1; h++) {
         int tmp_x;
-        if (!vnc_get_bit(vs->dirty_row[y + h], last_x))
+        if (!vnc_get_bit(s->dirty[y + h], last_x))
             break;
         for (tmp_x = last_x; tmp_x < x; tmp_x++)
-            vnc_clear_bit(vs->dirty_row[y + h], tmp_x);
+            vnc_clear_bit(s->dirty[y + h], tmp_x);
     }
 
     return h;
@@ -690,8 +694,9 @@ static void vnc_update_client(void *opaque)
     VncState *vs = opaque;
     if (vs->need_update && vs->csock != -1) {
         int y;
-        uint8_t *row;
-        char *old_row;
+        uint8_t *guest_row;
+        uint8_t *server_row;
+        int cmp_bytes = 16 * ds_get_bytes_per_pixel(vs->ds);
         uint32_t width_mask[VNC_DIRTY_WORDS];
         int n_rectangles;
         int saved_offset;
@@ -699,37 +704,42 @@ static void vnc_update_client(void *opaque)
 
         vga_hw_update();
 
-        vnc_set_bits(width_mask, (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
-
-        /* Walk through the dirty map and eliminate tiles that
-           really aren't dirty */
-        row = ds_get_data(vs->ds);
-        old_row = vs->old_data;
+        if (vs->output.offset) {
+            qemu_mod_timer(vs->timer, qemu_get_clock(rt_clock) + VNC_REFRESH_INTERVAL);
+            return;
+        }
 
-        for (y = 0; y < ds_get_height(vs->ds); y++) {
-            if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
+        /*
+         * Walk through the guest dirty map.
+         * Check and copy modified bits from guest to server surface.
+         * Update server dirty map.
+         */
+        vnc_set_bits(width_mask, (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
+        guest_row  = vs->guest.ds.data;
+        server_row = vs->server.ds.data;
+        for (y = 0; y < vs->guest.ds.height; y++) {
+            if (vnc_and_bits(vs->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
                 int x;
-                uint8_t *ptr;
-                char *old_ptr;
-
-                ptr = row;
-                old_ptr = (char*)old_row;
-
-                for (x = 0; x < ds_get_width(vs->ds); x += 16) {
-                    if (memcmp(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds)) == 0) {
-                        vnc_clear_bit(vs->dirty_row[y], (x / 16));
-                    } else {
-                        has_dirty = 1;
-                        memcpy(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds));
-                    }
-
-                    ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
-                    old_ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
+                uint8_t *guest_ptr;
+                uint8_t *server_ptr;
+
+                guest_ptr  = guest_row;
+                server_ptr = server_row;
+
+                for (x = 0; x < vs->guest.ds.width;
+                     x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
+                    if (!vnc_get_bit(vs->guest.dirty[y], (x / 16)))
+                        continue;
+                    vnc_clear_bit(vs->guest.dirty[y], (x / 16));
+                    if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0)
+                        continue;
+                    memcpy(server_ptr, guest_ptr, cmp_bytes);
+                    vnc_set_bit(vs->server.dirty[y], (x / 16));
+                    has_dirty++;
                 }
             }
-
-            row += ds_get_linesize(vs->ds);
-            old_row += ds_get_linesize(vs->ds);
+            guest_row  += ds_get_linesize(vs->ds);
+            server_row += ds_get_linesize(vs->ds);
         }
 
         if (!has_dirty && !vs->audio_cap) {
@@ -744,18 +754,18 @@ static void vnc_update_client(void *opaque)
         saved_offset = vs->output.offset;
         vnc_write_u16(vs, 0);
 
-        for (y = 0; y < vs->serverds.height; y++) {
+        for (y = 0; y < vs->server.ds.height; y++) {
             int x;
             int last_x = -1;
-            for (x = 0; x < vs->serverds.width / 16; x++) {
-                if (vnc_get_bit(vs->dirty_row[y], x)) {
+            for (x = 0; x < vs->server.ds.width / 16; x++) {
+                if (vnc_get_bit(vs->server.dirty[y], x)) {
                     if (last_x == -1) {
                         last_x = x;
                     }
-                    vnc_clear_bit(vs->dirty_row[y], x);
+                    vnc_clear_bit(vs->server.dirty[y], x);
                 } else {
                     if (last_x != -1) {
-                        int h = find_dirty_height(vs, y, last_x, x);
+                        int h = find_and_clear_dirty_height(&vs->server, y, last_x, x);
                         send_framebuffer_update(vs, last_x * 16, y, (x - last_x) * 16, h);
                         n_rectangles++;
                     }
@@ -763,7 +773,7 @@ static void vnc_update_client(void *opaque)
                 }
             }
             if (last_x != -1) {
-                int h = find_dirty_height(vs, y, last_x, x);
+                int h = find_and_clear_dirty_height(&vs->server, y, last_x, x);
                 send_framebuffer_update(vs, last_x * 16, y, (x - last_x) * 16, h);
                 n_rectangles++;
             }
@@ -892,9 +902,9 @@ int vnc_client_io_error(VncState *vs, int ret, int last_errno)
         if (!vs->vd->clients)
             dcl->idle = 1;
 
-        qemu_free(vs->old_data);
+        qemu_free(vs->server.ds.data);
         qemu_free(vs);
-  
+
         return 0;
     }
     return ret;
@@ -938,7 +948,7 @@ long vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
     } else
 #endif /* CONFIG_VNC_TLS */
         ret = send(vs->csock, data, datalen, 0);
-    VNC_DEBUG("Wrote wire %p %d -> %ld\n", data, datalen, ret);
+    VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
     return vnc_client_io_error(vs, ret, socket_error());
 }
 
@@ -958,7 +968,7 @@ static long vnc_client_write_plain(VncState *vs)
     long ret;
 
 #ifdef CONFIG_VNC_SASL
-    VNC_DEBUG("Write Plain: Pending output %p size %d offset %d. Wait SSF %d\n",
+    VNC_DEBUG("Write Plain: Pending output %p size %zd offset %zd. Wait SSF %d\n",
               vs->output.buffer, vs->output.capacity, vs->output.offset,
               vs->sasl.waitWriteSSF);
 
@@ -1043,7 +1053,7 @@ long vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
     } else
 #endif /* CONFIG_VNC_TLS */
         ret = recv(vs->csock, data, datalen, 0);
-    VNC_DEBUG("Read wire %p %d -> %ld\n", data, datalen, ret);
+    VNC_DEBUG("Read wire %p %zd -> %ld\n", data, datalen, ret);
     return vnc_client_io_error(vs, ret, socket_error());
 }
 
@@ -1059,7 +1069,7 @@ long vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
 static long vnc_client_read_plain(VncState *vs)
 {
     int ret;
-    VNC_DEBUG("Read plain %p size %d offset %d\n",
+    VNC_DEBUG("Read plain %p size %zd offset %zd\n",
               vs->input.buffer, vs->input.capacity, vs->input.offset);
     buffer_reserve(&vs->input, 4096);
     ret = vnc_client_read_buf(vs, buffer_end(&vs->input), 4096);
@@ -1389,13 +1399,11 @@ static void framebuffer_update_request(VncState *vs, int incremental,
     int i;
     vs->need_update = 1;
     if (!incremental) {
-        char *old_row = vs->old_data + y_position * ds_get_linesize(vs->ds);
-
         for (i = 0; i < h; i++) {
-            vnc_set_bits(vs->dirty_row[y_position + i],
+            vnc_set_bits(vs->guest.dirty[y_position + i],
+                         (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
+            vnc_set_bits(vs->server.dirty[y_position + i],
                          (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
-            memset(old_row, 42, ds_get_width(vs->ds) * ds_get_bytes_per_pixel(vs->ds));
-            old_row += ds_get_linesize(vs->ds);
         }
     }
 }
@@ -1523,7 +1531,7 @@ static void set_pixel_format(VncState *vs,
         return;
     }
 
-    vs->clientds = vs->serverds;
+    vs->clientds = vs->server.ds;
     vs->clientds.pf.rmax = red_max;
     count_bits(vs->clientds.pf.rbits, red_max);
     vs->clientds.pf.rshift = red_shift;
@@ -1977,8 +1985,6 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vnc_write(vs, "RFB 003.008\n", 12);
     vnc_flush(vs);
     vnc_read_when(vs, protocol_version, 12);
-    memset(vs->old_data, 0, ds_get_linesize(vs->ds) * ds_get_height(vs->ds));
-    memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
     vnc_update_client(vs);
     reset_keys(vs);
 
diff --git a/vnc.h b/vnc.h
index 8b6bc5e..7672175 100644
--- a/vnc.h
+++ b/vnc.h
@@ -35,7 +35,7 @@
 
 #include "keymaps.h"
 
-// #define _VNC_DEBUG 1
+#define _VNC_DEBUG 1
 
 #ifdef _VNC_DEBUG
 #define VNC_DEBUG(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
@@ -104,6 +104,12 @@ struct VncDisplay
 #endif
 };
 
+struct VncSurface
+{
+    uint32_t dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
+    DisplaySurface ds;
+};
+
 struct VncState
 {
     QEMUTimer *timer;
@@ -111,8 +117,6 @@ struct VncState
     DisplayState *ds;
     VncDisplay *vd;
     int need_update;
-    uint32_t dirty_row[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
-    char *old_data;
     uint32_t features;
     int absolute;
     int last_x;
@@ -138,7 +142,9 @@ struct VncState
     /* current output mode information */
     VncWritePixels *write_pixels;
     VncSendHextileTile *send_hextile_tile;
-    DisplaySurface clientds, serverds;
+    DisplaySurface clientds;
+    struct VncSurface server;
+    struct VncSurface guest;
 
     CaptureVoiceOut *audio_cap;
     struct audsettings as;
diff --git a/vnchextile.h b/vnchextile.h
index f3fdfb4..c5ac18c 100644
--- a/vnchextile.h
+++ b/vnchextile.h
@@ -13,7 +13,7 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
                                              void *last_fg_,
                                              int *has_bg, int *has_fg)
 {
-    uint8_t *row = (ds_get_data(vs->ds) + y * ds_get_linesize(vs->ds) + x * ds_get_bytes_per_pixel(vs->ds));
+    uint8_t *row = vs->server.ds.data + y * ds_get_linesize(vs->ds) + x * ds_get_bytes_per_pixel(vs->ds);
     pixel_t *irow = (pixel_t *)row;
     int j, i;
     pixel_t *last_bg = (pixel_t *)last_bg_;

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Stefano Stabellini]

Gerd Hoffmann wrote:

> Sending screen updates to the client will be done using the server
> surface + dirty map only, so the guest updating the screen in parallel
> can't cause trouble here.

Could you please explain an example of trouble caused by sich situations?

Given A, B and C three different points in time with A < B < C,
currently what we are doing is:

A) getting the guest dirty map

B) filtering the guest dirty map using memcmp with the server surface
(called old_data in the current code), however the dirty map could be
inconsistent with the framebuffer because was set in A

C) sending updates to the client from the guest surface based on the
dirty map, however the dirty map is inconsistent because it was set in A
and filtered in B

In any case we get the new dirty map next iteration and it will include
all the possible changes that happened after A, possibly already sent to
the client inadvertently in C.

---

with your patch we have:

A) getting the guest dirty map

B) setting the server dirty map using the guest dirty map and memcmp
with the server surface, however the dirty map could be inconsistent
with the guest framebuffer because was set in A

C) sending updates to the client from the server surface

In any case we get the new dirty map next iteration and it will include
all the possible changes that happened after A, possibly already sent to
the client inadvertently in C.

---

If your goal is to remove possible update incosistencies, your patch
certainly removes the one in C, but not the one in B.

If your goal is to send the most updated version of the framebuffer to
the guest ASAP, the current code is better at it.

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Gerd Hoffmann]

Stefano Stabellini wrote:
> Gerd Hoffmann wrote:
> 
>> Sending screen updates to the client will be done using the server
>> surface + dirty map only, so the guest updating the screen in parallel
>> can't cause trouble here.
> 
> Could you please explain an example of trouble caused by sich situations?

Sure.

> Given A, B and C three different points in time with A < B < C,
> currently what we are doing is:
> 
> A) getting the guest dirty map
> 
> B) filtering the guest dirty map using memcmp with the server surface
> (called old_data in the current code), however the dirty map could be
> inconsistent with the framebuffer because was set in A
> 
> C) sending updates to the client from the guest surface based on the
> dirty map, however the dirty map is inconsistent because it was set in A
> and filtered in B

The dirty bitmap inconsistency isn't a problem at all.  The next update
round will fix that.

The trouble spot is here:  The guest might have updated the screen
between (b) and (c).  This will cause the vnc clients view of the screen
content become inconsistent with old_data.  On the next update round (b)
will stop working correctly due to that.

What my patch does in terms of the old code is make step (c) use
old_data instead of the guest-visible framebuffer.  I didn't like
stacking hacks on hacks like this though, so I decided to explicitly
name the two surfaces.

You still could get away with just one dirty bitmap for both surfaces.
Having two bitmaps IMHO makes the code more readable though.  It also
allows killing the memset(old_data,42,size) hack for forced client
updates (see framebuffer_update_request()).  An -- in preparation for
the future -- it makes the vnc code a bit more thread-friendly.  Guest
screen updating (aka vnc_update()) can run in parallel with encoding
updates for the guest (i.e. step (c)).

> If your goal is to send the most updated version of the framebuffer to
> the guest ASAP, the current code is better at it.

But *that* is exactly where the bug is ;)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Stefano Stabellini]

Gerd Hoffmann wrote:

> The trouble spot is here:  The guest might have updated the screen
> between (b) and (c).  This will cause the vnc clients view of the screen
> content become inconsistent with old_data.  On the next update round (b)
> will stop working correctly due to that.


Even if old_data becomes inconsistent with the client's screen, the
worst that can happen is that (b) will set as 'to send' framebuffer
areas that have already been sent. Is that so bad? Does it really cause
visible problems? Intuitively I would think not, but I may be wrong.

 
> What my patch does in terms of the old code is make step (c) use
> old_data instead of the guest-visible framebuffer.  I didn't like
> stacking hacks on hacks like this though, so I decided to explicitly
> name the two surfaces.
> 
> You still could get away with just one dirty bitmap for both surfaces.
> Having two bitmaps IMHO makes the code more readable though.  It also
> allows killing the memset(old_data,42,size) hack for forced client
> updates (see framebuffer_update_request()).  An -- in preparation for
> the future -- it makes the vnc code a bit more thread-friendly.  Guest
> screen updating (aka vnc_update()) can run in parallel with encoding
> updates for the guest (i.e. step (c)).

I agree that your patch makes the code more readable, I only am not sure
if it fixes a real world problem.

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Gerd Hoffmann]

Stefano Stabellini wrote:
> Gerd Hoffmann wrote:
> 
>> The trouble spot is here:  The guest might have updated the screen
>> between (b) and (c).  This will cause the vnc clients view of the screen
>> content become inconsistent with old_data.  On the next update round (b)
>> will stop working correctly due to that.
> 
> 
> Even if old_data becomes inconsistent with the client's screen, the
> worst that can happen is that (b) will set as 'to send' framebuffer
> areas that have already been sent. Is that so bad?

(b) doesn't set the 'to send' bit.  It clears the 'to send' bit for
screen areas which are not modified.

You have two possible effects:
 * screen update is sent twice.
 * screen update is *not* sent.

The second causes persistent screen corruptions, it is not fixed up by
the next refresh.

Wanna have a screen shot?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Stefano Stabellini]

Gerd Hoffmann wrote:

> (b) doesn't set the 'to send' bit.  It clears the 'to send' bit for
> screen areas which are not modified.
> 
> You have two possible effects:
>  * screen update is sent twice.
>  * screen update is *not* sent.
> 
> The second causes persistent screen corruptions, it is not fixed up by
> the next refresh.
> 
> Wanna have a screen shot?
> 

Ok, ok I believe you :)

Your patch is fine for me then.

Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Stefano Stabellini]

Gerd Hoffmann wrote:

>diff --git a/vnc.c b/vnc.c
>index 81c842a..66f8946 100644
>--- a/vnc.c
>+++ b/vnc.c
>@@ -649,7 +649,8 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
>     VncDisplay *vd = ds->opaque;
>     VncState *vs = vd->clients;
>     while (vs != NULL) {
>-        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT))
>+        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT) &&
>+            !is_buffer_shared(ds->surface))
>             vnc_copy(vs, src_x, src_y, dst_x, dst_y, w, h);
>         else /* TODO */
>             vnc_update(vs, dst_x, dst_y, w, h);
>



I don't think this is needed (on qemu and qemu-xen, I don't know about
kvm): vnc_copy does not do any actual copy, only sends a copyrect update
to the vnc client.
Why should we prevent this when the buffer is shared?

>@@ -688,36 +689,51 @@ static void vnc_update_client(void *opaque)
> 
>         vnc_set_bits(width_mask, (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
> 
>-	/* Walk through the dirty map and eliminate tiles that
>-	   really aren't dirty */
>-	row = ds_get_data(vs->ds);
>-	old_row = vs->old_data;
>-
>-	for (y = 0; y < ds_get_height(vs->ds); y++) {
>-	    if (vnc_and_bits(vs->dirty_row[y], width_mask, VNC_DIRTY_WORDS)) {
>-		int x;
>-		uint8_t *ptr;
>-		char *old_ptr;
>-
>-		ptr = row;
>-		old_ptr = (char*)old_row;
>-
>-		for (x = 0; x < ds_get_width(vs->ds); x += 16) {
>-		    if (memcmp(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds)) == 0) {
>-			vnc_clear_bit(vs->dirty_row[y], (x / 16));
>-		    } else {
>-			has_dirty = 1;
>-			memcpy(old_ptr, ptr, 16 * ds_get_bytes_per_pixel(vs->ds));
>-		    }
>-
>-		    ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
>-		    old_ptr += 16 * ds_get_bytes_per_pixel(vs->ds);
>-		}
>-	    }
>


    
This loop filters the dirty_row bitmap using a memcmp against the
framebuffer, that could be more up to date than the bitmap itself.
In any case we are getting another vnc_dpy_update call next time
with the correct updated area.
So I don't think we risk losing updates here, the worst that could
happen is sending together portions of the screen more updated than
others to the client.
I would keep this loop even with the buffer shared.
Gerd, am I missing something?