[Qemu-devel] [RFC 0/7] qxl: fix hangs caused by qxl_render_update

[Qemu-devel] [RFC 0/7] qxl: fix hangs caused by qxl_render_update

[Alon Levy]

This is the second attempt to fix this issue, as a lesson from the last time it doesn't try to use an async monitor command. So with this patchset, in qxl mode, a screendump monitor command will complete before the file is written to disk. This is much better then a hang. To fix it does require asynchronous monitor command completion.

This is an RFC because of the last patch, that implements a DisplayAllocator trio (create,free,resize) for qxl. Without it there are two incorrect screendumps after each resolution change. With it there is no error, but it still seems it can be improved, in particular it does a redraw for every qxl_render_update because of wrong usage of the QEMU_ALLOCATED_FLAG that is required to get vga_draw_graphic to do the 24 to 32 bit conversion, to get correct display for VBE modes using 24 bit depth.

Please review,

Alon Levy (7):
  sdl: remove NULL check, g_malloc0 can't fail
  qxl: drop qxl_spice_update_area_async definition
  qxl: introduce QXLCookie
  qxl: make qxl_render_update async
  qxl-render: call ppm_save on callback
  qxl: use spice_qxl_update_area_dirty_async
  qxl: add allocator

 hw/qxl-render.c    |   98 +++++++++++++-----------
 hw/qxl.c           |  207 +++++++++++++++++++++++++++++++++++++++++++++-------
 hw/qxl.h           |   12 +--
 ui/sdl.c           |    4 -
 ui/spice-display.c |   27 ++++++-
 ui/spice-display.h |   15 ++++
 6 files changed, 276 insertions(+), 87 deletions(-)

-- 
1.7.9

[Qemu-devel] [RFC 1/7] sdl: remove NULL check, g_malloc0 can't fail

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 ui/sdl.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/ui/sdl.c b/ui/sdl.c
index 6f8091c..f6f711c 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -167,10 +167,6 @@ static PixelFormat sdl_to_qemu_pixelformat(SDL_PixelFormat *sdl_pf)
 static DisplaySurface* sdl_create_displaysurface(int width, int height)
 {
     DisplaySurface *surface = (DisplaySurface*) g_malloc0(sizeof(DisplaySurface));
-    if (surface == NULL) {
-        fprintf(stderr, "sdl_create_displaysurface: malloc failed\n");
-        exit(1);
-    }
 
     surface->width = width;
     surface->height = height;
-- 
1.7.9

[Qemu-devel] [RFC 2/7] qxl: drop qxl_spice_update_area_async definition

[Alon Levy]

It was never used. Introduced in
5ff4e36c804157bd84af43c139f8cd3a59722db9
qxl: async io support using new spice api

But not used even then.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.h |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/qxl.h b/hw/qxl.h
index 766aa6d..7df00f1 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -134,9 +134,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);
-#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);
-#endif
+
+void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie);
-- 
1.7.9

[Qemu-devel] [RFC 3/7] qxl: introduce QXLCookie

[Alon Levy]

Will be used in the next patch.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |    2 +-
 hw/qxl.c           |   65 +++++++++++++++++++++++++++++++++++++--------------
 hw/qxl.h           |    2 +-
 ui/spice-display.c |   26 ++++++++++++++++++--
 ui/spice-display.h |   12 +++++++++
 5 files changed, 84 insertions(+), 23 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 133d093..b238b96 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -133,7 +133,7 @@ void qxl_render_update(PCIQXLDevice *qxl)
 
     memset(dirty, 0, sizeof(dirty));
     qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
+                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL);
     if (redraw) {
         memset(dirty, 0, sizeof(dirty));
         dirty[0] = update;
diff --git a/hw/qxl.c b/hw/qxl.c
index ac69125..9d3b848 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -143,15 +143,20 @@ 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)
+                           qxl_async_io async, QXLCookie *cookie)
 {
     if (async == QXL_SYNC) {
         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 >= 1
+        if (cookie == NULL) {
+            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                    QXL_IO_UPDATE_AREA_ASYNC,
+                                    0);
+        }
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
-                                    clear_dirty_region, 0);
+                                    clear_dirty_region, (uint64_t)cookie);
 #else
         abort();
 #endif
@@ -171,11 +176,13 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
                                            qxl_async_io async)
 {
     if (async) {
-#if SPICE_INTERFACE_QXL_MINOR < 1
-        abort();
-#else
+#if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id,
-                                        (uint64_t)id);
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_DESTROY_SURFACE_ASYNC,
+                                         id));
+#else
+        abort();
 #endif
     } else {
         qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
@@ -186,7 +193,10 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl)
 {
-    spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, 0);
+ spice_qxl_flush_surfaces_async(&qxl->ssd.qxl,
+        (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                 QXL_IO_FLUSH_SURFACES_ASYNC,
+                                 0));
 }
 #endif
 
@@ -217,10 +227,13 @@ static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
 static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
 {
     if (async) {
-#if SPICE_INTERFACE_QXL_MINOR < 1
-        abort();
+#if SPICE_INTERFACE_QXL_MINOR >= 1
+        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl,
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_DESTROY_ALL_SURFACES,
+                                         0));
 #else
-        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl, 0);
+        abort();
 #endif
     } else {
         qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
@@ -737,18 +750,20 @@ static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
 
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 
-/* called from spice server thread context only */
-static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
+static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
 {
-    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
     uint32_t current_async;
 
     qemu_mutex_lock(&qxl->async_lock);
     current_async = qxl->current_async;
     qxl->current_async = QXL_UNDEFINED_IO;
     qemu_mutex_unlock(&qxl->async_lock);
+    dprint(qxl, 2, "async_complete: %d (%p) done\n", current_async, cookie);
+    if (current_async != cookie->io) {
+        fprintf(stderr, "qxl: %s: error: current_async = %d != %ld = cookie->io\n",
+                __func__, current_async, cookie->io);
+    }
 
-    dprint(qxl, 2, "async_complete: %d (%ld) done\n", current_async, cookie);
     switch (current_async) {
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
@@ -757,12 +772,28 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
         qxl_spice_destroy_surfaces_complete(qxl);
         break;
     case QXL_IO_DESTROY_SURFACE_ASYNC:
-        qxl_spice_destroy_surface_wait_complete(qxl, (uint32_t)cookie);
+        qxl_spice_destroy_surface_wait_complete(qxl, (uint32_t)cookie->data);
         break;
     }
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
 
+/* 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);
+    QXLCookie *cookie = (QXLCookie*)cookie_token;
+
+    switch (cookie->type) {
+    case QXL_COOKIE_TYPE_IO:
+        interface_async_complete_io(qxl, cookie);
+        break;
+    default:
+        fprintf(stderr, "qxl: %s: unexpected cookie type %d\n", __func__, cookie->type);
+    }
+    g_free(cookie);
+}
+
 #endif
 
 static const QXLInterface qxl_interface = {
@@ -1077,9 +1108,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async)
     if (d->mode == QXL_MODE_UNDEFINED) {
         return 0;
     }
-
     dprint(d, 1, "%s\n", __FUNCTION__);
-
     d->mode = QXL_MODE_UNDEFINED;
     qemu_spice_destroy_primary_surface(&d->ssd, 0, async);
     qxl_spice_reset_cursor(d);
@@ -1215,7 +1244,7 @@ async_common:
     {
         QXLRect update = d->ram->update_area;
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async);
+                              &update, NULL, 0, 0, async, NULL);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
diff --git a/hw/qxl.h b/hw/qxl.h
index 7df00f1..71d5c35 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -118,7 +118,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);
+                           qxl_async_io async, QXLCookie *cookie);
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count);
 void qxl_spice_oom(PCIQXLDevice *qxl);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6c302a3..680e6f4 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -60,12 +60,26 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
     dest->right = MAX(dest->right, r->right);
 }
 
+QXLCookie *qxl_cookie_new(int type, uint64_t io, uint64_t data)
+{
+    QXLCookie *cookie;
+
+    cookie = g_malloc0(sizeof(*cookie));
+    cookie->type = type;
+    cookie->io = io;
+    cookie->data = data;
+    return cookie;
+}
+
 void qemu_spice_add_memslot(SimpleSpiceDisplay *ssd, QXLDevMemSlot *memslot,
                             qxl_async_io async)
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_add_memslot_async(&ssd->qxl, memslot, 0);
+        spice_qxl_add_memslot_async(&ssd->qxl, memslot,
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_MEMSLOT_ADD_ASYNC,
+                                         0));
 #else
         abort();
 #endif
@@ -85,7 +99,10 @@ void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_create_primary_surface_async(&ssd->qxl, id, surface, 0);
+        spice_qxl_create_primary_surface_async(&ssd->qxl, id, surface,
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_CREATE_PRIMARY_ASYNC,
+                                         0));
 #else
         abort();
 #endif
@@ -100,7 +117,10 @@ void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
 {
     if (async != QXL_SYNC) {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        spice_qxl_destroy_primary_surface_async(&ssd->qxl, id, 0);
+        spice_qxl_destroy_primary_surface_async(&ssd->qxl, id,
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_DESTROY_PRIMARY_ASYNC,
+                                         0));
 #else
         abort();
 #endif
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 5e52df9..c8747ab 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -48,6 +48,18 @@ typedef enum qxl_async_io {
     QXL_ASYNC,
 } qxl_async_io;
 
+enum {
+    QXL_COOKIE_TYPE_IO,
+};
+
+typedef struct QXLCookie {
+    int      type;
+    uint64_t io;
+    uint64_t data;
+} QXLCookie;
+
+QXLCookie *qxl_cookie_new(int type, uint64_t io, uint64_t data);
+
 typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
 typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
 
-- 
1.7.9

[Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async

[Alon Levy]

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, allocated and copied before passing, deallocated on
the interface_async_complete callback.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |   43 ++++++++++++++++++++++++++++++++-----------
 hw/qxl.c           |   33 ++++++++++++++++++++++++++++++---
 ui/spice-display.h |    1 +
 3 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index b238b96..7f9fbca 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -71,12 +71,19 @@ void qxl_render_resize(PCIQXLDevice *qxl)
     }
 }
 
+typedef struct QXLRenderUpdateData {
+    int redraw;
+    QXLRect dirty[32];
+    QXLRect area;
+} QXLRenderUpdateData;
+
 void qxl_render_update(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
-    QXLRect dirty[32], update;
+    QXLRect dirty[32];
     void *ptr;
-    int i, redraw = 0;
+    int redraw = 0;
+    QXLRenderUpdateData *data;
 
     if (!is_buffer_shared(vga->ds->surface)) {
         dprint(qxl, 1, "%s: restoring shared displaysurface\n", __func__);
@@ -126,20 +133,33 @@ void qxl_render_update(PCIQXLDevice *qxl)
     }
     qxl->guest_primary.commands = 0;
 
-    update.left   = 0;
-    update.right  = qxl->guest_primary.surface.width;
-    update.top    = 0;
-    update.bottom = qxl->guest_primary.surface.height;
+    data = g_malloc0(sizeof(*data));
+    data->redraw = redraw;
+    data->area.left   = 0;
+    data->area.right  = qxl->guest_primary.surface.width;
+    data->area.top    = 0;
+    data->area.bottom = qxl->guest_primary.surface.height;
+    qxl_spice_update_area(qxl, 0, &data->area,
+                          data->dirty, ARRAY_SIZE(dirty), 1, QXL_ASYNC,
+                          qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
+                                         0,
+                                         (uint64_t)data));
+}
+
+void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
+{
+    QXLRenderUpdateData *data = (QXLRenderUpdateData *)cookie->data;
+    QXLRect update = data->area;
+    QXLRect *dirty = data->dirty;
+    VGACommonState *vga = &qxl->vga;
+    int i;
 
-    memset(dirty, 0, sizeof(dirty));
-    qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL);
-    if (redraw) {
+    if (data->redraw) {
         memset(dirty, 0, sizeof(dirty));
         dirty[0] = update;
     }
 
-    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
+    for (i = 0; i < ARRAY_SIZE(data->dirty); i++) {
         if (qemu_spice_rect_is_empty(dirty+i)) {
             break;
         }
@@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl)
                    dirty[i].right - dirty[i].left,
                    dirty[i].bottom - dirty[i].top);
     }
+    g_free(data);
 }
 
 static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
diff --git a/hw/qxl.c b/hw/qxl.c
index 9d3b848..5563293 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            uint32_t clear_dirty_region,
                            qxl_async_io async, QXLCookie *cookie)
 {
+    struct QXLRect *area_copy;
     if (async == QXL_SYNC) {
         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 >= 1
         if (cookie == NULL) {
+            area_copy = g_malloc0(sizeof(*area_copy));
+            memcpy(area_copy, area, sizeof(*area));
+            area = area_copy;
             cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
                                     QXL_IO_UPDATE_AREA_ASYNC,
-                                    0);
+                                    (uint64_t)area_copy);
         }
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
                                     clear_dirty_region, (uint64_t)cookie);
@@ -193,7 +197,7 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
 #if SPICE_INTERFACE_QXL_MINOR >= 1
 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,
                                  0));
@@ -765,6 +769,10 @@ 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_FLUSH_SURFACES_ASYNC:
+        break;
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
         break;
@@ -774,6 +782,12 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
     case QXL_IO_DESTROY_SURFACE_ASYNC:
         qxl_spice_destroy_surface_wait_complete(qxl, (uint32_t)cookie->data);
         break;
+    case QXL_IO_UPDATE_AREA_ASYNC:
+        g_free((void *)cookie->data);
+        break;
+    default:
+        fprintf(stderr, "qxl: %s: unexpected current_async %d\n", __func__,
+                current_async);
     }
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
@@ -788,6 +802,9 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     case QXL_COOKIE_TYPE_IO:
         interface_async_complete_io(qxl, 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);
     }
@@ -1243,8 +1260,18 @@ async_common:
     case QXL_IO_UPDATE_AREA:
     {
         QXLRect update = d->ram->update_area;
+        QXLRect *area_copy = &update;
+        QXLCookie *cookie = NULL;
+
+        if (async == QXL_ASYNC) {
+            area_copy = g_malloc0(sizeof(*area_copy));
+            memcpy(area_copy, &d->ram->update_area, sizeof(*area_copy));
+            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                    QXL_IO_UPDATE_AREA_ASYNC,
+                                    (uint64_t)area_copy);
+        }
         qxl_spice_update_area(d, d->ram->update_surface,
-                              &update, NULL, 0, 0, async, NULL);
+                              area_copy, NULL, 0, 0, async, cookie);
         break;
     }
     case QXL_IO_NOTIFY_CMD:
diff --git a/ui/spice-display.h b/ui/spice-display.h
index c8747ab..8f286f8 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 {
-- 
1.7.9

[Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

This changes the behavior of the monitor command. After the previous
patch, there is no longer an option of deadlock with virt-manager, but
ppm_save is called too early, before the update has completed. With this
patch it is called at the correct moment, but that means there is a race
between the monitor command completing and the screendump file being saved.

The only solution is to use an asynchronous monitor command. For a
previous round of this see:
 http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02810.html

Since that's contentious, I'm suggesting we do something that is almost
correct and doesn't hang, instead of correct and hangs. The screendump
user can inotify on the directory and the file if need be. For casual
monitor usage there is no difference.
---
 hw/qxl-render.c |   27 +++++++++++++++++++++++----
 hw/qxl.c        |   15 +++++++++++----
 hw/qxl.h        |    2 +-
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 7f9fbca..7b120ab 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -75,15 +75,17 @@ typedef struct QXLRenderUpdateData {
     int redraw;
     QXLRect dirty[32];
     QXLRect area;
+    char *filename;
 } QXLRenderUpdateData;
 
-void qxl_render_update(PCIQXLDevice *qxl)
+void qxl_render_update(PCIQXLDevice *qxl, const char *filename)
 {
     VGACommonState *vga = &qxl->vga;
     QXLRect dirty[32];
     void *ptr;
     int redraw = 0;
     QXLRenderUpdateData *data;
+    QXLCookie *cookie;
 
     if (!is_buffer_shared(vga->ds->surface)) {
         dprint(qxl, 1, "%s: restoring shared displaysurface\n", __func__);
@@ -139,11 +141,24 @@ void qxl_render_update(PCIQXLDevice *qxl)
     data->area.right  = qxl->guest_primary.surface.width;
     data->area.top    = 0;
     data->area.bottom = qxl->guest_primary.surface.height;
+    if (filename) {
+        data->filename = g_strdup(filename);
+    }
+    cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
+                                         0,
+                                         (uint64_t)data);
+#if SPICE_INTERFACE_QXL_MINOR >= 1
     qxl_spice_update_area(qxl, 0, &data->area,
                           data->dirty, ARRAY_SIZE(dirty), 1, QXL_ASYNC,
-                          qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
-                                         0,
-                                         (uint64_t)data));
+                          cookie);
+#else
+    qxl_spice_update_area(qxl, 0, &data->area,
+                          data->dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL);
+    qxl_render_update_area_done(qxl, cookie);
+    if (filename) {
+        ppm_save(filename, qxl->ssd.ds->surface);
+    }
+#endif
 }
 
 void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
@@ -171,6 +186,10 @@ void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
                    dirty[i].right - dirty[i].left,
                    dirty[i].bottom - dirty[i].top);
     }
+    if (data->filename) {
+        ppm_save(data->filename, qxl->ssd.ds->surface);
+        g_free(data->filename);
+    }
     g_free(data);
 }
 
diff --git a/hw/qxl.c b/hw/qxl.c
index 5563293..2409cb3 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -145,12 +145,12 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            uint32_t clear_dirty_region,
                            qxl_async_io async, QXLCookie *cookie)
 {
-    struct QXLRect *area_copy;
     if (async == QXL_SYNC) {
         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 >= 1
+        struct QXLRect *area_copy;
         if (cookie == NULL) {
             area_copy = g_malloc0(sizeof(*area_copy));
             memcpy(area_copy, area, sizeof(*area));
@@ -1476,7 +1476,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;
@@ -1499,8 +1499,15 @@ 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);
+        /*
+         * TODO: if we use an async update_area, to avoid deadlock with
+         * virt-manager, we postpone the saving of the image until the
+         * rendering is done. This means the image isn't guranteed to be
+         * written when we return to the monitor. Fixing this needs an async
+         * monitor command, whatever the implementation of the concept is
+         * called.
+         */
+        qxl_render_update(qxl, filename);
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename);
diff --git a/hw/qxl.h b/hw/qxl.h
index 71d5c35..198abdc 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -132,7 +132,7 @@ 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, const char *filename);
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
 
 void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie);
-- 
1.7.9

[Qemu-devel] [RFC 6/7] qxl: use spice_qxl_update_area_dirty_async

[Alon Levy]

---
 hw/qxl.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 2409cb3..6e25bd1 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -150,17 +150,16 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                         dirty_rects, num_dirty_rects, clear_dirty_region);
     } else {
 #if SPICE_INTERFACE_QXL_MINOR >= 1
-        struct QXLRect *area_copy;
-        if (cookie == NULL) {
-            area_copy = g_malloc0(sizeof(*area_copy));
-            memcpy(area_copy, area, sizeof(*area));
-            area = area_copy;
-            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
-                                    QXL_IO_UPDATE_AREA_ASYNC,
-                                    (uint64_t)area_copy);
-        }
+        assert(cookie != NULL);
+#if SPICE_SERVER_VERSION >= 0x000a02
+        /* use dirty rectangles updating api introduced in 0.10.2 */
+        spice_qxl_update_area_dirty_async(&qxl->ssd.qxl, surface_id, area,
+                        dirty_rects, num_dirty_rects, clear_dirty_region,
+                        (uint64_t)cookie);
+#else
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
                                     clear_dirty_region, (uint64_t)cookie);
+#endif
 #else
         abort();
 #endif
-- 
1.7.9

[Qemu-devel] [RFC 7/7] qxl: add allocator

[Alon Levy]

Add an implementation of the DisplayAllocator callbacks for qxl. Uses
the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32
bits per pixel line convertion. Since free/resize/create are defined in
qxl.c, it is easy to ensure consistent usage of the flag (it means
QXL_ALLOCATED basically).

With this patch and the previous two screendump works for vga and qxl
modes when using qxl and spice together. This might break sdl/vnc with
spice, untested since it isn't of known use.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |   34 +-----------------
 hw/qxl.c           |  103 +++++++++++++++++++++++++++++++++++++++++++++++++---
 ui/spice-display.c |    1 +
 ui/spice-display.h |    2 +
 4 files changed, 101 insertions(+), 39 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 7b120ab..8280b91 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -82,7 +82,6 @@ void qxl_render_update(PCIQXLDevice *qxl, const char *filename)
 {
     VGACommonState *vga = &qxl->vga;
     QXLRect dirty[32];
-    void *ptr;
     int redraw = 0;
     QXLRenderUpdateData *data;
     QXLCookie *cookie;
@@ -96,38 +95,7 @@ void qxl_render_update(PCIQXLDevice *qxl, const char *filename)
 
     if (qxl->guest_primary.resized) {
         qxl->guest_primary.resized = 0;
-
-        if (qxl->guest_primary.flipped) {
-            g_free(qxl->guest_primary.flipped);
-            qxl->guest_primary.flipped = NULL;
-        }
-        qemu_free_displaysurface(vga->ds);
-
-        qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
-        if (qxl->guest_primary.qxl_stride < 0) {
-            /* spice surface is upside down -> need extra buffer to flip */
-            qxl->guest_primary.flipped =
-                g_malloc(qxl->guest_primary.surface.width *
-                         qxl->guest_primary.abs_stride);
-            ptr = qxl->guest_primary.flipped;
-        } else {
-            ptr = qxl->guest_primary.data;
-        }
-        dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
-               __FUNCTION__,
-               qxl->guest_primary.surface.width,
-               qxl->guest_primary.surface.height,
-               qxl->guest_primary.qxl_stride,
-               qxl->guest_primary.bytes_pp,
-               qxl->guest_primary.bits_pp,
-               qxl->guest_primary.flipped ? "yes" : "no");
-        vga->ds->surface =
-            qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
-                                            qxl->guest_primary.surface.height,
-                                            qxl->guest_primary.bits_pp,
-                                            qxl->guest_primary.abs_stride,
-                                            ptr);
-        dpy_resize(vga->ds);
+        qemu_resize_displaysurface(qxl->vga.ds, 0, 0);
     }
 
     if (!qxl->guest_primary.commands) {
diff --git a/hw/qxl.c b/hw/qxl.c
index 6e25bd1..ec43c4e 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -138,6 +138,92 @@ void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
     }
 }
 
+static DisplaySurface *qxl_create_displaysurface(int width, int height)
+{
+    PCIQXLDevice *qxl = qxl0;
+    DisplaySurface *ret;
+    void *ptr;
+
+    dprint(qxl, 1, "%s: %d x %d (have %d, %d)\n", __func__,
+           width, height,
+           qxl->guest_primary.surface.width,
+           qxl->guest_primary.surface.height);
+    if (width != 0 && height != 0 &&
+        (qxl->mode == QXL_MODE_VGA || qxl->mode == QXL_MODE_UNDEFINED)) {
+        /* initial surface creation in vga mode, use give width and height */
+        qxl->guest_primary.surface.width = width;
+        qxl->guest_primary.surface.height = height;
+        qxl->guest_primary.qxl_stride = width * 4;
+        qxl->guest_primary.abs_stride = width * 4;
+        qxl->guest_primary.bytes_pp = 4;
+        qxl->guest_primary.bits_pp = 32;
+        qxl->guest_primary.flipped = 0;
+        qxl->guest_primary.data = qxl->ssd.buf;
+    } else {
+        if (qxl->guest_primary.flipped) {
+            g_free(qxl->guest_primary.flipped);
+            qxl->guest_primary.flipped = NULL;
+        }
+        qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
+    }
+    if (qxl->guest_primary.qxl_stride < 0) {
+        /* spice surface is upside down -> need extra buffer to flip */
+        qxl->guest_primary.flipped =
+            g_malloc(qxl->guest_primary.surface.width *
+                     qxl->guest_primary.abs_stride);
+        ptr = qxl->guest_primary.flipped;
+    } else {
+        ptr = qxl->guest_primary.data;
+    }
+    dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
+           __func__,
+           qxl->guest_primary.surface.width,
+           qxl->guest_primary.surface.height,
+           qxl->guest_primary.qxl_stride,
+           qxl->guest_primary.bytes_pp,
+           qxl->guest_primary.bits_pp,
+           qxl->guest_primary.flipped ? "yes" : "no");
+    ret = qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
+                                          qxl->guest_primary.surface.height,
+                                          qxl->guest_primary.bits_pp,
+                                          qxl->guest_primary.abs_stride,
+                                          ptr);
+    /*
+     * set this flag to ensure vga_draw_graphic treats this as a non shared
+     * buffer, and so does the correct convertion to 32 bits that we always use.
+     */
+    ret->flags |= QEMU_ALLOCATED_FLAG;
+    return ret;
+}
+
+static void qxl_free_displaysurface(DisplaySurface *surface)
+{
+    g_free(surface);
+}
+
+static DisplaySurface *qxl_resize_displaysurface(DisplaySurface *surface,
+                                                 int width, int height)
+{
+    if (surface != qxl0->vga.ds->surface) {
+        qxl_free_displaysurface(surface);
+    }
+    return qxl_create_displaysurface(width, height);
+}
+
+static struct DisplayAllocator qxl_displaysurface_allocator = {
+    qxl_create_displaysurface,
+    qxl_resize_displaysurface,
+    qxl_free_displaysurface
+};
+
+void qxl_set_allocator(DisplayState *ds)
+{
+    if (register_displayallocator(ds, &qxl_displaysurface_allocator) ==
+            &qxl_displaysurface_allocator) {
+        printf("qxl_set_allocator success\n");
+        dpy_resize(ds);
+    }
+}
 
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
@@ -1493,9 +1579,18 @@ static void qxl_hw_invalidate(void *opaque)
 static void qxl_hw_screen_dump(void *opaque, const char *filename)
 {
     PCIQXLDevice *qxl = opaque;
-    VGACommonState *vga = &qxl->vga;
 
     switch (qxl->mode) {
+    case QXL_MODE_VGA:
+        /*
+         * TODO: vga_hw_screen_dump needless does a number of ppm_save calls
+         * fix it instead of redoing it correctly here (needs testing which is
+         * why it isn't yet done)
+         */
+        qxl->vga.invalidate(&qxl->vga);
+        vga_hw_update();
+        ppm_save(filename, qxl->ssd.ds->surface);
+        break;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
         /*
@@ -1508,9 +1603,6 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename)
          */
         qxl_render_update(qxl, filename);
         break;
-    case QXL_MODE_VGA:
-        vga->screen_dump(vga, filename);
-        break;
     default:
         break;
     }
@@ -1682,9 +1774,8 @@ static int qxl_init_primary(PCIDevice *dev)
 
     vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
                                    qxl_hw_screen_dump, qxl_hw_text_update, qxl);
-    qemu_spice_display_init_common(&qxl->ssd, vga->ds);
-
     qxl0 = qxl;
+    qemu_spice_display_init_common(&qxl->ssd, vga->ds);
     register_displaychangelistener(vga->ds, &display_listener);
 
     return qxl_init_common(qxl);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 680e6f4..7fd18f4 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -295,6 +295,7 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
     ssd->mouse_y = -1;
     ssd->bufsize = (16 * 1024 * 1024);
     ssd->buf = g_malloc(ssd->bufsize);
+    qxl_set_allocator(ds);
 }
 
 /* display listener callbacks */
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 8f286f8..9f24097 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -123,3 +123,5 @@ void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
 void qemu_spice_wakeup(SimpleSpiceDisplay *ssd);
 void qemu_spice_start(SimpleSpiceDisplay *ssd);
 void qemu_spice_stop(SimpleSpiceDisplay *ssd);
+
+void qxl_set_allocator(DisplayState *ds);
-- 
1.7.9

Re: [Qemu-devel] [RFC 3/7] qxl: introduce QXLCookie

[Gerd Hoffmann]

> +        if (cookie == NULL) {
> +            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> +                                    QXL_IO_UPDATE_AREA_ASYNC,
> +                                    0);
> +        }

Automagic cookie creation is still there.
I think when cookie is NULL you should just pass it on ...

> -/* called from spice server thread context only */
> -static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
> +static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
>  {
> -    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>      uint32_t current_async;

... and do "if (cookie == NULL) return;" here.

Which spice server version added async support?  IIRC this is 0.8.2?

I'm tempted to raise the minimal supported version to 0.8.latest and zap
a bunch of #ifdefs from the code.  What do you think?

cheers,
  Gerd

Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async

[Gerd Hoffmann]

  Hi,


> +void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
> +{

This is called from spice server thread context, correct?

> -    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
> +    for (i = 0; i < ARRAY_SIZE(data->dirty); i++) {
>          if (qemu_spice_rect_is_empty(dirty+i)) {
>              break;
>          }
> @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl)

dpy_update() call here.  Calling that one isn't safe without grabbing
the qemu lock.

>                     dirty[i].right - dirty[i].left,
>                     dirty[i].bottom - dirty[i].top);
>      }

> @@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
>                             uint32_t clear_dirty_region,
>                             qxl_async_io async, QXLCookie *cookie)
>  {
> +    struct QXLRect *area_copy;
>      if (async == QXL_SYNC) {
>          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 >= 1
>          if (cookie == NULL) {
> +            area_copy = g_malloc0(sizeof(*area_copy));
> +            memcpy(area_copy, area, sizeof(*area));
> +            area = area_copy;
>              cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
>                                      QXL_IO_UPDATE_AREA_ASYNC,
> -                                    0);
> +                                    (uint64_t)area_copy);

I still think this is the wrong place.

Also: How about making removing QXLCookie->data and adding a union
instead?  It's not like we have to transparently pass through a pointer
for someone else, it's our own state data, so this extra indirection
doesn't make sense at all.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Gerd Hoffmann]

On 02/19/12 22:28, Alon Levy wrote:
> This changes the behavior of the monitor command. After the previous
> patch, there is no longer an option of deadlock with virt-manager, but
> ppm_save is called too early, before the update has completed. With this
> patch it is called at the correct moment, but that means there is a race
> between the monitor command completing and the screendump file being saved.
> 
> The only solution is to use an asynchronous monitor command. For a
> previous round of this see:
>  http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02810.html
> 
> Since that's contentious, I'm suggesting we do something that is almost
> correct and doesn't hang, instead of correct and hangs. The screendump
> user can inotify on the directory and the file if need be. For casual
> monitor usage there is no difference.

Hmm, that is pretty lame.  There are users like autotest which expect
the screen dump being there when the monitor command is finished, that
change will break them.

Unfortunaly there is no easy way out.  I think the options are:

 (1) Keep existing behavior.  That means the screenshot might show old
     screen content.  Not very nice too.  Would work sort-of ok for
     autotest though as autotest does screenshots every second and thus
     the screen content wouldn't be older than a second.

 (2) Async monitor command.  Keeps interface and works nicely.  A bunch
     of QAPI bits tickled into master meanwhile, so we could look at
     this again.  Luiz?  What is the status here?

 (3) Something like this patch + additionally introduce a
     "your-screenshot-is-finished-now" qmp event.  Will break existing
     users too.  But at least they can be adapted without requiring
     some external, nonportable service like inotify ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Gerd Hoffmann]

On 02/19/12 22:28, Alon Levy wrote:
> Add an implementation of the DisplayAllocator callbacks for qxl. Uses
> the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32
> bits per pixel line convertion. Since free/resize/create are defined in
> qxl.c, it is easy to ensure consistent usage of the flag (it means
> QXL_ALLOCATED basically).
> 
> With this patch and the previous two screendump works for vga and qxl
> modes when using qxl and spice together. This might break sdl/vnc with
> spice, untested since it isn't of known use.

Breaking vnc+spice being used in parallel breaking is exactly what I
suspect might be possible.  IIRC there where discussions about just
enabling both vnc+spice, then let users pick what to use, so I'd prefer
to not break this.

Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 3/7] qxl: introduce QXLCookie

[Alon Levy]

On Mon, Feb 20, 2012 at 11:56:58AM +0100, Gerd Hoffmann wrote:
> > +        if (cookie == NULL) {
> > +            cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> > +                                    QXL_IO_UPDATE_AREA_ASYNC,
> > +                                    0);
> > +        }
> 
> Automagic cookie creation is still there.
> I think when cookie is NULL you should just pass it on ...

My bad, I thought I zapped it. I've fixed some issues, now it runs with
vnc and sdl fine (with my DisplayAllocation patch, didn't see yet if you
had any comments on it), so I'll make sure to remove this before sending
the next RFC.

> 
> > -/* called from spice server thread context only */
> > -static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
> > +static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
> >  {
> > -    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> >      uint32_t current_async;
> 
> ... and do "if (cookie == NULL) return;" here.

I can do that too.

> 
> Which spice server version added async support?  IIRC this is 0.8.2?
> 
> I'm tempted to raise the minimal supported version to 0.8.latest and zap
> a bunch of #ifdefs from the code.  What do you think?
> 

How does that work? raise the minimal supported version in configure?
and in RHEL we would do that as well? should be fine. And solves
headaches of testing this with multiple spice-server versions. I'll do
it.

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async

[Alon Levy]

On Mon, Feb 20, 2012 at 12:10:38PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> 
> > +void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
> > +{
> 
> This is called from spice server thread context, correct?
> 
> > -    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
> > +    for (i = 0; i < ARRAY_SIZE(data->dirty); i++) {
> >          if (qemu_spice_rect_is_empty(dirty+i)) {
> >              break;
> >          }
> > @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl)
> 
> dpy_update() call here.  Calling that one isn't safe without grabbing
> the qemu lock.

About dpy_update, discovered it the hard way. You mean I need the lock
for dpy_update or also before?

> 
> >                     dirty[i].right - dirty[i].left,
> >                     dirty[i].bottom - dirty[i].top);
> >      }
> 
> > @@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
> >                             uint32_t clear_dirty_region,
> >                             qxl_async_io async, QXLCookie *cookie)
> >  {
> > +    struct QXLRect *area_copy;
> >      if (async == QXL_SYNC) {
> >          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 >= 1
> >          if (cookie == NULL) {
> > +            area_copy = g_malloc0(sizeof(*area_copy));
> > +            memcpy(area_copy, area, sizeof(*area));
> > +            area = area_copy;
> >              cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> >                                      QXL_IO_UPDATE_AREA_ASYNC,
> > -                                    0);
> > +                                    (uint64_t)area_copy);
> 
> I still think this is the wrong place.

Yes, I agree, I thought I removed this already, I'll fix.

> 
> Also: How about making removing QXLCookie->data and adding a union
> instead?  It's not like we have to transparently pass through a pointer
> for someone else, it's our own state data, so this extra indirection
> doesn't make sense at all.

ok, will do.

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Mon, Feb 20, 2012 at 12:32:44PM +0100, Gerd Hoffmann wrote:
> On 02/19/12 22:28, Alon Levy wrote:
> > This changes the behavior of the monitor command. After the previous
> > patch, there is no longer an option of deadlock with virt-manager, but
> > ppm_save is called too early, before the update has completed. With this
> > patch it is called at the correct moment, but that means there is a race
> > between the monitor command completing and the screendump file being saved.
> > 
> > The only solution is to use an asynchronous monitor command. For a
> > previous round of this see:
> >  http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02810.html
> > 
> > Since that's contentious, I'm suggesting we do something that is almost
> > correct and doesn't hang, instead of correct and hangs. The screendump
> > user can inotify on the directory and the file if need be. For casual
> > monitor usage there is no difference.
> 
> Hmm, that is pretty lame.  There are users like autotest which expect
> the screen dump being there when the monitor command is finished, that
> change will break them.
> 
> Unfortunaly there is no easy way out.  I think the options are:
> 
>  (1) Keep existing behavior.  That means the screenshot might show old
>      screen content.  Not very nice too.  Would work sort-of ok for
>      autotest though as autotest does screenshots every second and thus
>      the screen content wouldn't be older than a second.
> 
>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
>      of QAPI bits tickled into master meanwhile, so we could look at
>      this again.  Luiz?  What is the status here?
> 
>  (3) Something like this patch + additionally introduce a
>      "your-screenshot-is-finished-now" qmp event.  Will break existing
>      users too.  But at least they can be adapted without requiring
>      some external, nonportable service like inotify ...
> 

I was going to look for QAPI bits after this series (i.e. (2)). Doing
(3) is also possible.

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Alon Levy]

On Mon, Feb 20, 2012 at 12:41:09PM +0100, Gerd Hoffmann wrote:
> On 02/19/12 22:28, Alon Levy wrote:
> > Add an implementation of the DisplayAllocator callbacks for qxl. Uses
> > the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32
> > bits per pixel line convertion. Since free/resize/create are defined in
> > qxl.c, it is easy to ensure consistent usage of the flag (it means
> > QXL_ALLOCATED basically).
> > 
> > With this patch and the previous two screendump works for vga and qxl
> > modes when using qxl and spice together. This might break sdl/vnc with
> > spice, untested since it isn't of known use.
> 
> Breaking vnc+spice being used in parallel breaking is exactly what I
> suspect might be possible.  IIRC there where discussions about just
> enabling both vnc+spice, then let users pick what to use, so I'd prefer
> to not break this.

I'll send a series that works with vnc+spice and sdl+spice (didn't test
sdl+vnc+spice), and screendumps at the same time.

> 
> Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too.

So you think I should introduce a new flag? I could introduce a
QEMU_DISPLAYSURFACE_MAX and then use QEMU_DISPLAY_SURFACE_MAX<<1 for my
own flag.

Actually I think the right thing is to move/copy the 24bit->32bit convertion
from vga.c to pflib.c, what do you think?

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 3/7] qxl: introduce QXLCookie

[Gerd Hoffmann]

  Hi,

>> Which spice server version added async support?  IIRC this is 0.8.2?
>>
>> I'm tempted to raise the minimal supported version to 0.8.latest and zap
>> a bunch of #ifdefs from the code.  What do you think?
>>
> 
> How does that work? raise the minimal supported version in configure?

Yes.  Then zap code handling older versions.  Maybe some cleanups
afterwards.

> and in RHEL we would do that as well? should be fine.

Probably, otherwise backporting becomes messy ;)

cheers,
  Gerd

Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async

[Gerd Hoffmann]

  Hi,

>>> @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl)
>>
>> dpy_update() call here.  Calling that one isn't safe without grabbing
>> the qemu lock.
> 
> About dpy_update, discovered it the hard way. You mean I need the lock
> for dpy_update or also before?

Any qemu code should be considered thread-unsafe unless proven
otherwise.  On a quick scan I havn't noticed anything but the
dpy_update() call.  It makes sense to wrap the whole loop though, so you
grab the lock only once.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Gerd Hoffmann]

  Hi,

>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
>>      of QAPI bits tickled into master meanwhile, so we could look at
>>      this again.  Luiz?  What is the status here?
> 
> I was going to look for QAPI bits after this series (i.e. (2)). Doing
> (3) is also possible.

Good.  I'd just leave it as-is then for now.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Gerd Hoffmann]

  Hi,

> I'll send a series that works with vnc+spice and sdl+spice (didn't test
> sdl+vnc+spice), and screendumps at the same time.
> 
>>
>> Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too.

QEMU_ALLOCATED_FLAG is just a flag which is used by the
defaultallocator_free_displaysurface function to figure whenever the
displaysurface memory was allocated by qemu_alloc_display or not.

I think vga.c shouldn't look at it in the first place, and faking it
because vga.c looks at it is even worse.

I'm also not sure it is the right approach to to have qxl register a
display allocator in the first place.  The only other place doing this
is sdl, which is a user interface.  None of the gfx card emulations in
the tree do that ...

> Actually I think the right thing is to move/copy the 24bit->32bit convertion
> from vga.c to pflib.c, what do you think?

Agree, although that easily gets a patch series of its own when you
collect optimized format conversion functions to move them over to pflib ...

BTW: qxl insisting on a shared displaysurface isn't very clean too, it
better should be able to fallback to just copying/converting for the
non-shared case.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Alon Levy]

On Mon, Feb 20, 2012 at 02:18:19PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > I'll send a series that works with vnc+spice and sdl+spice (didn't test
> > sdl+vnc+spice), and screendumps at the same time.
> > 
> >>
> >> Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too.
> 
> QEMU_ALLOCATED_FLAG is just a flag which is used by the
> defaultallocator_free_displaysurface function to figure whenever the
> displaysurface memory was allocated by qemu_alloc_display or not.
> 
> I think vga.c shouldn't look at it in the first place, and faking it
> because vga.c looks at it is even worse.
> 
> I'm also not sure it is the right approach to to have qxl register a
> display allocator in the first place.  The only other place doing this
> is sdl, which is a user interface.  None of the gfx card emulations in
> the tree do that ...

TBH I'm having problems without doing it with my own Allocator. In
particular calling ppm_save from spice_server thread seems to be a
problem. I can remove the hackish use of QEMU_ALLOCATED_FLAG by not
checking for it in vga.c in the first place, but I don't know what other
code it affects (well, I do - every machine using vga, but I'm not
testing all of these).

Also reallocating the displaysurface seems to be the wrong thing, and
that's what we do whenever we check in qxl_render_update
is_shared_buffer.

> 
> > Actually I think the right thing is to move/copy the 24bit->32bit convertion
> > from vga.c to pflib.c, what do you think?
> 
> Agree, although that easily gets a patch series of its own when you
> collect optimized format conversion functions to move them over to pflib ...

Yes, I think I'll defer that to later.

> 
> BTW: qxl insisting on a shared displaysurface isn't very clean too, it
> better should be able to fallback to just copying/converting for the
> non-shared case.

I'll try to get that working, although it seems to require having some
timer/bh to do the ppm_save.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Eric Blake]

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

On 02/20/2012 04:32 AM, Gerd Hoffmann wrote:
> Hmm, that is pretty lame.  There are users like autotest which expect
> the screen dump being there when the monitor command is finished, that
> change will break them.

Libvirt is another such user.

> 
> Unfortunaly there is no easy way out.  I think the options are:
> 
>  (1) Keep existing behavior.  That means the screenshot might show old
>      screen content.  Not very nice too.  Would work sort-of ok for
>      autotest though as autotest does screenshots every second and thus
>      the screen content wouldn't be older than a second.
> 
>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
>      of QAPI bits tickled into master meanwhile, so we could look at
>      this again.  Luiz?  What is the status here?
> 
>  (3) Something like this patch + additionally introduce a
>      "your-screenshot-is-finished-now" qmp event.  Will break existing
>      users too.  But at least they can be adapted without requiring
>      some external, nonportable service like inotify ...

Libvirt would want 3) - any command that becomes async also needs an
event to tell us when the command is completed, so that libvirt can
maintain the synchronous interface to the user (and/or expose a new flag
to allow the user to also benefit from the asynchronous command).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Gerd Hoffmann]

  Hi,

> TBH I'm having problems without doing it with my own Allocator. In
> particular calling ppm_save from spice_server thread seems to be a
> problem.

Grabbed the qemu mutex?

>> BTW: qxl insisting on a shared displaysurface isn't very clean too, it
>> better should be able to fallback to just copying/converting for the
>> non-shared case.
> 
> I'll try to get that working, although it seems to require having some
> timer/bh to do the ppm_save.

bh should do, that is one of the things they are supposed to handle.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Mon, Feb 20, 2012 at 02:29:11PM -0700, Eric Blake wrote:
> On 02/20/2012 04:32 AM, Gerd Hoffmann wrote:
> > Hmm, that is pretty lame.  There are users like autotest which expect
> > the screen dump being there when the monitor command is finished, that
> > change will break them.
> 
> Libvirt is another such user.
> 
> > 
> > Unfortunaly there is no easy way out.  I think the options are:
> > 
> >  (1) Keep existing behavior.  That means the screenshot might show old
> >      screen content.  Not very nice too.  Would work sort-of ok for
> >      autotest though as autotest does screenshots every second and thus
> >      the screen content wouldn't be older than a second.
> > 
> >  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> >      of QAPI bits tickled into master meanwhile, so we could look at
> >      this again.  Luiz?  What is the status here?
> > 
> >  (3) Something like this patch + additionally introduce a
> >      "your-screenshot-is-finished-now" qmp event.  Will break existing
> >      users too.  But at least they can be adapted without requiring
> >      some external, nonportable service like inotify ...
> 
> Libvirt would want 3) - any command that becomes async also needs an
> event to tell us when the command is completed, so that libvirt can
> maintain the synchronous interface to the user (and/or expose a new flag
> to allow the user to also benefit from the asynchronous command).

If I do 2) then libvirt won't notice because the monitor command will
block as usual. Only change would be internal, qemu would continue
processing other fds in the interim.

> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Alon Levy]

On Tue, Feb 21, 2012 at 08:57:54AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > TBH I'm having problems without doing it with my own Allocator. In
> > particular calling ppm_save from spice_server thread seems to be a
> > problem.
> 
> Grabbed the qemu mutex?

Didn't we remove qemu mutex grabbing from the spice server thread a long
long time ago? I've switched to using a bh, it seems to work better but
still not perfectly. I've also found my notes on why I tried the
allocator approach in the first place (all the following is in
QXL_NATIVE mode):

 Boot with qxl only, no vnc or sdl (so no qxl_render_update users except
 screendump).

 Issue screendump.

 Right now qxl_render_update checks if the displaysurface buffer is not
 shared, meaning it was allocated by qemu, and in this case it replaces
 it with the flipped buffer.

 But right after that surface->data gets reset, by vga_hw_screen_dump:
 vga_hw_screen_dump/console_select/qemu_resize_displaysurface/ds->allocator->resize_displaysurface/defaultallocator_resize_displaysurface/qemu_alloc_display

 Hence my line of thought that replacing the allocator with my own would
 prevent this. Since you have misgivings about using our own allocator
 that I don't know how to resolve, I'm instead doing a second
 reallocation in our dpy_resize callback qxl.c:display_resize, in affect
 it means that we have three allocations and three deallocations for every
 screendump. Do you still think it's less ugly then an allocator? note
 that I have sdl and vnc working with spice with my allocator scheme.
 (just didn't test all three together yet).

> 
> >> BTW: qxl insisting on a shared displaysurface isn't very clean too, it
> >> better should be able to fallback to just copying/converting for the
> >> non-shared case.
> > 
> > I'll try to get that working, although it seems to require having some
> > timer/bh to do the ppm_save.
> 
> bh should do, that is one of the things they are supposed to handle.
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Gerd Hoffmann]

  Hi,

>  Right now qxl_render_update checks if the displaysurface buffer is not
>  shared, meaning it was allocated by qemu, and in this case it replaces
>  it with the flipped buffer.

I think we should first reqire spice-server 0.8.latest, so
update_area_complete is available unconditionally.  Then do any
displaysurface updates in that callback (or a bh kicked by that
callback).  Handle both shared & non-shared cases.  I think we also can
get rid of the flip buffer then and just use a non-shared displaysurface
in that case (and flip the upside-down qxl surface while copying to the
qemu displaysurface).

>  But right after that surface->data gets reset, by vga_hw_screen_dump:
>  vga_hw_screen_dump/console_select/qemu_resize_displaysurface/ds->allocator->resize_displaysurface/defaultallocator_resize_displaysurface/qemu_alloc_display
> 
>  Hence my line of thought that replacing the allocator with my own would
>  prevent this. Since you have misgivings about using our own allocator
>  that I don't know how to resolve, I'm instead doing a second
>  reallocation in our dpy_resize callback qxl.c:display_resize, in affect
>  it means that we have three allocations and three deallocations for every
>  screendump. Do you still think it's less ugly then an allocator? note
>  that I have sdl and vnc working with spice with my allocator scheme.
>  (just didn't test all three together yet).

IMHO that calls for a patch like this to get rid of the pointless
console_select() call:

--- a/console.c
+++ b/console.c
@@ -181,12 +181,14 @@ void vga_hw_screen_dump(const char *filename)

     /* There is currently no way of specifying which screen we want to
dump,
        so always dump the first one.  */
-    console_select(0);
+    if (previous_active_console && previous_active_console->index != 0) {
+        console_select(0);
+    }
     if (consoles[0] && consoles[0]->hw_screen_dump) {
         consoles[0]->hw_screen_dump(consoles[0]->hw, filename);
     }

-    if (previous_active_console) {
+    if (previous_active_console && previous_active_console->index != 0) {
         console_select(previous_active_console->index);
     }
 }


cheers,
  Gerd

Re: [Qemu-devel] [RFC 7/7] qxl: add allocator

[Alon Levy]

On Tue, Feb 21, 2012 at 10:20:49AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >  Right now qxl_render_update checks if the displaysurface buffer is not
> >  shared, meaning it was allocated by qemu, and in this case it replaces
> >  it with the flipped buffer.
> 
> I think we should first reqire spice-server 0.8.latest, so
> update_area_complete is available unconditionally.  Then do any
> displaysurface updates in that callback (or a bh kicked by that
> callback).  Handle both shared & non-shared cases.  I think we also can
> get rid of the flip buffer then and just use a non-shared displaysurface
> in that case (and flip the upside-down qxl surface while copying to the
> qemu displaysurface).

ok, I'll try that...

> 
> >  But right after that surface->data gets reset, by vga_hw_screen_dump:
> >  vga_hw_screen_dump/console_select/qemu_resize_displaysurface/ds->allocator->resize_displaysurface/defaultallocator_resize_displaysurface/qemu_alloc_display
> > 
> >  Hence my line of thought that replacing the allocator with my own would
> >  prevent this. Since you have misgivings about using our own allocator
> >  that I don't know how to resolve, I'm instead doing a second
> >  reallocation in our dpy_resize callback qxl.c:display_resize, in affect
> >  it means that we have three allocations and three deallocations for every
> >  screendump. Do you still think it's less ugly then an allocator? note
> >  that I have sdl and vnc working with spice with my allocator scheme.
> >  (just didn't test all three together yet).
> 
> IMHO that calls for a patch like this to get rid of the pointless
> console_select() call:
> 
> --- a/console.c
> +++ b/console.c
> @@ -181,12 +181,14 @@ void vga_hw_screen_dump(const char *filename)
> 
>      /* There is currently no way of specifying which screen we want to
> dump,
>         so always dump the first one.  */
> -    console_select(0);
> +    if (previous_active_console && previous_active_console->index != 0) {
> +        console_select(0);
> +    }
>      if (consoles[0] && consoles[0]->hw_screen_dump) {
>          consoles[0]->hw_screen_dump(consoles[0]->hw, filename);
>      }
> 
> -    if (previous_active_console) {
> +    if (previous_active_console && previous_active_console->index != 0) {
>          console_select(previous_active_console->index);
>      }
>  }

...and that. Thanks.

> 
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Eric Blake]

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

On 02/21/2012 01:19 AM, Alon Levy wrote:

>>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
>>>      of QAPI bits tickled into master meanwhile, so we could look at
>>>      this again.  Luiz?  What is the status here?
>>>
>>>  (3) Something like this patch + additionally introduce a
>>>      "your-screenshot-is-finished-now" qmp event.  Will break existing
>>>      users too.  But at least they can be adapted without requiring
>>>      some external, nonportable service like inotify ...
>>
>> Libvirt would want 3) - any command that becomes async also needs an
>> event to tell us when the command is completed, so that libvirt can
>> maintain the synchronous interface to the user (and/or expose a new flag
>> to allow the user to also benefit from the asynchronous command).
> 
> If I do 2) then libvirt won't notice because the monitor command will
> block as usual. Only change would be internal, qemu would continue
> processing other fds in the interim.

I guess I misunderstood things then.  I was assuming that an async
monitor command would mean that the monitor command would return control
to libvirt prior to the screenshot file being completely written; but
libvirt promises a synchronous interface to callers (that is, a caller
using virDomainScreenshot won't get a response until the screenshot file
has started streaming, but that means the file had better be consistent,
and not something that gets modified after libvirt has streamed it to
the caller).  I don't particularly care which solution we have, as long
as the overall result is still something where libvirt has guarantees
that the complete screenshot file is available before libvirt then hands
control of that file back to the caller.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote:
> On 02/21/2012 01:19 AM, Alon Levy wrote:
> 
> >>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> >>>      of QAPI bits tickled into master meanwhile, so we could look at
> >>>      this again.  Luiz?  What is the status here?
> >>>
> >>>  (3) Something like this patch + additionally introduce a
> >>>      "your-screenshot-is-finished-now" qmp event.  Will break existing
> >>>      users too.  But at least they can be adapted without requiring
> >>>      some external, nonportable service like inotify ...
> >>
> >> Libvirt would want 3) - any command that becomes async also needs an
> >> event to tell us when the command is completed, so that libvirt can
> >> maintain the synchronous interface to the user (and/or expose a new flag
> >> to allow the user to also benefit from the asynchronous command).
> > 
> > If I do 2) then libvirt won't notice because the monitor command will
> > block as usual. Only change would be internal, qemu would continue
> > processing other fds in the interim.
> 
> I guess I misunderstood things then.  I was assuming that an async
> monitor command would mean that the monitor command would return control
> to libvirt prior to the screenshot file being completely written; but
> libvirt promises a synchronous interface to callers (that is, a caller
> using virDomainScreenshot won't get a response until the screenshot file
> has started streaming, but that means the file had better be consistent,
> and not something that gets modified after libvirt has streamed it to
> the caller).  I don't particularly care which solution we have, as long
> as the overall result is still something where libvirt has guarantees
> that the complete screenshot file is available before libvirt then hands
> control of that file back to the caller.

Yes, that's the misunderstanding I think everyone is liable to have
because it is called "Asyncronous", but in actuallity the implementation
of an async monitor command is just what I mentioned: internal to qemu
the main thread select loop continuous to run until a callback completes
the monitor command. The monitor is suspended during that time, so no
change to the monitor user (i.e. libvirt) is visible.

Luiz said that this interface is going to be dropped, so we don't want
to introduce another user to it now. So the question becomes if there is
something equivalent. I totally agree the name "async monitor" should be
resereved for the behavior you describe above, and not for the one I
just described. In that case there may still be room for an improvement
to the monitor commands, maybe only selectively used to not force a lot
of code churn, that will allow any command that requires some long
computation / operation to take place before returning to the caller
(synchronously from it's point of view), while still being responsive by
handling any other callbacks that have nothing to do with the monitor in
the mean time. Something identical in practice to what is correcntly
called "async monitor".

> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Luiz Capitulino]

On Tue, 21 Feb 2012 19:40:16 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote:
> > On 02/21/2012 01:19 AM, Alon Levy wrote:
> > 
> > >>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> > >>>      of QAPI bits tickled into master meanwhile, so we could look at
> > >>>      this again.  Luiz?  What is the status here?

The qapi infra is already in place for sometime now, but it doesn't support
async commands yet. However, we're accepting a combination of command + async
event on completion for commands that have to be async.

We'll eventually have a good interface for async support, but it's not likely
we'll have it for 1.1 (possible, but unlikely).

I think item 2 above is a good way to go, considering it will add a new command,
of course.

> Luiz said that this interface is going to be dropped, so we don't want
> to introduce another user to it now.

Please don't :)

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote:
> On Tue, 21 Feb 2012 19:40:16 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote:
> > > On 02/21/2012 01:19 AM, Alon Levy wrote:
> > > 
> > > >>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> > > >>>      of QAPI bits tickled into master meanwhile, so we could look at
> > > >>>      this again.  Luiz?  What is the status here?
> 
> The qapi infra is already in place for sometime now, but it doesn't support
> async commands yet. However, we're accepting a combination of command + async
> event on completion for commands that have to be async.
> 
> We'll eventually have a good interface for async support, but it's not likely
> we'll have it for 1.1 (possible, but unlikely).
> 
> I think item 2 above is a good way to go, considering it will add a new command,
> of course.
> 

Ok, so that sounds good: I'll add an event, and later libvirt/autotest
can use it. But in that case I'll need to introduce a connection between
the command and the event, some id. That id will have to be generated by
the command issuer, so a new command with event id + complete event?

> > Luiz said that this interface is going to be dropped, so we don't want
> > to introduce another user to it now.
> 
> Please don't :)
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Luiz Capitulino]

On Wed, 22 Feb 2012 14:22:50 +0100
Alon Levy <alevy@redhat.com> wrote:

> On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote:
> > On Tue, 21 Feb 2012 19:40:16 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote:
> > > > On 02/21/2012 01:19 AM, Alon Levy wrote:
> > > > 
> > > > >>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> > > > >>>      of QAPI bits tickled into master meanwhile, so we could look at
> > > > >>>      this again.  Luiz?  What is the status here?
> > 
> > The qapi infra is already in place for sometime now, but it doesn't support
> > async commands yet. However, we're accepting a combination of command + async
> > event on completion for commands that have to be async.
> > 
> > We'll eventually have a good interface for async support, but it's not likely
> > we'll have it for 1.1 (possible, but unlikely).
> > 
> > I think item 2 above is a good way to go, considering it will add a new command,
> > of course.
> > 
> 
> Ok, so that sounds good: I'll add an event, and later libvirt/autotest
> can use it. But in that case I'll need to introduce a connection between
> the command and the event, some id. That id will have to be generated by
> the command issuer, so a new command with event id + complete event?

That's a good question.

The only events we have today which are actually a response to an asynchronous
command are the block streaming API ones and they don't include an id.

Honestly, for this particular case, I'm not 100% sure that having an id is
_required_, as I don't expect a client to submit multiple screendump calls
in parallel and we don't "officially" support multiple QMP clients either.
Also, having the screendump filename in the event will serve as a form of
identifier too.

Btw, are you planning to add the event to the already existing screendump
command? Adding a new command that doesn't use the monitor async API and
is truly asynchronous wouldn't better?

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Gerd Hoffmann]

  Hi,

> Honestly, for this particular case, I'm not 100% sure that having an id is
> _required_, as I don't expect a client to submit multiple screendump calls
> in parallel and we don't "officially" support multiple QMP clients either.
> Also, having the screendump filename in the event will serve as a form of
> identifier too.

That is exactly my thinking, echo the filename written in the event.

> Btw, are you planning to add the event to the already existing screendump
> command? Adding a new command that doesn't use the monitor async API and
> is truly asynchronous wouldn't better?

Good question.  I'd tend to just let the existing command send trigger
an event.  But libvirt needs some way to figure whenever it should wait
for an event ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Wed, Feb 22, 2012 at 11:49:27AM -0200, Luiz Capitulino wrote:
> On Wed, 22 Feb 2012 14:22:50 +0100
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Wed, Feb 22, 2012 at 11:17:17AM -0200, Luiz Capitulino wrote:
> > > On Tue, 21 Feb 2012 19:40:16 +0200
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > On Tue, Feb 21, 2012 at 09:15:45AM -0700, Eric Blake wrote:
> > > > > On 02/21/2012 01:19 AM, Alon Levy wrote:
> > > > > 
> > > > > >>>  (2) Async monitor command.  Keeps interface and works nicely.  A bunch
> > > > > >>>      of QAPI bits tickled into master meanwhile, so we could look at
> > > > > >>>      this again.  Luiz?  What is the status here?
> > > 
> > > The qapi infra is already in place for sometime now, but it doesn't support
> > > async commands yet. However, we're accepting a combination of command + async
> > > event on completion for commands that have to be async.
> > > 
> > > We'll eventually have a good interface for async support, but it's not likely
> > > we'll have it for 1.1 (possible, but unlikely).
> > > 
> > > I think item 2 above is a good way to go, considering it will add a new command,
> > > of course.
> > > 
> > 
> > Ok, so that sounds good: I'll add an event, and later libvirt/autotest
> > can use it. But in that case I'll need to introduce a connection between
> > the command and the event, some id. That id will have to be generated by
> > the command issuer, so a new command with event id + complete event?
> 
> That's a good question.
> 
> The only events we have today which are actually a response to an asynchronous
> command are the block streaming API ones and they don't include an id.
> 
> Honestly, for this particular case, I'm not 100% sure that having an id is
> _required_, as I don't expect a client to submit multiple screendump calls
> in parallel and we don't "officially" support multiple QMP clients either.
> Also, having the screendump filename in the event will serve as a form of
> identifier too.
> 
> Btw, are you planning to add the event to the already existing screendump
> command? Adding a new command that doesn't use the monitor async API and
> is truly asynchronous wouldn't better?

I was thinking to add a new command since I'll want to add the id, and
if I'm already adding a new command I'll put in a display number too.

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Honestly, for this particular case, I'm not 100% sure that having an id is
> > _required_, as I don't expect a client to submit multiple screendump calls
> > in parallel and we don't "officially" support multiple QMP clients either.
> > Also, having the screendump filename in the event will serve as a form of
> > identifier too.
> 
> That is exactly my thinking, echo the filename written in the event.
> 
> > Btw, are you planning to add the event to the already existing screendump
> > command? Adding a new command that doesn't use the monitor async API and
> > is truly asynchronous wouldn't better?
> 
> Good question.  I'd tend to just let the existing command send trigger
> an event.  But libvirt needs some way to figure whenever it should wait
> for an event ...

Right, that's the second reason I think a new command is needed.
Additionally a new command can be implemented only by qxl and not by
anything else (although I guess that would be a NACK?)

> 
> cheers,
>   Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Gerd Hoffmann]

  Hi,

> I was thinking to add a new command since I'll want to add the id, and
> if I'm already adding a new command I'll put in a display number too.

Big question is what the display number is supposed to be ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Wed, Feb 22, 2012 at 03:47:08PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > I was thinking to add a new command since I'll want to add the id, and
> > if I'm already adding a new command I'll put in a display number too.
> 
> Big question is what the display number is supposed to be ...
> 

Ah, yes, it's not specified well enough. So I guess it should actually
be two parameters:
 device path - whatever we are calling it nowadays.
 device specific monitor number (or frame buffer id).

This should be good enough for current 4 pci qxl devices, and future
proof for a single qxl with multiple monitor outputs. Same goes for
s/qxl/any other framebuffer device/, I'm just not sure which are there
(although I'll find out a little by trying out the arm emulater for the
r-pi).

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Luiz Capitulino]

On Wed, 22 Feb 2012 15:29:33 +0100
Alon Levy <alevy@redhat.com> wrote:

> On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > Honestly, for this particular case, I'm not 100% sure that having an id is
> > > _required_, as I don't expect a client to submit multiple screendump calls
> > > in parallel and we don't "officially" support multiple QMP clients either.
> > > Also, having the screendump filename in the event will serve as a form of
> > > identifier too.
> > 
> > That is exactly my thinking, echo the filename written in the event.
> > 
> > > Btw, are you planning to add the event to the already existing screendump
> > > command? Adding a new command that doesn't use the monitor async API and
> > > is truly asynchronous wouldn't better?
> > 
> > Good question.  I'd tend to just let the existing command send trigger
> > an event.  But libvirt needs some way to figure whenever it should wait
> > for an event ...
> 
> Right, that's the second reason I think a new command is needed.
> Additionally a new command can be implemented only by qxl and not by
> anything else (although I guess that would be a NACK?)

Is there anything specific to qlx in the command? If there's, then you
should also consider making this a QOM device property. The downside is
that QOM commands are not going to be stable for 1.1.

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Alon Levy]

On Wed, Feb 22, 2012 at 01:55:45PM -0200, Luiz Capitulino wrote:
> On Wed, 22 Feb 2012 15:29:33 +0100
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > Honestly, for this particular case, I'm not 100% sure that having an id is
> > > > _required_, as I don't expect a client to submit multiple screendump calls
> > > > in parallel and we don't "officially" support multiple QMP clients either.
> > > > Also, having the screendump filename in the event will serve as a form of
> > > > identifier too.
> > > 
> > > That is exactly my thinking, echo the filename written in the event.
> > > 
> > > > Btw, are you planning to add the event to the already existing screendump
> > > > command? Adding a new command that doesn't use the monitor async API and
> > > > is truly asynchronous wouldn't better?
> > > 
> > > Good question.  I'd tend to just let the existing command send trigger
> > > an event.  But libvirt needs some way to figure whenever it should wait
> > > for an event ...
> > 
> > Right, that's the second reason I think a new command is needed.
> > Additionally a new command can be implemented only by qxl and not by
> > anything else (although I guess that would be a NACK?)
> 
> Is there anything specific to qlx in the command? If there's, then you
> should also consider making this a QOM device property. The downside is
> that QOM commands are not going to be stable for 1.1.

qxl is the only one that needs the async stuff since it needs to ask
spice-server to do the rendering, which is done in another thread
outside of qemu control. The rest of the screendump implementations
afaict don't need any such complexity.

The command itself would not be specific to qxl.

I thought I just need a QAPI command, and docs/ say QMP doesn't use that
yet, so what's the connection to QMP?

Re: [Qemu-devel] [RFC 5/7] qxl-render: call ppm_save on callback

[Luiz Capitulino]

On Wed, 22 Feb 2012 17:35:15 +0100
Alon Levy <alevy@redhat.com> wrote:

> On Wed, Feb 22, 2012 at 01:55:45PM -0200, Luiz Capitulino wrote:
> > On Wed, 22 Feb 2012 15:29:33 +0100
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Wed, Feb 22, 2012 at 03:22:11PM +0100, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > Honestly, for this particular case, I'm not 100% sure that having an id is
> > > > > _required_, as I don't expect a client to submit multiple screendump calls
> > > > > in parallel and we don't "officially" support multiple QMP clients either.
> > > > > Also, having the screendump filename in the event will serve as a form of
> > > > > identifier too.
> > > > 
> > > > That is exactly my thinking, echo the filename written in the event.
> > > > 
> > > > > Btw, are you planning to add the event to the already existing screendump
> > > > > command? Adding a new command that doesn't use the monitor async API and
> > > > > is truly asynchronous wouldn't better?
> > > > 
> > > > Good question.  I'd tend to just let the existing command send trigger
> > > > an event.  But libvirt needs some way to figure whenever it should wait
> > > > for an event ...
> > > 
> > > Right, that's the second reason I think a new command is needed.
> > > Additionally a new command can be implemented only by qxl and not by
> > > anything else (although I guess that would be a NACK?)
> > 
> > Is there anything specific to qlx in the command? If there's, then you
> > should also consider making this a QOM device property. The downside is
> > that QOM commands are not going to be stable for 1.1.
> 
> qxl is the only one that needs the async stuff since it needs to ask
> spice-server to do the rendering, which is done in another thread
> outside of qemu control. The rest of the screendump implementations
> afaict don't need any such complexity.

In theory, any file I/O should be asynchronous.

> The command itself would not be specific to qxl.
> 
> I thought I just need a QAPI command, and docs/ say QMP doesn't use that
> yet, so what's the connection to QMP?

Not sure I can follow you here, "that" what?