[Qemu-devel] [PATCHv4 0/6] ui/vnc: update optimizations

[Qemu-devel] [PATCHv4 0/6] ui/vnc: update optimizations

[Peter Lieven]

this series includes several optimizations for the ui/vnc guest to server and server to client
update cycles. comments/reviews appreciated.

v3->v4: - patch 1,4,6 unchanged
        - patch 2: fix 2 missing changes from 16 -> VNC_DIRTY_PIXELS_PER_BIT [Wenchao]
        - patch 3: fixed missing roundup of width in vnc_dpy_switch [Wenchao]
        - patch 3: fixed x offset calculation in vnc_refresh_server_surface
        - patch 5: fixed missing roundup of width in vnc_dpy_update, cleaned up function
v2->v3: - fixed checkpatch warnings [Stefan]
        - fixed an old coding style violation [Stefan]
v1->v2: - new patches 2,4,5
        - patch3: added performance test [Anthony]
        - patch3: further optimized the vnc_update_client by searching for the next zero bit
          with find_next_zero_bit.
        - patch3: further optimized vnc_dpy_switch by using bitmap_set to mask bits dirty.

Peter Lieven (6):
  ui/vnc: introduce VNC_DIRTY_PIXELS_PER_BIT macro
  ui/vnc: derive cmp_bytes from VNC_DIRTY_PIXELS_PER_BIT
  ui/vnc: optimize dirty bitmap tracking
  ui/vnc: optimize clearing in find_and_clear_dirty_height()
  ui/vnc: optimize setting in vnc_dpy_update()
  ui/vnc: disable adaptive update calculations if not needed

 ui/vnc.c |  206 +++++++++++++++++++++++++++++++++++---------------------------
 ui/vnc.h |   10 ++-
 2 files changed, 127 insertions(+), 89 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv4 1/6] ui/vnc: introduce VNC_DIRTY_PIXELS_PER_BIT macro

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |   65 +++++++++++++++++++++++++++++++++++++++-----------------------
 ui/vnc.h |    6 +++++-
 2 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 5601cc3..0925bf2 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -442,17 +442,19 @@ static void vnc_dpy_update(DisplayChangeListener *dcl,
        iteration.  otherwise, if (x % 16) != 0, the last iteration may span
        two 16-pixel blocks but we only mark the first as dirty
     */
-    w += (x % 16);
-    x -= (x % 16);
+    w += (x % VNC_DIRTY_PIXELS_PER_BIT);
+    x -= (x % VNC_DIRTY_PIXELS_PER_BIT);
 
     x = MIN(x, width);
     y = MIN(y, height);
     w = MIN(x + w, width) - x;
     h = MIN(h, height);
 
-    for (; y < h; y++)
-        for (i = 0; i < w; i += 16)
-            set_bit((x + i) / 16, s->dirty[y]);
+    for (; y < h; y++) {
+        for (i = 0; i < w; i += VNC_DIRTY_PIXELS_PER_BIT) {
+            set_bit((x + i) / VNC_DIRTY_PIXELS_PER_BIT, s->dirty[y]);
+        }
+    }
 }
 
 void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
@@ -769,11 +771,12 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
         y = dst_y + h - 1;
         inc = -1;
     }
-    w_lim = w - (16 - (dst_x % 16));
-    if (w_lim < 0)
+    w_lim = w - (VNC_DIRTY_PIXELS_PER_BIT - (dst_x % VNC_DIRTY_PIXELS_PER_BIT));
+    if (w_lim < 0) {
         w_lim = w;
-    else
-        w_lim = w - (w_lim % 16);
+    } else {
+        w_lim = w - (w_lim % VNC_DIRTY_PIXELS_PER_BIT);
+    }
     for (i = 0; i < h; i++) {
         for (x = 0; x <= w_lim;
                 x += s, src_row += cmp_bytes, dst_row += cmp_bytes) {
@@ -781,10 +784,11 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
                 if ((s = w - w_lim) == 0)
                     break;
             } else if (!x) {
-                s = (16 - (dst_x % 16));
+                s = (VNC_DIRTY_PIXELS_PER_BIT -
+                    (dst_x % VNC_DIRTY_PIXELS_PER_BIT));
                 s = MIN(s, w_lim);
             } else {
-                s = 16;
+                s = VNC_DIRTY_PIXELS_PER_BIT;
             }
             cmp_bytes = s * VNC_SERVER_FB_BYTES;
             if (memcmp(src_row, dst_row, cmp_bytes) == 0)
@@ -792,7 +796,8 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
             memmove(dst_row, src_row, cmp_bytes);
             QTAILQ_FOREACH(vs, &vd->clients, next) {
                 if (!vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
-                    set_bit(((x + dst_x) / 16), vs->dirty[y]);
+                    set_bit(((x + dst_x) / VNC_DIRTY_PIXELS_PER_BIT),
+                            vs->dirty[y]);
                 }
             }
         }
@@ -911,7 +916,7 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         for (y = 0; y < height; y++) {
             int x;
             int last_x = -1;
-            for (x = 0; x < width / 16; x++) {
+            for (x = 0; x < width / VNC_DIRTY_PIXELS_PER_BIT; x++) {
                 if (test_and_clear_bit(x, vs->dirty[y])) {
                     if (last_x == -1) {
                         last_x = x;
@@ -921,16 +926,22 @@ static int vnc_update_client(VncState *vs, int has_dirty)
                         int h = find_and_clear_dirty_height(vs, y, last_x, x,
                                                             height);
 
-                        n += vnc_job_add_rect(job, last_x * 16, y,
-                                              (x - last_x) * 16, h);
+                        n += vnc_job_add_rect(job,
+                                              last_x * VNC_DIRTY_PIXELS_PER_BIT,
+                                              y,
+                                              (x - last_x) *
+                                              VNC_DIRTY_PIXELS_PER_BIT,
+                                              h);
                     }
                     last_x = -1;
                 }
             }
             if (last_x != -1) {
                 int h = find_and_clear_dirty_height(vs, y, last_x, x, height);
-                n += vnc_job_add_rect(job, last_x * 16, y,
-                                      (x - last_x) * 16, h);
+                n += vnc_job_add_rect(job, last_x * VNC_DIRTY_PIXELS_PER_BIT,
+                                      y,
+                                      (x - last_x) * VNC_DIRTY_PIXELS_PER_BIT,
+                                      h);
             }
         }
 
@@ -1861,7 +1872,7 @@ static void framebuffer_update_request(VncState *vs, int incremental,
                                        int w, int h)
 {
     int i;
-    const size_t width = surface_width(vs->vd->ds) / 16;
+    const size_t width = surface_width(vs->vd->ds) / VNC_DIRTY_PIXELS_PER_BIT;
     const size_t height = surface_height(vs->vd->ds);
 
     if (y_position > height) {
@@ -2563,7 +2574,9 @@ static int vnc_refresh_lossy_rect(VncDisplay *vd, int x, int y)
 
         vs->lossy_rect[sty][stx] = 0;
         for (j = 0; j < VNC_STAT_RECT; ++j) {
-            bitmap_set(vs->dirty[y + j], x / 16, VNC_STAT_RECT / 16);
+            bitmap_set(vs->dirty[y + j],
+                       x / VNC_DIRTY_PIXELS_PER_BIT,
+                       VNC_STAT_RECT / VNC_DIRTY_PIXELS_PER_BIT);
         }
         has_dirty++;
     }
@@ -2710,17 +2723,21 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
             }
             server_ptr = server_row;
 
-            for (x = 0; x + 15 < width;
-                    x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
-                if (!test_and_clear_bit((x / 16), vd->guest.dirty[y]))
+            for (x = 0; x + VNC_DIRTY_PIXELS_PER_BIT - 1 < width;
+                 x += VNC_DIRTY_PIXELS_PER_BIT, guest_ptr += cmp_bytes,
+                 server_ptr += cmp_bytes) {
+                if (!test_and_clear_bit((x / VNC_DIRTY_PIXELS_PER_BIT),
+                    vd->guest.dirty[y])) {
                     continue;
-                if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0)
+                }
+                if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) {
                     continue;
+                }
                 memcpy(server_ptr, guest_ptr, cmp_bytes);
                 if (!vd->non_adaptive)
                     vnc_rect_updated(vd, x, y, &tv);
                 QTAILQ_FOREACH(vs, &vd->clients, next) {
-                    set_bit((x / 16), vs->dirty[y]);
+                    set_bit((x / VNC_DIRTY_PIXELS_PER_BIT), vs->dirty[y]);
                 }
                 has_dirty++;
             }
diff --git a/ui/vnc.h b/ui/vnc.h
index 6e99213..4a8f33c 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -81,8 +81,12 @@ typedef void VncSendHextileTile(VncState *vs,
 #define VNC_MAX_WIDTH 2560
 #define VNC_MAX_HEIGHT 2048
 
+/* VNC_DIRTY_PIXELS_PER_BIT is the number of dirty pixels represented
+ * by one bit in the dirty bitmap */
+#define VNC_DIRTY_PIXELS_PER_BIT 16
+
 /* VNC_DIRTY_BITS is the number of bits in the dirty bitmap. */
-#define VNC_DIRTY_BITS (VNC_MAX_WIDTH / 16)
+#define VNC_DIRTY_BITS (VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT)
 
 #define VNC_STAT_RECT  64
 #define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 2/6] ui/vnc: derive cmp_bytes from VNC_DIRTY_PIXELS_PER_BIT

[Peter Lieven]

this allows for setting VNC_DIRTY_PIXELS_PER_BIT to different
values than 16 if desired.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 0925bf2..f42398d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2699,7 +2699,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      * Check and copy modified bits from guest to server surface.
      * Update server dirty map.
      */
-    cmp_bytes = 64;
+    cmp_bytes = VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES;
     if (cmp_bytes > vnc_server_fb_stride(vd)) {
         cmp_bytes = vnc_server_fb_stride(vd);
     }
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 3/6] ui/vnc: optimize dirty bitmap tracking

[Peter Lieven]

vnc_update_client currently scans the dirty bitmap of each client
bitwise which is a very costly operation if only few bits are dirty.
vnc_refresh_server_surface does almost the same.
this patch optimizes both by utilizing the heavily optimized
function find_next_bit to find the offset of the next dirty
bit in the dirty bitmaps.

The following artifical test (just the bitmap operation part) running
vnc_update_client 65536 times on a 2560x2048 surface illustrates the
performance difference:

All bits clean - vnc_update_client_new: 0.07 secs
 vnc_update_client_old: 10.98 secs

All bits dirty - vnc_update_client_new: 11.26 secs
 vnc_update_client_old: 20.19 secs

Few bits dirty - vnc_update_client_new: 0.08 secs
 vnc_update_client_old: 10.98 secs

The case for all bits dirty is still rather slow, this
is due to the implementation of find_and_clear_dirty_height.
This will be addresses in a separate patch.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |  155 ++++++++++++++++++++++++++++++++++----------------------------
 ui/vnc.h |    4 ++
 2 files changed, 88 insertions(+), 71 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index f42398d..3412cdf 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -572,6 +572,15 @@ void *vnc_server_fb_ptr(VncDisplay *vd, int x, int y)
     ptr += x * VNC_SERVER_FB_BYTES;
     return ptr;
 }
+/* this sets only the visible pixels of a dirty bitmap */
+#define VNC_SET_VISIBLE_PIXELS_DIRTY(bitmap, w, h) {\
+        int y;\
+        memset(bitmap, 0x00, sizeof(bitmap));\
+        for (y = 0; y < h; y++) {\
+            bitmap_set(bitmap[y], 0,\
+                       DIV_ROUND_UP(w, VNC_DIRTY_PIXELS_PER_BIT));\
+        } \
+    }
 
 static void vnc_dpy_switch(DisplayChangeListener *dcl,
                            DisplaySurface *surface)
@@ -597,7 +606,9 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
     qemu_pixman_image_unref(vd->guest.fb);
     vd->guest.fb = pixman_image_ref(surface->image);
     vd->guest.format = surface->format;
-    memset(vd->guest.dirty, 0xFF, sizeof(vd->guest.dirty));
+    VNC_SET_VISIBLE_PIXELS_DIRTY(vd->guest.dirty,
+                                 surface_width(vd->ds),
+                                 surface_height(vd->ds));
 
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
@@ -605,7 +616,9 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
         if (vs->vd->cursor) {
             vnc_cursor_define(vs);
         }
-        memset(vs->dirty, 0xFF, sizeof(vs->dirty));
+        VNC_SET_VISIBLE_PIXELS_DIRTY(vs->dirty,
+                                     surface_width(vd->ds),
+                                     surface_height(vd->ds));
     }
 }
 
@@ -891,10 +904,9 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         VncDisplay *vd = vs->vd;
         VncJob *job;
         int y;
-        int width, height;
+        int height;
         int n = 0;
 
-
         if (vs->output.offset && !vs->audio_cap && !vs->force_update)
             /* kernel send buffers are full -> drop frames to throttle */
             return 0;
@@ -910,39 +922,27 @@ static int vnc_update_client(VncState *vs, int has_dirty)
          */
         job = vnc_job_new(vs);
 
-        width = MIN(pixman_image_get_width(vd->server), vs->client_width);
         height = MIN(pixman_image_get_height(vd->server), vs->client_height);
 
-        for (y = 0; y < height; y++) {
-            int x;
-            int last_x = -1;
-            for (x = 0; x < width / VNC_DIRTY_PIXELS_PER_BIT; x++) {
-                if (test_and_clear_bit(x, vs->dirty[y])) {
-                    if (last_x == -1) {
-                        last_x = x;
-                    }
-                } else {
-                    if (last_x != -1) {
-                        int h = find_and_clear_dirty_height(vs, y, last_x, x,
-                                                            height);
-
-                        n += vnc_job_add_rect(job,
-                                              last_x * VNC_DIRTY_PIXELS_PER_BIT,
-                                              y,
-                                              (x - last_x) *
-                                              VNC_DIRTY_PIXELS_PER_BIT,
-                                              h);
-                    }
-                    last_x = -1;
-                }
-            }
-            if (last_x != -1) {
-                int h = find_and_clear_dirty_height(vs, y, last_x, x, height);
-                n += vnc_job_add_rect(job, last_x * VNC_DIRTY_PIXELS_PER_BIT,
-                                      y,
-                                      (x - last_x) * VNC_DIRTY_PIXELS_PER_BIT,
-                                      h);
+        y = 0;
+        for (;;) {
+            int x, h;
+            unsigned long x2;
+            unsigned long offset = find_next_bit((unsigned long *) &vs->dirty,
+                                                 height * VNC_DIRTY_BPL(vs),
+                                                 y * VNC_DIRTY_BPL(vs));
+            if (offset == height * VNC_DIRTY_BPL(vs)) {
+                /* no more dirty bits */
+                break;
             }
+            y = offset / VNC_DIRTY_BPL(vs);
+            x = offset % VNC_DIRTY_BPL(vs);
+            x2 = find_next_zero_bit((unsigned long *) &vs->dirty[y],
+                                    VNC_DIRTY_BPL(vs), x);
+            bitmap_clear(vs->dirty[y], x, x2 - x);
+            h = find_and_clear_dirty_height(vs, y, x, x2, height);
+            n += vnc_job_add_rect(job, x * VNC_DIRTY_PIXELS_PER_BIT, y,
+                                  (x2 - x) * VNC_DIRTY_PIXELS_PER_BIT, h);
         }
 
         vnc_job_push(job);
@@ -2680,8 +2680,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
     int width = pixman_image_get_width(vd->guest.fb);
     int height = pixman_image_get_height(vd->guest.fb);
     int y;
-    uint8_t *guest_row;
-    uint8_t *server_row;
+    uint8_t *guest_row0 = NULL, *server_row0;
+    int guest_stride = 0, server_stride;
     int cmp_bytes;
     VncState *vs;
     int has_dirty = 0;
@@ -2706,44 +2706,57 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
     if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
         int width = pixman_image_get_width(vd->server);
         tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
-    }
-    guest_row = (uint8_t *)pixman_image_get_data(vd->guest.fb);
-    server_row = (uint8_t *)pixman_image_get_data(vd->server);
-    for (y = 0; y < height; y++) {
-        if (!bitmap_empty(vd->guest.dirty[y], VNC_DIRTY_BITS)) {
-            int x;
-            uint8_t *guest_ptr;
-            uint8_t *server_ptr;
-
-            if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
-                qemu_pixman_linebuf_fill(tmpbuf, vd->guest.fb, width, 0, y);
-                guest_ptr = (uint8_t *)pixman_image_get_data(tmpbuf);
-            } else {
-                guest_ptr = guest_row;
-            }
-            server_ptr = server_row;
+    } else {
+        guest_row0 = (uint8_t *)pixman_image_get_data(vd->guest.fb);
+        guest_stride = pixman_image_get_stride(vd->guest.fb);
+    }
+    server_row0 = (uint8_t *)pixman_image_get_data(vd->server);
+    server_stride = pixman_image_get_stride(vd->server);
+
+    y = 0;
+    for (;;) {
+        int x;
+        uint8_t *guest_ptr, *server_ptr;
+        unsigned long offset = find_next_bit((unsigned long *) &vd->guest.dirty,
+                                             height * VNC_DIRTY_BPL(&vd->guest),
+                                             y * VNC_DIRTY_BPL(&vd->guest));
+        if (offset == height * VNC_DIRTY_BPL(&vd->guest)) {
+            /* no more dirty bits */
+            break;
+        }
+        y = offset / VNC_DIRTY_BPL(&vd->guest);
+        x = offset % VNC_DIRTY_BPL(&vd->guest);
 
-            for (x = 0; x + VNC_DIRTY_PIXELS_PER_BIT - 1 < width;
-                 x += VNC_DIRTY_PIXELS_PER_BIT, guest_ptr += cmp_bytes,
-                 server_ptr += cmp_bytes) {
-                if (!test_and_clear_bit((x / VNC_DIRTY_PIXELS_PER_BIT),
-                    vd->guest.dirty[y])) {
-                    continue;
-                }
-                if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) {
-                    continue;
-                }
-                memcpy(server_ptr, guest_ptr, cmp_bytes);
-                if (!vd->non_adaptive)
-                    vnc_rect_updated(vd, x, y, &tv);
-                QTAILQ_FOREACH(vs, &vd->clients, next) {
-                    set_bit((x / VNC_DIRTY_PIXELS_PER_BIT), vs->dirty[y]);
-                }
-                has_dirty++;
+        server_ptr = server_row0 + y * server_stride + x * cmp_bytes;
+
+        if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
+            qemu_pixman_linebuf_fill(tmpbuf, vd->guest.fb, width, 0, y);
+            guest_ptr = (uint8_t *)pixman_image_get_data(tmpbuf);
+        } else {
+            guest_ptr = guest_row0 + y * guest_stride;
+        }
+        guest_ptr += x * cmp_bytes;
+
+        for (; x < DIV_ROUND_UP(width, VNC_DIRTY_PIXELS_PER_BIT);
+             x++, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
+            if (!test_and_clear_bit(x, vd->guest.dirty[y])) {
+                continue;
+            }
+            if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) {
+                continue;
+            }
+            memcpy(server_ptr, guest_ptr, cmp_bytes);
+            if (!vd->non_adaptive) {
+                vnc_rect_updated(vd, x * VNC_DIRTY_PIXELS_PER_BIT,
+                                 y, &tv);
             }
+            QTAILQ_FOREACH(vs, &vd->clients, next) {
+                set_bit(x, vs->dirty[y]);
+            }
+            has_dirty++;
         }
-        guest_row  += pixman_image_get_stride(vd->guest.fb);
-        server_row += pixman_image_get_stride(vd->server);
+
+        y++;
     }
     qemu_pixman_image_unref(tmpbuf);
     return has_dirty;
diff --git a/ui/vnc.h b/ui/vnc.h
index 4a8f33c..07e1f59 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -88,6 +88,10 @@ typedef void VncSendHextileTile(VncState *vs,
 /* VNC_DIRTY_BITS is the number of bits in the dirty bitmap. */
 #define VNC_DIRTY_BITS (VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT)
 
+/* VNC_DIRTY_BPL (BPL = bits per line) might be greater than
+ * VNC_DIRTY_BITS due to alignment */
+#define VNC_DIRTY_BPL(x) (sizeof((x)->dirty) / VNC_MAX_HEIGHT * BITS_PER_BYTE)
+
 #define VNC_STAT_RECT  64
 #define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
 #define VNC_STAT_ROWS (VNC_MAX_HEIGHT / VNC_STAT_RECT)
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 4/6] ui/vnc: optimize clearing in find_and_clear_dirty_height()

[Peter Lieven]

The following artifical test (just the bitmap operation part) running
vnc_update_client 65536 times on a 2560x2048 surface illustrates the
performance difference:

All bits clean - vnc_update_client_new: 0.07 secs
                 vnc_update_client_new2: 0.07 secs
                 vnc_update_client_old: 10.98 secs

All bits dirty - vnc_update_client_new: 11.26 secs
               - vnc_update_client_new2: 0.29 secs
                 vnc_update_client_old: 20.19 secs

Few bits dirty - vnc_update_client_new: 0.07 secs
               - vnc_update_client_new2: 0.07 secs
                 vnc_update_client_old: 10.98 secs

vnc_update_client_new2 shows the performance of vnc_update_client
with this patch added.

Comparing with the test run of the last patch the performance
is at least unchanged while it is significantly improved
for the all bits dirty case.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 3412cdf..4117230 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -879,13 +879,10 @@ static int find_and_clear_dirty_height(struct VncState *vs,
     int h;
 
     for (h = 1; h < (height - y); h++) {
-        int tmp_x;
         if (!test_bit(last_x, vs->dirty[y + h])) {
             break;
         }
-        for (tmp_x = last_x; tmp_x < x; tmp_x++) {
-            clear_bit(tmp_x, vs->dirty[y + h]);
-        }
+        bitmap_clear(vs->dirty[y + h], last_x, x - last_x);
     }
 
     return h;
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 5/6] ui/vnc: optimize setting in vnc_dpy_update()

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 4117230..da552fe 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -430,30 +430,24 @@ static int vnc_refresh_server_surface(VncDisplay *vd);
 static void vnc_dpy_update(DisplayChangeListener *dcl,
                            int x, int y, int w, int h)
 {
-    int i;
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     struct VncSurface *s = &vd->guest;
     int width = surface_width(vd->ds);
     int height = surface_height(vd->ds);
 
-    h += y;
-
-    /* round x down to ensure the loop only spans one 16-pixel block per,
-       iteration.  otherwise, if (x % 16) != 0, the last iteration may span
-       two 16-pixel blocks but we only mark the first as dirty
-    */
+    /* this is needed this to ensure we updated all affected
+     * blocks if x % VNC_DIRTY_PIXELS_PER_BIT != 0 */
     w += (x % VNC_DIRTY_PIXELS_PER_BIT);
     x -= (x % VNC_DIRTY_PIXELS_PER_BIT);
 
     x = MIN(x, width);
     y = MIN(y, height);
     w = MIN(x + w, width) - x;
-    h = MIN(h, height);
+    h = MIN(y + h, height);
 
     for (; y < h; y++) {
-        for (i = 0; i < w; i += VNC_DIRTY_PIXELS_PER_BIT) {
-            set_bit((x + i) / VNC_DIRTY_PIXELS_PER_BIT, s->dirty[y]);
-        }
+        bitmap_set(s->dirty[y], x / VNC_DIRTY_PIXELS_PER_BIT,
+                   DIV_ROUND_UP(w, VNC_DIRTY_PIXELS_PER_BIT));
     }
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index da552fe..a742d32 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
             acl = 1;
 #endif
         } else if (strncmp(options, "lossy", 5) == 0) {
+#ifdef CONFIG_VNC_JPEG
             vs->lossy = true;
+#endif
         } else if (strncmp(options, "non-adaptive", 12) == 0) {
             vs->non_adaptive = true;
         } else if (strncmp(options, "share=", 6) == 0) {
@@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
         }
     }
 
+    /* adaptive updates are only used with tight encoding and
+     * if lossy updates are enabled so we can disable all the
+     * calculations otherwise */
+    if (!vs->lossy) {
+        vs->non_adaptive = true;
+    }
+
 #ifdef CONFIG_VNC_TLS
     if (acl && x509 && vs->tls.x509verify) {
         if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 1/6] ui/vnc: introduce VNC_DIRTY_PIXELS_PER_BIT macro

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCHv4 3/6] ui/vnc: optimize dirty bitmap tracking

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCHv4 4/6] ui/vnc: optimize clearing in find_and_clear_dirty_height()

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCHv4 5/6] ui/vnc: optimize setting in vnc_dpy_update()

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Wenchao Xia]

于 2014/1/8 17:08, Peter Lieven 写道:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   ui/vnc.c |    9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index da552fe..a742d32 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>               acl = 1;
>   #endif
>           } else if (strncmp(options, "lossy", 5) == 0) {
> +#ifdef CONFIG_VNC_JPEG
>               vs->lossy = true;
> +#endif
>           } else if (strncmp(options, "non-adaptive", 12) == 0) {
>               vs->non_adaptive = true;
>           } else if (strncmp(options, "share=", 6) == 0) {
> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>           }
>       }
> 
> +    /* adaptive updates are only used with tight encoding and
> +     * if lossy updates are enabled so we can disable all the
> +     * calculations otherwise */
> +    if (!vs->lossy) {
> +        vs->non_adaptive = true;
> +    }
> +

  The code seems: if vs->loosy == false, then vs->non_adaptive = true,
translate as: if loosy update is not used, then don't do adaptive
update., which doesn't conform with the comments. I am not sure if this
is on expectation.


>   #ifdef CONFIG_VNC_TLS
>       if (acl && x509 && vs->tls.x509verify) {
>           if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
> 

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Peter Lieven]

Am 09.01.2014 09:29, schrieb Wenchao Xia:
> 于 2014/1/8 17:08, Peter Lieven 写道:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   ui/vnc.c |    9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index da552fe..a742d32 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>               acl = 1;
>>   #endif
>>           } else if (strncmp(options, "lossy", 5) == 0) {
>> +#ifdef CONFIG_VNC_JPEG
>>               vs->lossy = true;
>> +#endif
>>           } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>               vs->non_adaptive = true;
>>           } else if (strncmp(options, "share=", 6) == 0) {
>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>           }
>>       }
>>
>> +    /* adaptive updates are only used with tight encoding and
>> +     * if lossy updates are enabled so we can disable all the
>> +     * calculations otherwise */
>> +    if (!vs->lossy) {
>> +        vs->non_adaptive = true;
>> +    }
>> +
>   The code seems: if vs->loosy == false, then vs->non_adaptive = true,
> translate as: if loosy update is not used, then don't do adaptive
> update., which doesn't conform with the comments. I am not sure if this
> is on expectation.
It don't see the logic break. The option means non_adaptive, not adaptive.

I write "adaptive updates are only used ... with lossy updates...". Which
is the same as "without lossy updates we don't need adaptive updates".

Peter

>
>
>>   #ifdef CONFIG_VNC_TLS
>>       if (acl && x509 && vs->tls.x509verify) {
>>           if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
>>

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Wenchao Xia]

于 2014/1/10 0:25, Peter Lieven 写道:
> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>    ui/vnc.c |    9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>> index da552fe..a742d32 100644
>>> --- a/ui/vnc.c
>>> +++ b/ui/vnc.c
>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>                acl = 1;
>>>    #endif
>>>            } else if (strncmp(options, "lossy", 5) == 0) {
>>> +#ifdef CONFIG_VNC_JPEG
>>>                vs->lossy = true;
>>> +#endif
>>>            } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>                vs->non_adaptive = true;
>>>            } else if (strncmp(options, "share=", 6) == 0) {
>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>            }
>>>        }
>>>
>>> +    /* adaptive updates are only used with tight encoding and
>>> +     * if lossy updates are enabled so we can disable all the
>>> +     * calculations otherwise */
>>> +    if (!vs->lossy) {
>>> +        vs->non_adaptive = true;
>>> +    }
>>> +
>>    The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>> translate as: if loosy update is not used, then don't do adaptive
>> update., which doesn't conform with the comments. I am not sure if this
>> is on expectation.
> It don't see the logic break. The option means non_adaptive, not adaptive.
> 
> I write "adaptive updates are only used ... with lossy updates...". Which

  So tight encoding means loosy updates?

> is the same as "without lossy updates we don't need adaptive updates".
> 
> Peter
> 
>>
>>
>>>    #ifdef CONFIG_VNC_TLS
>>>        if (acl && x509 && vs->tls.x509verify) {
>>>            if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
>>>
> 
> 

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Peter Lieven]

Am 10.01.2014 04:09, schrieb Wenchao Xia:
> 于 2014/1/10 0:25, Peter Lieven 写道:
>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>    ui/vnc.c |    9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>> index da552fe..a742d32 100644
>>>> --- a/ui/vnc.c
>>>> +++ b/ui/vnc.c
>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>                acl = 1;
>>>>    #endif
>>>>            } else if (strncmp(options, "lossy", 5) == 0) {
>>>> +#ifdef CONFIG_VNC_JPEG
>>>>                vs->lossy = true;
>>>> +#endif
>>>>            } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>                vs->non_adaptive = true;
>>>>            } else if (strncmp(options, "share=", 6) == 0) {
>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>            }
>>>>        }
>>>>
>>>> +    /* adaptive updates are only used with tight encoding and
>>>> +     * if lossy updates are enabled so we can disable all the
>>>> +     * calculations otherwise */
>>>> +    if (!vs->lossy) {
>>>> +        vs->non_adaptive = true;
>>>> +    }
>>>> +
>>>    The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>> translate as: if loosy update is not used, then don't do adaptive
>>> update., which doesn't conform with the comments. I am not sure if this
>>> is on expectation.
>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>
>> I write "adaptive updates are only used ... with lossy updates...". Which
>   So tight encoding means loosy updates?
It means you can only enable lossy updates if you have tight encoding. So if you are
missing tight encoding or lossy is false then you can set non_adaptive to true.

Peter

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Wenchao Xia]

于 2014/1/11 6:28, Peter Lieven 写道:
> Am 10.01.2014 04:09, schrieb Wenchao Xia:
>> 于 2014/1/10 0:25, Peter Lieven 写道:
>>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> ---
>>>>>     ui/vnc.c |    9 +++++++++
>>>>>     1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>> index da552fe..a742d32 100644
>>>>> --- a/ui/vnc.c
>>>>> +++ b/ui/vnc.c
>>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>                 acl = 1;
>>>>>     #endif
>>>>>             } else if (strncmp(options, "lossy", 5) == 0) {
>>>>> +#ifdef CONFIG_VNC_JPEG
>>>>>                 vs->lossy = true;
>>>>> +#endif
>>>>>             } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>>                 vs->non_adaptive = true;
>>>>>             } else if (strncmp(options, "share=", 6) == 0) {
>>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>             }
>>>>>         }
>>>>>
>>>>> +    /* adaptive updates are only used with tight encoding and
>>>>> +     * if lossy updates are enabled so we can disable all the
>>>>> +     * calculations otherwise */
>>>>> +    if (!vs->lossy) {
>>>>> +        vs->non_adaptive = true;
>>>>> +    }
>>>>> +
>>>>     The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>>> translate as: if loosy update is not used, then don't do adaptive
>>>> update., which doesn't conform with the comments. I am not sure if this
>>>> is on expectation.
>>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>>
>>> I write "adaptive updates are only used ... with lossy updates...". Which
>>    So tight encoding means loosy updates?
> It means you can only enable lossy updates if you have tight encoding. So if you are
> missing tight encoding or lossy is false then you can set non_adaptive to true.
> 
> Peter
> 
  I see the logic, guess I punctuated the comments in a wrong way, the
real meaning may be:

    /* Adaptive updates are only used with tight encoding and
     * if lossy updates are enabled, so we can disable all the
     * calculations otherwise */

Re: [Qemu-devel] [PATCHv4 6/6] ui/vnc: disable adaptive update calculations if not needed

[Peter Lieven]

On 13.01.2014 03:42, Wenchao Xia wrote:
> 于 2014/1/11 6:28, Peter Lieven 写道:
>> Am 10.01.2014 04:09, schrieb Wenchao Xia:
>>> 于 2014/1/10 0:25, Peter Lieven 写道:
>>>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>> ---
>>>>>>      ui/vnc.c |    9 +++++++++
>>>>>>      1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>>> index da552fe..a742d32 100644
>>>>>> --- a/ui/vnc.c
>>>>>> +++ b/ui/vnc.c
>>>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>>                  acl = 1;
>>>>>>      #endif
>>>>>>              } else if (strncmp(options, "lossy", 5) == 0) {
>>>>>> +#ifdef CONFIG_VNC_JPEG
>>>>>>                  vs->lossy = true;
>>>>>> +#endif
>>>>>>              } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>>>                  vs->non_adaptive = true;
>>>>>>              } else if (strncmp(options, "share=", 6) == 0) {
>>>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>>              }
>>>>>>          }
>>>>>>
>>>>>> +    /* adaptive updates are only used with tight encoding and
>>>>>> +     * if lossy updates are enabled so we can disable all the
>>>>>> +     * calculations otherwise */
>>>>>> +    if (!vs->lossy) {
>>>>>> +        vs->non_adaptive = true;
>>>>>> +    }
>>>>>> +
>>>>>      The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>>>> translate as: if loosy update is not used, then don't do adaptive
>>>>> update., which doesn't conform with the comments. I am not sure if this
>>>>> is on expectation.
>>>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>>>
>>>> I write "adaptive updates are only used ... with lossy updates...". Which
>>>     So tight encoding means loosy updates?
>> It means you can only enable lossy updates if you have tight encoding. So if you are
>> missing tight encoding or lossy is false then you can set non_adaptive to true.
>>
>> Peter
>>
>    I see the logic, guess I punctuated the comments in a wrong way, the
> real meaning may be:
>
>      /* Adaptive updates are only used with tight encoding and
>       * if lossy updates are enabled, so we can disable all the
>       * calculations otherwise */
>

Agreed. The comma there makes it clearer.

Peter


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................

Re: [Qemu-devel] [PATCHv4 0/6] ui/vnc: update optimizations

[Peter Lieven]

Ping

On 08.01.2014 10:08, Peter Lieven wrote:
> this series includes several optimizations for the ui/vnc guest to server and server to client
> update cycles. comments/reviews appreciated.
>
> v3->v4: - patch 1,4,6 unchanged
>          - patch 2: fix 2 missing changes from 16 -> VNC_DIRTY_PIXELS_PER_BIT [Wenchao]
>          - patch 3: fixed missing roundup of width in vnc_dpy_switch [Wenchao]
>          - patch 3: fixed x offset calculation in vnc_refresh_server_surface
>          - patch 5: fixed missing roundup of width in vnc_dpy_update, cleaned up function
> v2->v3: - fixed checkpatch warnings [Stefan]
>          - fixed an old coding style violation [Stefan]
> v1->v2: - new patches 2,4,5
>          - patch3: added performance test [Anthony]
>          - patch3: further optimized the vnc_update_client by searching for the next zero bit
>            with find_next_zero_bit.
>          - patch3: further optimized vnc_dpy_switch by using bitmap_set to mask bits dirty.
>
> Peter Lieven (6):
>    ui/vnc: introduce VNC_DIRTY_PIXELS_PER_BIT macro
>    ui/vnc: derive cmp_bytes from VNC_DIRTY_PIXELS_PER_BIT
>    ui/vnc: optimize dirty bitmap tracking
>    ui/vnc: optimize clearing in find_and_clear_dirty_height()
>    ui/vnc: optimize setting in vnc_dpy_update()
>    ui/vnc: disable adaptive update calculations if not needed
>
>   ui/vnc.c |  206 +++++++++++++++++++++++++++++++++++---------------------------
>   ui/vnc.h |   10 ++-
>   2 files changed, 127 insertions(+), 89 deletions(-)
>

Re: [Qemu-devel] [PATCHv4 0/6] ui/vnc: update optimizations

[Peter Lieven]

On 08.01.2014 10:08, Peter Lieven wrote:
> this series includes several optimizations for the ui/vnc guest to server and server to client
> update cycles. comments/reviews appreciated.
>
> v3->v4: - patch 1,4,6 unchanged
>          - patch 2: fix 2 missing changes from 16 -> VNC_DIRTY_PIXELS_PER_BIT [Wenchao]
>          - patch 3: fixed missing roundup of width in vnc_dpy_switch [Wenchao]
>          - patch 3: fixed x offset calculation in vnc_refresh_server_surface
>          - patch 5: fixed missing roundup of width in vnc_dpy_update, cleaned up function
> v2->v3: - fixed checkpatch warnings [Stefan]
>          - fixed an old coding style violation [Stefan]
> v1->v2: - new patches 2,4,5
>          - patch3: added performance test [Anthony]
>          - patch3: further optimized the vnc_update_client by searching for the next zero bit
>            with find_next_zero_bit.
>          - patch3: further optimized vnc_dpy_switch by using bitmap_set to mask bits dirty.
>
> Peter Lieven (6):
>    ui/vnc: introduce VNC_DIRTY_PIXELS_PER_BIT macro
>    ui/vnc: derive cmp_bytes from VNC_DIRTY_PIXELS_PER_BIT
>    ui/vnc: optimize dirty bitmap tracking
>    ui/vnc: optimize clearing in find_and_clear_dirty_height()
>    ui/vnc: optimize setting in vnc_dpy_update()
>    ui/vnc: disable adaptive update calculations if not needed
>
>   ui/vnc.c |  206 +++++++++++++++++++++++++++++++++++---------------------------
>   ui/vnc.h |   10 ++-
>   2 files changed, 127 insertions(+), 89 deletions(-)
>

PingV2 ;-)

Peter