[Spice-devel] [RFC v4 8/9] qxl: make qxl_render_update async

Alon Levy alevy at redhat.com
Tue Feb 21 13:39:36 PST 2012


RHBZ# 747011

Removes the last user of QXL_SYNC when using update drivers that use the
_ASYNC io ports.

The last user is qxl_render_update, it is called both by qxl_hw_update
which is the vga_hw_update_ptr passed to graphic_console_init, and by
qxl_hw_screen_dump.

At the same time the QXLRect area being passed to the red_worker thread
is passed as a copy, as part of the QXLCookie.

The implementation uses a bh to make sure dpy_update and qxl_flip are
called from the io thread, otherwise the vga->ds->surface.data can
change under our feet.

With this patch sdl+spice works fine. But spice by itself doesn't
produce the expected screendumps unless repeated a few times, due to
ppm_save being called before update_area (rendering done in spice server
thread) having a chance to complete. Fixed by next patch, but see commit
message for problem introduced by it.

Signed-off-by: Alon Levy <alevy at redhat.com>
---
 hw/qxl-render.c    |  140 +++++++++++++++++++++++++++++++++++++++------------
 hw/qxl.c           |   80 +++++++++++++++++++++++++++--
 hw/qxl.h           |   12 +++++
 ui/spice-display.h |    6 ++
 4 files changed, 199 insertions(+), 39 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 0127856..23e6929 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -31,6 +31,8 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
         dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__);
         qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
     }
+    dprint(qxl, 1, "%s: %d, %d, %d, %d\n", __func__,
+            rect->left, rect->right, rect->top, rect->bottom);
     src = qxl->guest_primary.data;
     src += (qxl->guest_primary.surface.height - rect->top - 1) *
         qxl->guest_primary.abs_stride;
@@ -76,13 +78,78 @@ void qxl_render_resize(PCIQXLDevice *qxl)
     }
 }
 
-void qxl_render_update(PCIQXLDevice *qxl)
+static void qxl_render_clear_update_redraw_unlocked(PCIQXLDevice *qxl)
+{
+    qxl->render_update_redraw = 0;
+    memset(&qxl->render_update_redraw_area, 0,
+            sizeof(qxl->render_update_redraw_area));
+    qxl->num_dirty_rects = 0;
+}
+
+static void qxl_displaysurface_resize(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
-    QXLRect dirty[32], update;
-    int i, redraw = 0;
+
+    dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n",
+           __func__);
+    qemu_resize_displaysurface(vga->ds,
+            qxl->guest_primary.surface.width,
+            qxl->guest_primary.surface.height);
+    qxl->render_update_redraw = 1;
+    qxl->render_update_redraw_area.left   = 0;
+    qxl->render_update_redraw_area.right  =
+        qxl->guest_primary.surface.width;
+    qxl->render_update_redraw_area.top    = 0;
+    qxl->render_update_redraw_area.bottom =
+        qxl->guest_primary.surface.height;
+}
+
+static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
+{
+    VGACommonState *vga = &qxl->vga;
+    int i;
     DisplaySurface *surface = vga->ds->surface;
 
+    if (surface->width != qxl->guest_primary.surface.width ||
+        surface->height != qxl->guest_primary.surface.height) {
+        qxl_displaysurface_resize(qxl);
+    }
+    if (qxl->render_update_redraw) {
+        qxl->dirty[0] = qxl->render_update_redraw_area;
+        qxl->num_dirty_rects = 1;
+        qxl_render_clear_update_redraw_unlocked(qxl);
+        dprint(qxl, 1, "%s: redrawing %d,%d,%d,%d\n", __func__,
+                qxl->dirty[0].left,
+                qxl->dirty[0].right,
+                qxl->dirty[0].top,
+                qxl->dirty[0].bottom);
+    }
+    for (i = 0; i < qxl->num_dirty_rects; i++) {
+        if (qemu_spice_rect_is_empty(qxl->dirty+i)) {
+            break;
+        }
+        qxl_flip(qxl, qxl->dirty+i);
+        dpy_update(vga->ds,
+                   qxl->dirty[i].left, qxl->dirty[i].top,
+                   qxl->dirty[i].right - qxl->dirty[i].left,
+                   qxl->dirty[i].bottom - qxl->dirty[i].top);
+    }
+    qxl->num_dirty_rects = 0;
+}
+
+/*
+ * use ssd.lock to protect render_update_cookie_num.
+ * qxl_render_update is called by io thread or vcpu thread, and the completion
+ * callbacks are called by spice_server thread, defering to bh called from the
+ * io thread.
+ */
+void qxl_render_update(PCIQXLDevice *qxl)
+{
+    int redraw = 0;
+    QXLCookie *cookie;
+
+    qemu_mutex_lock(&qxl->ssd.lock);
+
     if (qxl->guest_primary.resized) {
         qxl->guest_primary.resized = 0;
 
@@ -94,43 +161,50 @@ void qxl_render_update(PCIQXLDevice *qxl)
                qxl->guest_primary.qxl_stride,
                qxl->guest_primary.bytes_pp,
                qxl->guest_primary.bits_pp);
+        qxl_displaysurface_resize(qxl);
     }
 
     if (!qxl->guest_primary.commands) {
+        qemu_mutex_unlock(&qxl->ssd.lock);
         return;
     }
     qxl->guest_primary.commands = 0;
+    cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
+                            0);
+    qxl->render_update_redraw = MAX(qxl->render_update_redraw, redraw);
+    cookie->u.render.area.left   = 0;
+    cookie->u.render.area.right  = qxl->guest_primary.surface.width;
+    cookie->u.render.area.top    = 0;
+    cookie->u.render.area.bottom = qxl->guest_primary.surface.height;
+    qxl->render_update_redraw_area = cookie->u.render.area;
+    qxl->render_update_cookie_num++;
+    qemu_mutex_unlock(&qxl->ssd.lock);
+    qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL,
+                          0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie);
+}
 
-    update.left   = 0;
-    update.right  = qxl->guest_primary.surface.width;
-    update.top    = 0;
-    update.bottom = qxl->guest_primary.surface.height;
-
-    memset(dirty, 0, sizeof(dirty));
-    qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL);
-    if (redraw) {
-        memset(dirty, 0, sizeof(dirty));
-        dirty[0] = update;
-    }
-    if (surface->width != qxl->guest_primary.surface.width ||
-        surface->height != qxl->guest_primary.surface.height) {
-        dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n",
-               __func__);
-        qemu_resize_displaysurface(vga->ds,
-                qxl->guest_primary.surface.width,
-                qxl->guest_primary.surface.height);
-    }
-    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
-        if (qemu_spice_rect_is_empty(dirty+i)) {
-            break;
-        }
-        qxl_flip(qxl, dirty+i);
-        dpy_update(vga->ds,
-                   dirty[i].left, dirty[i].top,
-                   dirty[i].right - dirty[i].left,
-                   dirty[i].bottom - dirty[i].top);
-    }
+/*
+ * Called from main thread, via bh
+ *
+ * Since it doesn't involve a cookie there is no way to match it to a specific
+ * qxl_render_update operation (there may be multiple). So ignore it if there
+ * are none pending (render_update_cookie_num == 0).
+ */
+void qxl_render_update_area_bh(void *opaque)
+{
+    PCIQXLDevice *qxl = opaque;
+
+    qemu_mutex_lock(&qxl->ssd.lock);
+    qxl_render_update_area_unlocked(qxl);
+    qemu_mutex_unlock(&qxl->ssd.lock);
+}
+
+void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
+{
+    qemu_mutex_lock(&qxl->ssd.lock);
+    qxl->render_update_cookie_num--;
+    qemu_mutex_unlock(&qxl->ssd.lock);
+    g_free(cookie);
 }
 
 static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
diff --git a/hw/qxl.c b/hw/qxl.c
index 6f94edb..b4f84f2 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -180,7 +180,7 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
 
 static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl)
 {
- spice_qxl_flush_surfaces_async(&qxl->ssd.qxl,
+    spice_qxl_flush_surfaces_async(&qxl->ssd.qxl,
         (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
                                  QXL_IO_FLUSH_SURFACES_ASYNC));
 }
@@ -746,6 +746,11 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
     }
 
     switch (current_async) {
+    case QXL_IO_MEMSLOT_ADD_ASYNC:
+    case QXL_IO_DESTROY_PRIMARY_ASYNC:
+    case QXL_IO_UPDATE_AREA_ASYNC:
+    case QXL_IO_FLUSH_SURFACES_ASYNC:
+        break;
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
         break;
@@ -755,11 +760,63 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
     case QXL_IO_DESTROY_SURFACE_ASYNC:
         qxl_spice_destroy_surface_wait_complete(qxl, cookie->u.surface_id);
         break;
+    default:
+        fprintf(stderr, "qxl: %s: unexpected current_async %d\n", __func__,
+                current_async);
     }
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
 
 /* called from spice server thread context only */
+static void interface_update_area_complete(QXLInstance *sin,
+        uint32_t surface_id,
+        QXLRect *dirty, uint32_t num_updated_rects)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    int i;
+    int qxl_i;
+    int num_merged;
+    int num_unmerged;
+
+    if (surface_id != 0 || !qxl->render_update_cookie_num) {
+        return;
+    }
+    qemu_mutex_lock(&qxl->ssd.lock);
+    if (qxl->render_update_redraw) {
+        /* don't bother copying them over since we will ignore them */
+        qxl->num_dirty_rects += num_updated_rects;
+        dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n",
+               __func__, qxl->num_dirty_rects);
+        qemu_bh_schedule(qxl->update_area_bh);
+        qemu_mutex_unlock(&qxl->ssd.lock);
+        return;
+    }
+    if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) {
+        /*
+         * overflow - merge all remaining rects. Hoping this is not
+         * common so doesn't need to be optimized
+         */
+    }
+    num_merged = MAX(0,
+            (int)qxl->num_dirty_rects + (int)num_updated_rects
+                                      - QXL_NUM_DIRTY_RECTS);
+    num_unmerged = num_updated_rects - num_merged;
+    qxl_i = qxl->num_dirty_rects;
+    for (i = 0; i < num_unmerged; i++) {
+        qxl->dirty[qxl_i++] = dirty[i];
+    }
+    qxl->num_dirty_rects += num_unmerged;
+    dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n",
+           __func__, qxl->num_dirty_rects);
+    for (i = 0; i < num_merged; i++) {
+        qemu_spice_rect_union(&qxl->dirty[QXL_NUM_DIRTY_RECTS - 1],
+                &dirty[i + num_unmerged]);
+    }
+    qemu_bh_schedule(qxl->update_area_bh);
+    qemu_mutex_unlock(&qxl->ssd.lock);
+}
+
+/* called from spice server thread context only */
 static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
@@ -768,11 +825,15 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     switch (cookie->type) {
     case QXL_COOKIE_TYPE_IO:
         interface_async_complete_io(qxl, cookie);
+        g_free(cookie);
+        break;
+    case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
+        qxl_render_update_area_done(qxl, cookie);
         break;
     default:
         fprintf(stderr, "qxl: %s: unexpected cookie type %d\n", __func__, cookie->type);
+        g_free(cookie);
     }
-    g_free(cookie);
 }
 
 static const QXLInterface qxl_interface = {
@@ -795,6 +856,7 @@ static const QXLInterface qxl_interface = {
     .notify_update           = interface_notify_update,
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
+    .update_area_complete    = interface_update_area_complete,
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1211,11 +1273,15 @@ async_common:
     switch (io_port) {
     case QXL_IO_UPDATE_AREA:
     {
-        QXLRect update = d->ram->update_area;
+        QXLCookie *cookie = NULL;
+
+        if (async == QXL_ASYNC) {
+            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                    QXL_IO_UPDATE_AREA_ASYNC);
+            cookie->u.area = d->ram->update_area;
+        }
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async,
-                              qxl_cookie_new(QXL_COOKIE_TYPE_IO,
-                                             QXL_IO_UPDATE_AREA_ASYNC));
+                              &cookie->u.area, NULL, 0, 0, async, cookie);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
@@ -1596,6 +1662,8 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     init_pipe_signaling(qxl);
     qxl_reset_state(qxl);
 
+    qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
+
     return 0;
 }
 
diff --git a/hw/qxl.h b/hw/qxl.h
index ed297cc..57a94ca 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -18,6 +18,8 @@ enum qxl_mode {
 
 #define QXL_UNDEFINED_IO UINT32_MAX
 
+#define QXL_NUM_DIRTY_RECTS 64
+
 typedef struct PCIQXLDevice {
     PCIDevice          pci;
     SimpleSpiceDisplay ssd;
@@ -89,6 +91,14 @@ typedef struct PCIQXLDevice {
 
     /* io bar */
     MemoryRegion       io_bar;
+
+    /* qxl_render_update state */
+    int                render_update_cookie_num;
+    int                render_update_redraw;
+    QXLRect            render_update_redraw_area;
+    int                num_dirty_rects;
+    QXLRect            dirty[QXL_NUM_DIRTY_RECTS];
+    QEMUBH            *update_area_bh;
 } PCIQXLDevice;
 
 #define PANIC_ON(x) if ((x)) {                         \
@@ -130,3 +140,5 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext);
 void qxl_render_resize(PCIQXLDevice *qxl);
 void qxl_render_update(PCIQXLDevice *qxl);
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
+void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie);
+void qxl_render_update_area_bh(void *opaque);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 93ac78d..81fa1e4 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -50,6 +50,7 @@ typedef enum qxl_async_io {
 
 enum {
     QXL_COOKIE_TYPE_IO,
+    QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
 };
 
 typedef struct QXLCookie {
@@ -57,6 +58,11 @@ typedef struct QXLCookie {
     uint64_t io;
     union {
         uint32_t surface_id;
+        QXLRect area;
+        struct {
+            QXLRect area;
+            int redraw;
+        } render;
     } u;
 } QXLCookie;
 
-- 
1.7.9.1



More information about the Spice-devel mailing list