[Qemu-devel] [PATCH 5/5] qxl: support async monitor screen dump

[Alon Levy]

Split qxl_spice_update_area_complete from qxl_render_update, use
SPICE_INTERFACE_QXL_MINOR 2 introduced spice_qxl_update_area_dirty_async
to retrive the dirty rectangles asyncronously (the previous
spice_qxl_update_area_async did not accept a dirty rectangles array).

Introduce SpiceAsyncMonitorScreenDump for a screen_dump.

vga mode screen dumps are still synchronous.

Signed-off-by: Alon Levy <address@hidden>

---
patchcheck gives one false positive, identifying a point function
argument as a multiplication:

ERROR: need consistent spacing around '*' (ctx:WxV)
 #135: FILE: hw/qxl.c:148:
+                           SpiceAsyncCommand *async_command)
                                              ^
---
 hw/qxl-render.c |   67 +++++++++++++++++++++++++++++---------
 hw/qxl.c        |   95 +++++++++++++++++++++++++++++++++++++++++++-----------
 hw/qxl.h        |   38 ++++++++++++++++++++--
 3 files changed, 161 insertions(+), 39 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index fd3c016..7dfd67f 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -27,6 +27,10 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
     uint8_t *dst = qxl->guest_primary.flipped;
     int len, i;
 
+    assert(rect->bottom >= 0);
+    assert(rect->bottom <= qxl->guest_primary.surface.height);
+    assert(rect->top >= 0);
+    assert(rect->top <= qxl->guest_primary.surface.height);
     src += (qxl->guest_primary.surface.height - rect->top - 1) *
         qxl->guest_primary.stride;
     dst += rect->top  * qxl->guest_primary.stride;
@@ -110,17 +114,39 @@ static void qxl_render_display_resized(PCIQXLDevice *qxl)
     dpy_resize(vga->ds);
 }
 
-void qxl_render_update(PCIQXLDevice *qxl)
+void qxl_spice_update_area_complete(PCIQXLDevice *qxl, QXLRect *dirty,
+    int n_dirty)
 {
     VGACommonState *vga = &qxl->vga;
-    QXLRect dirty[32], update;
     int i;
 
+    for (i = 0; i < n_dirty; i++) {
+        if (qemu_spice_rect_is_empty(dirty + i)) {
+            break;
+        }
+        if (qxl->guest_primary.flipped) {
+            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);
+    }
+}
+
+void qxl_render_update(PCIQXLDevice *qxl,
+                       SpiceAsyncMonitorScreenDump *async_screen_dump)
+{
+    QXLRect dirty[32], update;
+
     if (qxl->guest_primary.resized) {
         qxl_render_display_resized(qxl);
     }
-
     if (!qxl->guest_primary.commands) {
+        if (async_screen_dump) {
+            dprint(qxl, 2, "%s: no update required\n", __func__);
+            complete_spice_async_command(qxl, &async_screen_dump->base);
+        }
         return;
     }
     qxl->guest_primary.commands = 0;
@@ -131,20 +157,29 @@ void qxl_render_update(PCIQXLDevice *qxl)
     update.bottom = qxl->guest_primary.surface.height;
 
     memset(dirty, 0, sizeof(dirty));
+    if (async_screen_dump) {
+#if SPICE_INTERFACE_QXL_MINOR >= 2
+        dprint(qxl, 2, "A-SYNC update_area 0 (%d,%d,%d,%d)\n",
+               update.top, update.left, update.bottom, update.right);
+        async_screen_dump->n_dirty = 32;
+        async_screen_dump->dirty = g_malloc0(sizeof(QXLRect) *
+            async_screen_dump->n_dirty);
+        qxl_spice_update_area(qxl, 0, &update,
+                              async_screen_dump->dirty,
+                              async_screen_dump->n_dirty, 1,
+                              &async_screen_dump->base);
+        return;
+#else
+        fprintf(stderr, "%s: warning: fallback to sync, spice-server < 
0.8.4\n",
+                __func__);
+#endif
+    }
+    dprint(qxl, 2, "SYNC update_area\n");
     qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, 0);
-
-    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
-        if (qemu_spice_rect_is_empty(dirty+i)) {
-            break;
-        }
-        if (qxl->guest_primary.flipped) {
-            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);
+                          dirty, ARRAY_SIZE(dirty), 1, NULL);
+    qxl_spice_update_area_complete(qxl, dirty, ARRAY_SIZE(dirty));
+    if (async_screen_dump) {
+        complete_spice_async_command(qxl, &async_screen_dump->base);
     }
 }
 
diff --git a/hw/qxl.c b/hw/qxl.c
index 008f5f7..a3e89cd 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -145,18 +145,32 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t 
surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async, uint64_t cookie)
+                           SpiceAsyncCommand *async_command)
 {
-    if (async == QXL_SYNC) {
+    if (async_command == NULL) {
         qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
                         dirty_rects, num_dirty_rects, clear_dirty_region);
     } else {
+#if SPICE_INTERFACE_QXL_MINOR >= 2
+        if (num_dirty_rects > 0) {
+            spice_qxl_update_area_dirty_async(&qxl->ssd.qxl, surface_id, area,
+                                        dirty_rects, num_dirty_rects,
+                                        clear_dirty_region,
+                                        async_command->cookie);
+        } else {
+            spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
+                                        clear_dirty_region,
+                                        async_command->cookie);
+        }
+#else
 #if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
-                                    clear_dirty_region, cookie);
+                                    clear_dirty_region,
+                                    async_command->cookie);
 #else
         abort();
-#endif
+#endif /* >= 1 */
+#endif /* >= 2 */
     }
 }
 
@@ -742,9 +756,11 @@ SpiceAsyncCommand *push_spice_async_command(PCIQXLDevice 
*qxl,
     assert(size >= sizeof(SpiceAsyncCommand));
     SpiceAsyncCommand *spice_async_command = g_malloc0(size);
 
+    qemu_mutex_lock(&qxl->async_lock);
     spice_async_command->cookie = qxl->async_next_cookie++;
-    spice_async_command->async_io = async_io;
+    spice_async_command->io = async_io;
     QLIST_INSERT_HEAD(&qxl->async_commands, spice_async_command, next);
+    qemu_mutex_unlock(&qxl->async_lock);
     dprint(qxl, 2, "allocated async cookie %"PRId64"\n",
            qxl->async_next_cookie - 1);
     return spice_async_command;
@@ -781,7 +797,7 @@ void complete_spice_async_command(PCIQXLDevice *qxl,
 static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
-    uint32_t current_async;
+    uint32_t io;
     SpiceAsyncCommand *spice_async_command;
 
     spice_async_command = pop_spice_async_command(qxl, cookie);
@@ -790,9 +806,9 @@ static void interface_async_complete(QXLInstance *sin, 
uint64_t cookie)
                        "pending operation, cookie=%ld\n", cookie);
         return;
     }
-    current_async = spice_async_command->async_io;
-    dprint(qxl, 2, "async_complete: %d (%ld) done\n", current_async, cookie);
-    switch (current_async) {
+    io = spice_async_command->io;
+    dprint(qxl, 2, "async_complete: %d (%ld) done\n", io, cookie);
+    switch (io) {
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
         break;
@@ -806,7 +822,11 @@ static void interface_async_complete(QXLInstance *sin, 
uint64_t cookie)
     }
     complete_spice_async_command(qxl, spice_async_command);
     g_free(spice_async_command);
-    qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
+    if (io <= QXL_IO_RANGE_SIZE) {
+        dprint(qxl, 2, "%s: calling qxl_send_events (cookie %"PRIu64")\n",
+               __func__, cookie);
+        qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
+    }
 }
 
 #endif
@@ -1162,17 +1182,27 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, 
int loadvm)
     qxl_rom_set_dirty(d);
 }
 
+typedef SpiceAsyncCommand *(*SpiceAsyncCommandCreator)(PCIQXLDevice *qxl,
+    uint32_t io, uint32_t val);
+
+static SpiceAsyncCommand *create_async_command(PCIQXLDevice *qxl, uint32_t io,
+                                               uint32_t val)
+{
+    return push_spice_async_command(qxl, io, sizeof(SpiceAsyncCommand));
+}
+
 static void ioport_write(void *opaque, target_phys_addr_t addr,
                          uint64_t val, unsigned size)
 {
     PCIQXLDevice *d = opaque;
     uint32_t io_port = addr;
     qxl_async_io async = QXL_SYNC;
+    SpiceAsyncCommand *spice_async_command = NULL;
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     uint32_t orig_io_port = io_port;
-    SpiceAsyncCommand *spice_async_command = NULL;
 #endif
     uint64_t cookie = 0;
+    SpiceAsyncCommandCreator creator = create_async_command;
 
     switch (io_port) {
     case QXL_IO_RESET:
@@ -1228,10 +1258,7 @@ static void ioport_write(void *opaque, 
target_phys_addr_t addr,
     case QXL_IO_FLUSH_SURFACES_ASYNC:
 async_common:
         async = QXL_ASYNC;
-        qemu_mutex_lock(&d->async_lock);
-        spice_async_command = push_spice_async_command(d, orig_io_port,
-                                                sizeof(SpiceAsyncCommand));
-        qemu_mutex_unlock(&d->async_lock);
+        spice_async_command = creator(d, orig_io_port, val);
         cookie = spice_async_command->cookie;
         dprint(d, 2, "start async %d (%"PRId64") cookie %"PRId64"\n", io_port,
                val, cookie);
@@ -1246,7 +1273,7 @@ async_common:
     {
         QXLRect update = d->ram->update_area;
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async, cookie);
+                              &update, NULL, 0, 0, spice_async_command);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
@@ -1457,7 +1484,7 @@ static void qxl_hw_update(void *opaque)
         break;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
-        qxl_render_update(qxl);
+        qxl_render_update(qxl, NULL);
         break;
     default:
         break;
@@ -1472,6 +1499,36 @@ static void qxl_hw_invalidate(void *opaque)
     vga->invalidate(vga);
 }
 
+static void screen_dump_complete(PCIQXLDevice *qxl,
+                                 SpiceAsyncCommand *spice_async_command)
+{
+    SpiceAsyncMonitorScreenDump *async_screen_dump =
+        (SpiceAsyncMonitorScreenDump *)spice_async_command;
+
+    dprint(qxl, 2, "%s: calling screen_dump_cb >%s<\n", __func__,
+           async_screen_dump->filename);
+    qxl_spice_update_area_complete(qxl, async_screen_dump->dirty,
+                                   async_screen_dump->n_dirty);
+    g_free(async_screen_dump->dirty);
+    ppm_save(async_screen_dump->filename, qxl->ssd.ds->surface);
+    g_free((void *)async_screen_dump->filename);
+    async_screen_dump->cb(async_screen_dump->cb_opaque, NULL);
+}
+
+static SpiceAsyncMonitorScreenDump *push_screen_dump(PCIQXLDevice *qxl,
+        const char *filename, MonitorCompletion *cb, void *cb_opaque)
+{
+    SpiceAsyncMonitorScreenDump *async_command =
+        (SpiceAsyncMonitorScreenDump *)
+            push_spice_async_command(qxl, MONITOR_UPDATE_AREA_ASYNC,
+                                 sizeof(SpiceAsyncMonitorScreenDump));
+    async_command->cb = cb;
+    async_command->cb_opaque = cb_opaque;
+    async_command->filename = g_strdup(filename);
+    async_command->base.completion = screen_dump_complete;
+    return async_command;
+}
+
 static void qxl_hw_screen_dump(void *opaque, const char *filename,
                                MonitorCompletion *cb, void *cb_opaque)
 {
@@ -1481,9 +1538,7 @@ static void qxl_hw_screen_dump(void *opaque, const char 
*filename,
     switch (qxl->mode) {
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
-        qxl_render_update(qxl);
-        ppm_save(filename, qxl->ssd.ds->surface);
-        cb(cb_opaque, NULL);
+        qxl_render_update(qxl, push_screen_dump(qxl, filename, cb, cb_opaque));
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename, cb, cb_opaque);
diff --git a/hw/qxl.h b/hw/qxl.h
index 4c89e14..2375c03 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -17,6 +17,26 @@ enum qxl_mode {
 
 #define QXL_UNDEFINED_IO UINT32_MAX
 
+/* The monitor's screen_dump triggers a update_area
+ * command which must be done asynchroniously to prevent the following
+ * deadlock scenario when the spice client is also a monitor client and
+ * is running single threaded:
+ *
+ *  io thread                   worker thread                client
+ *
+ *   <---------------------------------------------------- screendump
+ *  do_screen_dump-> read------->
+ *                               flush, read
+ *                               client socket------------->
+ *
+ * To avoid adding these to the spice-protocol/spice/qxl_dev.h, they
+ * are defined here. */
+enum {
+    /* Must start larger then QXL_IO_RANGE_SIZE, but that is a moving
+     * target, so split in half. */
+    MONITOR_UPDATE_AREA_ASYNC = 0x80000000,
+};
+
 typedef struct PCIQXLDevice PCIQXLDevice;
 typedef struct SpiceAsyncCommand SpiceAsyncCommand;
 typedef void (*SpiceAsyncCommandCompletion)(PCIQXLDevice *,
@@ -27,11 +47,20 @@ typedef void (*SpiceAsyncCommandCompletion)(PCIQXLDevice *,
 struct SpiceAsyncCommand {
     uint64_t                       cookie;
     void                          *opaque;
-    uint32_t                       async_io;
+    uint32_t                       io;
     SpiceAsyncCommandCompletion    completion;
     QLIST_ENTRY(SpiceAsyncCommand) next;
 };
 
+typedef struct SpiceAsyncMonitorScreenDump {
+    SpiceAsyncCommand  base;
+    MonitorCompletion *cb;
+    void              *cb_opaque;
+    const char        *filename;
+    QXLRect           *dirty;
+    int                n_dirty;
+} SpiceAsyncMonitorScreenDump;
+
 struct PCIQXLDevice {
     PCIDevice          pci;
     SimpleSpiceDisplay ssd;
@@ -132,7 +161,7 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t 
surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
                            uint32_t num_dirty_rects,
                            uint32_t clear_dirty_region,
-                           qxl_async_io async, uint64_t cookie);
+                           SpiceAsyncCommand *async_command);
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count);
 void qxl_spice_oom(PCIQXLDevice *qxl);
@@ -151,11 +180,14 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, 
QXLCommandExt *ext);
 
 /* qxl-render.c */
 void qxl_render_resize(PCIQXLDevice *qxl);
-void qxl_render_update(PCIQXLDevice *qxl);
+void qxl_render_update(PCIQXLDevice *qxl,
+                       SpiceAsyncMonitorScreenDump *async_screen_dump);
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
                                  struct QXLRect *area,
                                  uint32_t clear_dirty_region,
                                  int is_vga);
+void qxl_spice_update_area_complete(PCIQXLDevice *qxl, QXLRect *dirty,
+    int n_dirty);
 #endif
-- 
1.7.7