[Qemu-devel] [PATCH v2 0/4] spice: improve vga mode performance

[Qemu-devel] [PATCH v2 0/4] spice: improve vga mode performance

[Gerd Hoffmann]

  Hi,

This patch series makes spice be more clever on screen updates when in
vga mode (i.e. without qxl guest driver loaded or when using a non-qxl
vga card).  qemu keeps a copy of the screen with the content forwarded
to spice-server, then on updates it goes compare the guest screen with
the copy to figure what has really changed.

[ v2: fix two bugs so the series actually has the desired effect ]

cheers,
  Gerd

Gerd Hoffmann (4):
  spice: switch to queue for vga mode updates
  spice: split qemu_spice_create_update
  spice: add screen mirror
  spice: send updates only for changed screen content

 hw/qxl.c           |    6 +-
 ui/spice-display.c |  136 ++++++++++++++++++++++++++++++++++++++++------------
 ui/spice-display.h |    4 +-
 3 files changed, 111 insertions(+), 35 deletions(-)

[Qemu-devel] [PATCH v2 1/4] spice: switch to queue for vga mode updates

[Gerd Hoffmann]

---
 hw/qxl.c           |    6 +++---
 ui/spice-display.c |   25 ++++++++++++++-----------
 ui/spice-display.h |    3 ++-
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index b726c19..833cd77 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -597,9 +597,9 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     case QXL_MODE_VGA:
         ret = false;
         qemu_mutex_lock(&qxl->ssd.lock);
-        if (qxl->ssd.update != NULL) {
-            update = qxl->ssd.update;
-            qxl->ssd.update = NULL;
+        update = QTAILQ_FIRST(&qxl->ssd.updates);
+        if (update != NULL) {
+            QTAILQ_REMOVE(&qxl->ssd.updates, update, next);
             *ext = update->ext;
             ret = true;
         }
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 99bc665..59c5fd7 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -164,7 +164,7 @@ int qemu_spice_display_is_running(SimpleSpiceDisplay *ssd)
 #endif
 }
 
-static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
+static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
     SimpleSpiceUpdate *update;
     QXLDrawable *drawable;
@@ -175,7 +175,7 @@ static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     struct timespec time_space;
 
     if (qemu_spice_rect_is_empty(&ssd->dirty)) {
-        return NULL;
+        return;
     };
 
     trace_qemu_spice_create_update(
@@ -239,7 +239,7 @@ static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     cmd->data = (uintptr_t)drawable;
 
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
-    return update;
+    QTAILQ_INSERT_TAIL(&ssd->updates, update, next);
 }
 
 /*
@@ -315,6 +315,7 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
 {
     ssd->ds = ds;
     qemu_mutex_init(&ssd->lock);
+    QTAILQ_INIT(&ssd->updates);
     ssd->mouse_x = -1;
     ssd->mouse_y = -1;
     if (ssd->num_surfaces == 0) {
@@ -345,6 +346,8 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
 
 void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
 {
+    SimpleSpiceUpdate *update;
+
     dprint(1, "%s:\n", __FUNCTION__);
 
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
@@ -352,9 +355,9 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
     ssd->conv = NULL;
 
     qemu_mutex_lock(&ssd->lock);
-    if (ssd->update != NULL) {
-        qemu_spice_destroy_update(ssd, ssd->update);
-        ssd->update = NULL;
+    while ((update = QTAILQ_FIRST(&ssd->updates)) != NULL) {
+        QTAILQ_REMOVE(&ssd->updates, update, next);
+        qemu_spice_destroy_update(ssd, update);
     }
     qemu_mutex_unlock(&ssd->lock);
     qemu_spice_destroy_host_primary(ssd);
@@ -384,8 +387,8 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
     vga_hw_update();
 
     qemu_mutex_lock(&ssd->lock);
-    if (ssd->update == NULL) {
-        ssd->update = qemu_spice_create_update(ssd);
+    if (QTAILQ_EMPTY(&ssd->updates)) {
+        qemu_spice_create_update(ssd);
         ssd->notify++;
     }
     qemu_spice_cursor_refresh_unlocked(ssd);
@@ -442,9 +445,9 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     dprint(3, "%s:\n", __FUNCTION__);
 
     qemu_mutex_lock(&ssd->lock);
-    if (ssd->update != NULL) {
-        update = ssd->update;
-        ssd->update = NULL;
+    update = QTAILQ_FIRST(&ssd->updates);
+    if (update != NULL) {
+        QTAILQ_REMOVE(&ssd->updates, update, next);
         *ext = update->ext;
         ret = true;
     }
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 512ab78..3fcb6fe 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -92,7 +92,7 @@ struct SimpleSpiceDisplay {
      * to them must be protected by the lock.
      */
     QemuMutex lock;
-    SimpleSpiceUpdate *update;
+    QTAILQ_HEAD(, SimpleSpiceUpdate) updates;
     QEMUCursor *cursor;
     int mouse_x, mouse_y;
 };
@@ -102,6 +102,7 @@ struct SimpleSpiceUpdate {
     QXLImage image;
     QXLCommandExt ext;
     uint8_t *bitmap;
+    QTAILQ_ENTRY(SimpleSpiceUpdate) next;
 };
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/4] spice: split qemu_spice_create_update

[Gerd Hoffmann]

Creating one function which creates a single update for a given
rectangle.  And one (for now) pretty simple wrapper around it to
queue up screen updates for the dirty region.

[ v2: also update bounding box ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c |   31 ++++++++++++++++++-------------
 1 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 59c5fd7..6f68f28 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -164,7 +164,8 @@ int qemu_spice_display_is_running(SimpleSpiceDisplay *ssd)
 #endif
 }
 
-static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
+static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
+                                         QXLRect *rect)
 {
     SimpleSpiceUpdate *update;
     QXLDrawable *drawable;
@@ -174,24 +175,20 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     int by, bw, bh;
     struct timespec time_space;
 
-    if (qemu_spice_rect_is_empty(&ssd->dirty)) {
-        return;
-    };
-
     trace_qemu_spice_create_update(
-           ssd->dirty.left, ssd->dirty.right,
-           ssd->dirty.top, ssd->dirty.bottom);
+           rect->left, rect->right,
+           rect->top, rect->bottom);
 
     update   = g_malloc0(sizeof(*update));
     drawable = &update->drawable;
     image    = &update->image;
     cmd      = &update->ext.cmd;
 
-    bw       = ssd->dirty.right - ssd->dirty.left;
-    bh       = ssd->dirty.bottom - ssd->dirty.top;
+    bw       = rect->right - rect->left;
+    bh       = rect->bottom - rect->top;
     update->bitmap = g_malloc(bw * bh * 4);
 
-    drawable->bbox            = ssd->dirty;
+    drawable->bbox            = *rect;
     drawable->clip.type       = SPICE_CLIP_TYPE_NONE;
     drawable->effect          = QXL_EFFECT_OPAQUE;
     drawable->release_info.id = (uintptr_t)update;
@@ -226,8 +223,8 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     }
 
     src = ds_get_data(ssd->ds) +
-        ssd->dirty.top * ds_get_linesize(ssd->ds) +
-        ssd->dirty.left * ds_get_bytes_per_pixel(ssd->ds);
+        rect->top * ds_get_linesize(ssd->ds) +
+        rect->left * ds_get_bytes_per_pixel(ssd->ds);
     dst = update->bitmap;
     for (by = 0; by < bh; by++) {
         qemu_pf_conv_run(ssd->conv, dst, src, bw);
@@ -238,10 +235,18 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     cmd->type = QXL_CMD_DRAW;
     cmd->data = (uintptr_t)drawable;
 
-    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
     QTAILQ_INSERT_TAIL(&ssd->updates, update, next);
 }
 
+static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
+{
+    if (qemu_spice_rect_is_empty(&ssd->dirty)) {
+        return;
+    };
+    qemu_spice_create_one_update(ssd, &ssd->dirty);
+    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
+}
+
 /*
  * Called from spice server thread context (via interface_release_ressource)
  * We do *not* hold the global qemu mutex here, so extra care is needed
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/4] spice: add screen mirror

[Gerd Hoffmann]

Create a screen mirror, keep there a copy of the most recent update
passed on to spice-server.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c |   32 ++++++++++++++++++++++----------
 ui/spice-display.h |    1 +
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6f68f28..973cd53 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -171,8 +171,8 @@ static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
     QXLDrawable *drawable;
     QXLImage *image;
     QXLCommand *cmd;
-    uint8_t *src, *dst;
-    int by, bw, bh;
+    uint8_t *src, *mirror, *dst;
+    int by, bw, bh, offset, bytes;
     struct timespec time_space;
 
     trace_qemu_spice_create_update(
@@ -216,19 +216,18 @@ static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
     image->bitmap.palette = 0;
     image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
 
-    if (ssd->conv == NULL) {
-        PixelFormat dst = qemu_default_pixelformat(32);
-        ssd->conv = qemu_pf_conv_get(&dst, &ssd->ds->surface->pf);
-        assert(ssd->conv);
-    }
-
-    src = ds_get_data(ssd->ds) +
+    offset =
         rect->top * ds_get_linesize(ssd->ds) +
         rect->left * ds_get_bytes_per_pixel(ssd->ds);
+    bytes = ds_get_bytes_per_pixel(ssd->ds) * bw;
+    src = ds_get_data(ssd->ds) + offset;
+    mirror = ssd->ds_mirror + offset;
     dst = update->bitmap;
     for (by = 0; by < bh; by++) {
-        qemu_pf_conv_run(ssd->conv, dst, src, bw);
+        memcpy(mirror, src, bytes);
+        qemu_pf_conv_run(ssd->conv, dst, mirror, bw);
         src += ds_get_linesize(ssd->ds);
+        mirror += ds_get_linesize(ssd->ds);
         dst += image->bitmap.stride;
     }
 
@@ -243,6 +242,17 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     if (qemu_spice_rect_is_empty(&ssd->dirty)) {
         return;
     };
+
+    if (ssd->conv == NULL) {
+        PixelFormat dst = qemu_default_pixelformat(32);
+        ssd->conv = qemu_pf_conv_get(&dst, &ssd->ds->surface->pf);
+        assert(ssd->conv);
+    }
+    if (ssd->ds_mirror == NULL) {
+        int size = ds_get_height(ssd->ds) * ds_get_linesize(ssd->ds);
+        ssd->ds_mirror = g_malloc0(size);
+    }
+
     qemu_spice_create_one_update(ssd, &ssd->dirty);
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
 }
@@ -358,6 +368,8 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
     qemu_pf_conv_put(ssd->conv);
     ssd->conv = NULL;
+    g_free(ssd->ds_mirror);
+    ssd->ds_mirror = NULL;
 
     qemu_mutex_lock(&ssd->lock);
     while ((update = QTAILQ_FIRST(&ssd->updates)) != NULL) {
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 3fcb6fe..dea41c1 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -72,6 +72,7 @@ typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
 
 struct SimpleSpiceDisplay {
     DisplayState *ds;
+    uint8_t *ds_mirror;
     void *buf;
     int bufsize;
     QXLWorker *worker;
-- 
1.7.1

[Qemu-devel] [PATCH v2 4/4] spice: send updates only for changed screen content

[Gerd Hoffmann]

when creating screen updates go compare the current guest screen
against the mirror (which holds the most recent update sent), then
only create updates for the screen areas which did actually change.

[ v2: drop redundant qemu_spice_create_one_update call ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 973cd53..d062765 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -239,6 +239,13 @@ static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
 
 static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
+    static const int blksize = 32;
+    int blocks = (ds_get_width(ssd->ds) + blksize - 1) / blksize;
+    int dirty_top[blocks];
+    int y, yoff, x, xoff, blk, bw;
+    int bpp = ds_get_bytes_per_pixel(ssd->ds);
+    uint8_t *guest, *mirror;
+
     if (qemu_spice_rect_is_empty(&ssd->dirty)) {
         return;
     };
@@ -253,7 +260,54 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
         ssd->ds_mirror = g_malloc0(size);
     }
 
-    qemu_spice_create_one_update(ssd, &ssd->dirty);
+    for (blk = 0; blk < blocks; blk++) {
+        dirty_top[blk] = -1;
+    }
+
+    guest = ds_get_data(ssd->ds);
+    mirror = ssd->ds_mirror;
+    for (y = ssd->dirty.top; y < ssd->dirty.bottom; y++) {
+        yoff = y * ds_get_linesize(ssd->ds);
+        for (x = ssd->dirty.left; x < ssd->dirty.right; x += blksize) {
+            xoff = x * bpp;
+            blk = x / blksize;
+            bw = MIN(blksize, ssd->dirty.right - x);
+            if (memcmp(guest + yoff + xoff,
+                       mirror + yoff + xoff,
+                       bw * bpp) == 0) {
+                if (dirty_top[blk] != -1) {
+                    QXLRect update = {
+                        .top    = dirty_top[blk],
+                        .bottom = y,
+                        .left   = x,
+                        .right  = x + bw,
+                    };
+                    qemu_spice_create_one_update(ssd, &update);
+                    dirty_top[blk] = -1;
+                }
+            } else {
+                if (dirty_top[blk] == -1) {
+                    dirty_top[blk] = y;
+                }
+            }
+        }
+    }
+
+    for (x = ssd->dirty.left; x < ssd->dirty.right; x += blksize) {
+        blk = x / blksize;
+        bw = MIN(blksize, ssd->dirty.right - x);
+        if (dirty_top[blk] != -1) {
+            QXLRect update = {
+                .top    = dirty_top[blk],
+                .bottom = ssd->dirty.bottom,
+                .left   = x,
+                .right  = x + bw,
+            };
+            qemu_spice_create_one_update(ssd, &update);
+            dirty_top[blk] = -1;
+        }
+    }
+
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 1/4] spice: switch to queue for vga mode updates

[Kevin Wolf]

Am 06.09.2012 07:39, schrieb Gerd Hoffmann:
> ---
>  hw/qxl.c           |    6 +++---
>  ui/spice-display.c |   25 ++++++++++++++-----------
>  ui/spice-display.h |    3 ++-
>  3 files changed, 19 insertions(+), 15 deletions(-)

SoB is missing in this one.

Kevin

Re: [Qemu-devel] [PATCH v2 1/4] spice: switch to queue for vga mode updates

[Gerd Hoffmann]

On 09/06/12 14:07, Kevin Wolf wrote:
> SoB is missing in this one.

Added.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH v2 0/4] spice: improve vga mode performance

[Alon Levy]

On Thu, Sep 06, 2012 at 07:39:23AM +0200, Gerd Hoffmann wrote:
>   Hi,

Series is

Reviewed-by: Alon Levy <alevy@redhat.com>

> 
> This patch series makes spice be more clever on screen updates when in
> vga mode (i.e. without qxl guest driver loaded or when using a non-qxl
> vga card).  qemu keeps a copy of the screen with the content forwarded
> to spice-server, then on updates it goes compare the guest screen with
> the copy to figure what has really changed.
> 
> [ v2: fix two bugs so the series actually has the desired effect ]
> 
> cheers,
>   Gerd
> 
> Gerd Hoffmann (4):
>   spice: switch to queue for vga mode updates
>   spice: split qemu_spice_create_update
>   spice: add screen mirror
>   spice: send updates only for changed screen content
> 
>  hw/qxl.c           |    6 +-
>  ui/spice-display.c |  136 ++++++++++++++++++++++++++++++++++++++++------------
>  ui/spice-display.h |    4 +-
>  3 files changed, 111 insertions(+), 35 deletions(-)
> 
>