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

[Qemu-devel] [RFC v4 0/9] 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.

Changes from previous RFC (v4 to match my internal patches, it's actually the second):
 - dropped the DisplayAllocator
 - dropping flip (Gerd Hoffman suggestion)
 - dropping needless call to console_select (from Gerd)
 - use a bh for the bug fix itself, previously called dpy_update
  and wrote to vga->ds.surface->data from spice server thread, a no no.
 - last patch uses a bh for the ppm_save as well, same problem as one
  in the previous round. I'm going to work on a solution to that as discussed
  on the list (either a QAPI equivalent of the old async monitor or a new really
  async command with completion event that libvirt/autotest/etc would have to
  use), but didn't want to wait for that with the already prolonged wait from the last
  RFC.

Please review,

Alon Levy (8):
  sdl: remove NULL check, g_malloc0 can't fail
  qxl: drop qxl_spice_update_area_async definition
  qxl: screen_dump in vga: do a single ppm_save
  qxl: require spice >= 0.8.2
  qxl: remove flipped
  qxl: introduce QXLCookie
  qxl: make qxl_render_update async
  qxl-render: call ppm_save on bh

Gerd Hoffman (1):
  console: don't call console_select unnecessarily

 configure          |    2 +-
 console.c          |    6 +-
 hw/qxl-render.c    |  228 ++++++++++++++++++++++++++++++++++++++--------------
 hw/qxl.c           |  189 ++++++++++++++++++++++++++++++-------------
 hw/qxl.h           |   28 ++++---
 ui/sdl.c           |    4 -
 ui/spice-display.c |   34 +++++----
 ui/spice-display.h |   22 +++++
 8 files changed, 359 insertions(+), 154 deletions(-)

-- 
1.7.9.1

[Qemu-devel] [RFC v4 1/9] console: don't call console_select unnecessarily

[Alon Levy]

From: Gerd Hoffman <kraxel@redhat.com>

Tested-by: Alon Levy <alevy@redhat.com>
---
 console.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/console.c b/console.c
index 135394f..cfcc2f7 100644
--- 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);
     }
 }
-- 
1.7.9.1

[Qemu-devel] [RFC v4 2/9] 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.1

[Qemu-devel] [RFC v4 3/9] 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 |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/hw/qxl.h b/hw/qxl.h
index 766aa6d..6399c72 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -134,9 +134,3 @@ 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
-- 
1.7.9.1

[Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Alon Levy]

Using vga->screen_dump results in a number of calls to ppm_save,
instead of a single one. Lacking time to test all the possible users of
vga->screen_dump, avoid the redundant calls by doing the vga_hw_update+
ppm_save in qxl_hw_screen_dump.

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

diff --git a/hw/qxl.c b/hw/qxl.c
index ac69125..8bdc510 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1438,7 +1438,6 @@ 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_COMPAT:
@@ -1447,7 +1446,14 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename)
         ppm_save(filename, qxl->ssd.ds->surface);
         break;
     case QXL_MODE_VGA:
-        vga->screen_dump(vga, filename);
+        /*
+         * 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;
     default:
         break;
-- 
1.7.9.1

[Qemu-devel] [RFC v4 5/9] qxl: require spice >= 0.8.2

[Alon Levy]

drop all ifdefs on SPICE_INTERFACE_QXL_MINOR >= 1 as a result,
0.8.2 has SPICE_INTERFACE_QXL_MINOR == 1.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 configure          |    2 +-
 hw/qxl.c           |   40 ----------------------------------------
 hw/qxl.h           |    4 ----
 ui/spice-display.c |   12 ------------
 4 files changed, 1 insertions(+), 57 deletions(-)

diff --git a/configure b/configure
index 037f7f7..a1a5ac3 100755
--- a/configure
+++ b/configure
@@ -2536,7 +2536,7 @@ int main(void) { spice_server_new(); return 0; }
 EOF
   spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
   spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
-  if $pkg_config --atleast-version=0.6.0 spice-server >/dev/null 2>&1 && \
+  if $pkg_config --atleast-version=0.8.2 spice-server >/dev/null 2>&1 && \
      compile_prog "$spice_cflags" "$spice_libs" ; then
     spice="yes"
     libs_softmmu="$libs_softmmu $spice_libs"
diff --git a/hw/qxl.c b/hw/qxl.c
index 8bdc510..08e2275 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -125,9 +125,7 @@ static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 
 void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
-#endif
     if (qxl->guestdebug) {
         va_list ap;
         va_start(ap, msg);
@@ -149,12 +147,8 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
         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
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
                                     clear_dirty_region, 0);
-#else
-        abort();
-#endif
     }
 }
 
@@ -171,24 +165,18 @@ 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
         spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id,
                                         (uint64_t)id);
-#endif
     } else {
         qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
         qxl_spice_destroy_surface_wait_complete(qxl, 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);
 }
-#endif
 
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count)
@@ -217,11 +205,7 @@ 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();
-#else
         spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl, 0);
-#endif
     } else {
         qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
         qxl_spice_destroy_surfaces_complete(qxl);
@@ -490,7 +474,6 @@ static const char *io_port_to_string(uint32_t io_port)
         [QXL_IO_DESTROY_PRIMARY]        = "QXL_IO_DESTROY_PRIMARY",
         [QXL_IO_DESTROY_SURFACE_WAIT]   = "QXL_IO_DESTROY_SURFACE_WAIT",
         [QXL_IO_DESTROY_ALL_SURFACES]   = "QXL_IO_DESTROY_ALL_SURFACES",
-#if SPICE_INTERFACE_QXL_MINOR >= 1
         [QXL_IO_UPDATE_AREA_ASYNC]      = "QXL_IO_UPDATE_AREA_ASYNC",
         [QXL_IO_MEMSLOT_ADD_ASYNC]      = "QXL_IO_MEMSLOT_ADD_ASYNC",
         [QXL_IO_CREATE_PRIMARY_ASYNC]   = "QXL_IO_CREATE_PRIMARY_ASYNC",
@@ -500,7 +483,6 @@ static const char *io_port_to_string(uint32_t io_port)
                                         = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
         [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
         [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
-#endif
     };
     return io_port_to_string[io_port];
 }
@@ -735,8 +717,6 @@ static int interface_flush_resources(QXLInstance *sin)
 
 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)
 {
@@ -763,8 +743,6 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
 
-#endif
-
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -784,9 +762,7 @@ static const QXLInterface qxl_interface = {
     .req_cursor_notification = interface_req_cursor_notification,
     .notify_update           = interface_notify_update,
     .flush_resources         = interface_flush_resources,
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     .async_complete          = interface_async_complete,
-#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1136,9 +1112,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     PCIQXLDevice *d = opaque;
     uint32_t io_port = addr;
     qxl_async_io async = QXL_SYNC;
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     uint32_t orig_io_port = io_port;
-#endif
 
     switch (io_port) {
     case QXL_IO_RESET:
@@ -1148,10 +1122,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     case QXL_IO_CREATE_PRIMARY:
     case QXL_IO_UPDATE_IRQ:
     case QXL_IO_LOG:
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     case QXL_IO_MEMSLOT_ADD_ASYNC:
     case QXL_IO_CREATE_PRIMARY_ASYNC:
-#endif
         break;
     default:
         if (d->mode != QXL_MODE_VGA) {
@@ -1159,17 +1131,14 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
         }
         dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n",
             __func__, io_port, io_port_to_string(io_port));
-#if SPICE_INTERFACE_QXL_MINOR >= 1
         /* be nice to buggy guest drivers */
         if (io_port >= QXL_IO_UPDATE_AREA_ASYNC &&
             io_port <= QXL_IO_DESTROY_ALL_SURFACES_ASYNC) {
             qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
         }
-#endif
         return;
     }
 
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     /* we change the io_port to avoid ifdeffery in the main switch */
     orig_io_port = io_port;
     switch (io_port) {
@@ -1208,7 +1177,6 @@ async_common:
     default:
         break;
     }
-#endif
 
     switch (io_port) {
     case QXL_IO_UPDATE_AREA:
@@ -1300,7 +1268,6 @@ async_common:
         }
         qxl_spice_destroy_surface_wait(d, val, async);
         break;
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     case QXL_IO_FLUSH_RELEASE: {
         QXLReleaseRing *ring = &d->ram->release_ring;
         if (ring->prod - ring->cons + 1 == ring->num_items) {
@@ -1321,7 +1288,6 @@ async_common:
                d->num_free_res);
         qxl_spice_flush_surfaces_async(d);
         break;
-#endif
     case QXL_IO_DESTROY_ALL_SURFACES:
         d->mode = QXL_MODE_UNDEFINED;
         qxl_spice_destroy_surfaces(d, async);
@@ -1332,16 +1298,12 @@ async_common:
     }
     return;
 cancel_async:
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     if (async) {
         qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
         qemu_mutex_lock(&d->async_lock);
         d->current_async = QXL_UNDEFINED_IO;
         qemu_mutex_unlock(&d->async_lock);
     }
-#else
-    return;
-#endif
 }
 
 static uint64_t ioport_read(void *opaque, target_phys_addr_t addr,
@@ -1545,9 +1507,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     case 2: /* spice 0.6 -- qxl-2 */
         pci_device_rev = QXL_REVISION_STABLE_V06;
         break;
-#if SPICE_INTERFACE_QXL_MINOR >= 1
     case 3: /* qxl-3 */
-#endif
     default:
         pci_device_rev = QXL_DEFAULT_REVISION;
         break;
diff --git a/hw/qxl.h b/hw/qxl.h
index 6399c72..739ec27 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -104,11 +104,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#if SPICE_INTERFACE_QXL_MINOR >= 1
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
-#else
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V06
-#endif
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6c302a3..b6a62ed 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -64,11 +64,7 @@ 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);
-#else
-        abort();
-#endif
     } else {
         ssd->worker->add_memslot(ssd->worker, memslot);
     }
@@ -84,11 +80,7 @@ void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
                                        qxl_async_io async)
 {
     if (async != QXL_SYNC) {
-#if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_create_primary_surface_async(&ssd->qxl, id, surface, 0);
-#else
-        abort();
-#endif
     } else {
         ssd->worker->create_primary_surface(ssd->worker, id, surface);
     }
@@ -99,11 +91,7 @@ void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
                                         uint32_t id, qxl_async_io async)
 {
     if (async != QXL_SYNC) {
-#if SPICE_INTERFACE_QXL_MINOR >= 1
         spice_qxl_destroy_primary_surface_async(&ssd->qxl, id, 0);
-#else
-        abort();
-#endif
     } else {
         ssd->worker->destroy_primary_surface(ssd->worker, id);
     }
-- 
1.7.9.1

[Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c |   58 ++++++++++++++++++-------------------------------------
 hw/qxl.h        |    2 +-
 2 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 133d093..4518a56 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -23,10 +23,15 @@
 
 static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
 {
-    uint8_t *src = qxl->guest_primary.data;
-    uint8_t *dst = qxl->guest_primary.flipped;
+    uint8_t *src;
+    uint8_t *dst = qxl->vga.ds->surface->data;
     int len, i;
 
+    if (!qxl->guest_primary.data) {
+        dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__);
+        qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
+    }
+    src = qxl->guest_primary.data;
     src += (qxl->guest_primary.surface.height - rect->top - 1) *
         qxl->guest_primary.abs_stride;
     dst += rect->top  * qxl->guest_primary.abs_stride;
@@ -75,50 +80,20 @@ void qxl_render_update(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
     QXLRect dirty[32], update;
-    void *ptr;
     int i, redraw = 0;
-
-    if (!is_buffer_shared(vga->ds->surface)) {
-        dprint(qxl, 1, "%s: restoring shared displaysurface\n", __func__);
-        qxl->guest_primary.resized++;
-        qxl->guest_primary.commands++;
-        redraw = 1;
-    }
+    DisplaySurface *surface = vga->ds->surface;
 
     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",
+        dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d\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);
+               qxl->guest_primary.bits_pp);
     }
 
     if (!qxl->guest_primary.commands) {
@@ -138,14 +113,19 @@ void qxl_render_update(PCIQXLDevice *qxl)
         memset(dirty, 0, sizeof(dirty));
         dirty[0] = update;
     }
-
+    if (surface->width != qxl->guest_primary.surface.width ||
+        surface->height != qxl->guest_primary.surface.height) {
+        dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n",
+               __func__);
+        qemu_resize_displaysurface(vga->ds,
+                qxl->guest_primary.surface.width,
+                qxl->guest_primary.surface.height);
+    }
     for (i = 0; i < ARRAY_SIZE(dirty); i++) {
         if (qemu_spice_rect_is_empty(dirty+i)) {
             break;
         }
-        if (qxl->guest_primary.flipped) {
-            qxl_flip(qxl, dirty+i);
-        }
+        qxl_flip(qxl, dirty+i);
         dpy_update(vga->ds,
                    dirty[i].left, dirty[i].top,
                    dirty[i].right - dirty[i].left,
diff --git a/hw/qxl.h b/hw/qxl.h
index 739ec27..eadd016 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -52,7 +52,7 @@ typedef struct PCIQXLDevice {
         uint32_t       abs_stride;
         uint32_t       bits_pp;
         uint32_t       bytes_pp;
-        uint8_t        *data, *flipped;
+        uint8_t        *data;
     } guest_primary;
 
     struct surfaces {
-- 
1.7.9.1

[Qemu-devel] [RFC v4 7/9] 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           |   58 ++++++++++++++++++++++++++++++++++++++++-----------
 hw/qxl.h           |    2 +-
 ui/spice-display.c |   22 +++++++++++++++++--
 ui/spice-display.h |   14 ++++++++++++
 5 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 4518a56..0127856 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -108,7 +108,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 08e2275..6f94edb 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -141,14 +141,15 @@ 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 {
+        assert(cookie != NULL);
         spice_qxl_update_area_async(&qxl->ssd.qxl, surface_id, area,
-                                    clear_dirty_region, 0);
+                                    clear_dirty_region, (uint64_t)cookie);
     }
 }
 
@@ -164,9 +165,13 @@ static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl,
 static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
                                            qxl_async_io async)
 {
+    QXLCookie *cookie;
+
     if (async) {
-        spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id,
-                                        (uint64_t)id);
+        cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                QXL_IO_DESTROY_SURFACE_ASYNC);
+        cookie->u.surface_id = id;
+        spice_qxl_destroy_surface_async(&qxl->ssd.qxl, id, (uint64_t)cookie);
     } else {
         qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
         qxl_spice_destroy_surface_wait_complete(qxl, id);
@@ -175,7 +180,9 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id,
 
 static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl)
 {
-    spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, 0);
+ spice_qxl_flush_surfaces_async(&qxl->ssd.qxl,
+        (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                 QXL_IO_FLUSH_SURFACES_ASYNC));
 }
 
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
@@ -205,7 +212,9 @@ static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
 static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
 {
     if (async) {
-        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl, 0);
+        spice_qxl_destroy_surfaces_async(&qxl->ssd.qxl,
+                (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                         QXL_IO_DESTROY_ALL_SURFACES_ASYNC));
     } else {
         qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
         qxl_spice_destroy_surfaces_complete(qxl);
@@ -718,17 +727,24 @@ static int interface_flush_resources(QXLInstance *sin)
 static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
 
 /* 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 (!cookie) {
+        fprintf(stderr, "qxl: %s: error, cookie is NULL\n", __func__);
+        return;
+    }
+    if (cookie && 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);
@@ -737,12 +753,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, cookie->u.surface_id);
         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);
+}
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -1053,9 +1085,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);
@@ -1183,7 +1213,9 @@ 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,
+                              qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                             QXL_IO_UPDATE_AREA_ASYNC));
         break;
     }
     case QXL_IO_NOTIFY_CMD:
diff --git a/hw/qxl.h b/hw/qxl.h
index eadd016..ed297cc 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -114,7 +114,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 b6a62ed..7483c1a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -60,11 +60,23 @@ 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)
+{
+    QXLCookie *cookie;
+
+    cookie = g_malloc0(sizeof(*cookie));
+    cookie->type = type;
+    cookie->io = io;
+    return cookie;
+}
+
 void qemu_spice_add_memslot(SimpleSpiceDisplay *ssd, QXLDevMemSlot *memslot,
                             qxl_async_io async)
 {
     if (async != QXL_SYNC) {
-        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));
     } else {
         ssd->worker->add_memslot(ssd->worker, memslot);
     }
@@ -80,7 +92,9 @@ void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
                                        qxl_async_io async)
 {
     if (async != QXL_SYNC) {
-        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));
     } else {
         ssd->worker->create_primary_surface(ssd->worker, id, surface);
     }
@@ -91,7 +105,9 @@ void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
                                         uint32_t id, qxl_async_io async)
 {
     if (async != QXL_SYNC) {
-        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));
     } else {
         ssd->worker->destroy_primary_surface(ssd->worker, id);
     }
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 5e52df9..93ac78d 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -48,6 +48,20 @@ typedef enum qxl_async_io {
     QXL_ASYNC,
 } qxl_async_io;
 
+enum {
+    QXL_COOKIE_TYPE_IO,
+};
+
+typedef struct QXLCookie {
+    int      type;
+    uint64_t io;
+    union {
+        uint32_t surface_id;
+    } u;
+} QXLCookie;
+
+QXLCookie *qxl_cookie_new(int type, uint64_t io);
+
 typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
 typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
 
-- 
1.7.9.1

[Qemu-devel] [RFC v4 8/9] 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, as part of the QXLCookie.

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

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

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

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

[Qemu-devel] [RFC v4 9/9] qxl-render: call ppm_save on bh

[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.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |   60 ++++++++++++++++++++++++++++++++++++++++++++++++---
 hw/qxl.c           |   13 ++++++++--
 hw/qxl.h           |    2 +-
 ui/spice-display.h |    2 +
 4 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 23e6929..4b34c6f 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -78,6 +78,11 @@ void qxl_render_resize(PCIQXLDevice *qxl)
     }
 }
 
+typedef struct QXLPPMSaveBHData {
+    PCIQXLDevice *qxl;
+    QXLCookie *cookie;
+} QXLPPMSaveBHData;
+
 static void qxl_render_clear_update_redraw_unlocked(PCIQXLDevice *qxl)
 {
     qxl->render_update_redraw = 0;
@@ -86,6 +91,33 @@ static void qxl_render_clear_update_redraw_unlocked(PCIQXLDevice *qxl)
     qxl->num_dirty_rects = 0;
 }
 
+static void qxl_render_ppm_save_bh(void *opaque)
+{
+    QXLPPMSaveBHData *data = opaque;
+    PCIQXLDevice *qxl = data->qxl;
+    QXLCookie *cookie = data->cookie;
+    QEMUBH *bh = cookie->u.render.ppm_save_bh;
+    QXLRect area = {
+        .left = 0,
+        .right = qxl->guest_primary.surface.width,
+        .top = 0,
+        .bottom = qxl->guest_primary.surface.height
+    };
+
+    qemu_mutex_lock(&qxl->ssd.lock);
+    dprint(qxl, 1, "%s: %p (primary %p)\n", __func__,
+           qxl->ssd.ds->surface->data, qxl->guest_primary.data);
+    qxl_flip(qxl, &area);
+    qxl_render_clear_update_redraw_unlocked(qxl);
+    ppm_save(cookie->u.render.filename, qxl->ssd.ds->surface);
+    g_free(cookie->u.render.filename);
+    g_free(cookie);
+    --qxl->render_update_cookie_num;
+    g_free(data);
+    qemu_mutex_unlock(&qxl->ssd.lock);
+    qemu_bh_delete(bh);
+}
+
 static void qxl_displaysurface_resize(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
@@ -143,16 +175,17 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
  * callbacks are called by spice_server thread, defering to bh called from the
  * io thread.
  */
-void qxl_render_update(PCIQXLDevice *qxl)
+void qxl_render_update(PCIQXLDevice *qxl, const char *filename)
 {
     int redraw = 0;
     QXLCookie *cookie;
+    QEMUBH *ppm_save_bh;
+    QXLPPMSaveBHData *ppm_save_bh_data;
 
     qemu_mutex_lock(&qxl->ssd.lock);
 
     if (qxl->guest_primary.resized) {
         qxl->guest_primary.resized = 0;
-
         qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
         dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d\n",
                __FUNCTION__,
@@ -165,6 +198,12 @@ void qxl_render_update(PCIQXLDevice *qxl)
     }
 
     if (!qxl->guest_primary.commands) {
+        if (filename) {
+            dprint(qxl, 1, "%s: screendump with no pending commands\n",
+                   __func__);
+            qxl_render_update_area_unlocked(qxl);
+            ppm_save(filename, qxl->ssd.ds->surface);
+        }
         qemu_mutex_unlock(&qxl->ssd.lock);
         return;
     }
@@ -177,6 +216,14 @@ void qxl_render_update(PCIQXLDevice *qxl)
     cookie->u.render.area.top    = 0;
     cookie->u.render.area.bottom = qxl->guest_primary.surface.height;
     qxl->render_update_redraw_area = cookie->u.render.area;
+    if (filename) {
+        ppm_save_bh_data = g_malloc0(sizeof(*ppm_save_bh_data));
+        ppm_save_bh_data->qxl = qxl;
+        ppm_save_bh_data->cookie = cookie;
+        ppm_save_bh = qemu_bh_new(qxl_render_ppm_save_bh, ppm_save_bh_data);
+        cookie->u.render.filename = g_strdup(filename);
+        cookie->u.render.ppm_save_bh = ppm_save_bh;
+    }
     qxl->render_update_cookie_num++;
     qemu_mutex_unlock(&qxl->ssd.lock);
     qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL,
@@ -202,9 +249,14 @@ void qxl_render_update_area_bh(void *opaque)
 void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
 {
     qemu_mutex_lock(&qxl->ssd.lock);
-    qxl->render_update_cookie_num--;
+    if (cookie->u.render.filename) {
+        dprint(qxl, 1, "%s: scheduling ppm_save_bh\n", __func__);
+        qemu_bh_schedule(cookie->u.render.ppm_save_bh);
+    } else {
+        --qxl->render_update_cookie_num;
+        g_free(cookie);
+    }
     qemu_mutex_unlock(&qxl->ssd.lock);
-    g_free(cookie);
 }
 
 static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
diff --git a/hw/qxl.c b/hw/qxl.c
index b4f84f2..82f9ffa 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1480,7 +1480,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;
@@ -1502,8 +1502,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:
         /*
diff --git a/hw/qxl.h b/hw/qxl.h
index 57a94ca..91297bd 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -138,7 +138,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);
 void qxl_render_update_area_bh(void *opaque);
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 81fa1e4..076e0e5 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -62,6 +62,8 @@ typedef struct QXLCookie {
         struct {
             QXLRect area;
             int redraw;
+            char *filename;
+            QEMUBH *ppm_save_bh;
         } render;
     } u;
 } QXLCookie;
-- 
1.7.9.1

Re: [Qemu-devel] [Spice-devel] [RFC v4 0/9] qxl: fix hangs caused by qxl_render_update

[Alon Levy]

On Tue, Feb 21, 2012 at 11:39:28PM +0200, Alon Levy wrote:
> 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.
> 
> Changes from previous RFC (v4 to match my internal patches, it's actually the second):
Forgot:
 depend on spice-server >= 0.8.2

Last patch has an ugly hack in the ppm_save bh, that does flipping,
since I missed something somewhere.. Also the whole flipping assumes
that qxl always has negative stride and the displaysurface always has
positive stride, no checks anywhere..

>  - dropped the DisplayAllocator
>  - dropping flip (Gerd Hoffman suggestion)
>  - dropping needless call to console_select (from Gerd)
>  - use a bh for the bug fix itself, previously called dpy_update
>   and wrote to vga->ds.surface->data from spice server thread, a no no.
>  - last patch uses a bh for the ppm_save as well, same problem as one
>   in the previous round. I'm going to work on a solution to that as discussed
>   on the list (either a QAPI equivalent of the old async monitor or a new really
>   async command with completion event that libvirt/autotest/etc would have to
>   use), but didn't want to wait for that with the already prolonged wait from the last
>   RFC.
> 
> Please review,
> 
> Alon Levy (8):
>   sdl: remove NULL check, g_malloc0 can't fail
>   qxl: drop qxl_spice_update_area_async definition
>   qxl: screen_dump in vga: do a single ppm_save
>   qxl: require spice >= 0.8.2
>   qxl: remove flipped
>   qxl: introduce QXLCookie
>   qxl: make qxl_render_update async
>   qxl-render: call ppm_save on bh
> 
> Gerd Hoffman (1):
>   console: don't call console_select unnecessarily
> 
>  configure          |    2 +-
>  console.c          |    6 +-
>  hw/qxl-render.c    |  228 ++++++++++++++++++++++++++++++++++++++--------------
>  hw/qxl.c           |  189 ++++++++++++++++++++++++++++++-------------
>  hw/qxl.h           |   28 ++++---
>  ui/sdl.c           |    4 -
>  ui/spice-display.c |   34 +++++----
>  ui/spice-display.h |   22 +++++
>  8 files changed, 359 insertions(+), 154 deletions(-)
> 
> -- 
> 1.7.9.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Gerd Hoffmann]

On 02/21/12 22:39, Alon Levy wrote:
> Using vga->screen_dump results in a number of calls to ppm_save,
> instead of a single one.

Can you investigate why?

> Lacking time to test all the possible users of
> vga->screen_dump, avoid the redundant calls by doing the vga_hw_update+
> ppm_save in qxl_hw_screen_dump.

I'd prefer to fix the root cause instead of adding workarounds.

cheers,
  Gerd

Re: [Qemu-devel] [RFC v4 5/9] qxl: require spice >= 0.8.2

[Gerd Hoffmann]

On 02/21/12 22:39, Alon Levy wrote:
> drop all ifdefs on SPICE_INTERFACE_QXL_MINOR >= 1 as a result,
> 0.8.2 has SPICE_INTERFACE_QXL_MINOR == 1.

Nice.  I think there are a few more SPICE_SERVER_VERSION #ifdefs which
can be dropped too.  Also SPICE_INTERFACE_CORE_MINOR ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Gerd Hoffmann]

  Hi,

It's not obvious to me how the non-flipped case (qxl_stride > 0) is
handled now.  Have you tested this with both windows+linux guests?

cheers,
  Gerd

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

[Gerd Hoffmann]

On 02/21/12 22:39, Alon Levy wrote:
> Will be used in the next patch.

Looks good, and is more readable without all the #ifdefs.

>  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));

minor whitespace issue here ;)

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> +    qxl->render_update_redraw_area.left   = 0;
> +    qxl->render_update_redraw_area.right  =
> +        qxl->guest_primary.surface.width;
> +    qxl->render_update_redraw_area.top    = 0;
> +    qxl->render_update_redraw_area.bottom =
> +        qxl->guest_primary.surface.height;

Are there cases where render_update_redraw_area != full screen?

> +    qemu_mutex_lock(&qxl->ssd.lock);
> +    if (qxl->render_update_redraw) {
> +        /* don't bother copying them over since we will ignore them */
> +        qxl->num_dirty_rects += num_updated_rects;
> +        dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n",
> +               __func__, qxl->num_dirty_rects);
> +        qemu_bh_schedule(qxl->update_area_bh);
> +        qemu_mutex_unlock(&qxl->ssd.lock);
> +        return;
> +    }
> +    if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) {
> +        /*
> +         * overflow - merge all remaining rects. Hoping this is not
> +         * common so doesn't need to be optimized
> +         */
> +    }

Another easy way out is to simply set qxl->render_update_redraw = 1.

cheers,
  Gerd

Re: [Qemu-devel] [RFC v4 9/9] qxl-render: call ppm_save on bh

[Gerd Hoffmann]

On 02/21/12 22:39, 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.

I still think we should defer that and figure how to fix that properly,
either using (internally) async monitor commands via qapi, or using an
event.

> +typedef struct QXLPPMSaveBHData {
> +    PCIQXLDevice *qxl;
> +    QXLCookie *cookie;
> +} QXLPPMSaveBHData;

Is there a need for a separate struct?  I think you can just stick the
filename into the QXLCookie struct, then write out screen shots in the
update area bottom half, no?

cheers,
  Gerd

Re: [Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Alon Levy]

On Wed, Feb 22, 2012 at 12:10:08PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, Alon Levy wrote:
> > Using vga->screen_dump results in a number of calls to ppm_save,
> > instead of a single one.
> 
> Can you investigate why?

oh, I know why. vga_screen_dump implementation:

    screen_dump_filename = filename;
    vga_invalidate_display(s);
->  vga_hw_update();
    screen_dump_filename = NULL;

And the only user of screen_dump_filename is:

static void vga_save_dpy_update(DisplayState *ds,
                                int x, int y, int w, int h)
{
    if (screen_dump_filename) {
        ppm_save(screen_dump_filename, ds->surface);
    }
}

vga_save_dpy_update is called on any dpy_update, registered after the
first vga_screen_dump call.

Since there are a number of callers to dpy_update: vga_draw_text calls
it potentially once every line, vga_draw_graphic the same.
vga_draw_blank calls it once.

> 
> > Lacking time to test all the possible users of
> > vga->screen_dump, avoid the redundant calls by doing the vga_hw_update+
> > ppm_save in qxl_hw_screen_dump.
> 
> I'd prefer to fix the root cause instead of adding workarounds.
> 

This seems to work, only tested with -vga qxl and in vga mode:


diff --git a/hw/vga.c b/hw/vga.c
index c1029db..51f20c1 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -163,8 +163,6 @@ static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
 static void vga_screen_dump(void *opaque, const char *filename);
-static const char *screen_dump_filename;
-static DisplayChangeListener *screen_dump_dcl;
 
 static void vga_update_memory_access(VGACommonState *s)
 {
@@ -2364,22 +2362,6 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 /********************************************************/
 /* vga screen dump */
 
-static void vga_save_dpy_update(DisplayState *ds,
-                                int x, int y, int w, int h)
-{
-    if (screen_dump_filename) {
-        ppm_save(screen_dump_filename, ds->surface);
-    }
-}
-
-static void vga_save_dpy_resize(DisplayState *s)
-{
-}
-
-static void vga_save_dpy_refresh(DisplayState *s)
-{
-}
-
 int ppm_save(const char *filename, struct DisplaySurface *ds)
 {
     FILE *f;
@@ -2389,10 +2371,12 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
     uint8_t r, g, b;
     int ret;
     char *linebuf, *pbuf;
+    static int calls;
 
     f = fopen(filename, "wb");
     if (!f)
         return -1;
+    fprintf(stderr, "ppm_save %d\n", ++calls);
     fprintf(f, "P6\n%d %d\n%d\n",
             ds->width, ds->height, 255);
     linebuf = g_malloc(ds->width * 3);
@@ -2423,29 +2407,13 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
     return 0;
 }
 
-static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
-{
-    DisplayChangeListener *dcl;
-
-    dcl = g_malloc0(sizeof(DisplayChangeListener));
-    dcl->dpy_update = vga_save_dpy_update;
-    dcl->dpy_resize = vga_save_dpy_resize;
-    dcl->dpy_refresh = vga_save_dpy_refresh;
-    register_displaychangelistener(ds, dcl);
-    return dcl;
-}
-
 /* save the vga display in a PPM image even if no display is
    available */
 static void vga_screen_dump(void *opaque, const char *filename)
 {
     VGACommonState *s = opaque;
 
-    if (!screen_dump_dcl)
-        screen_dump_dcl = vga_screen_dump_init(s->ds);
-
-    screen_dump_filename = filename;
     vga_invalidate_display(s);
     vga_hw_update();
-    screen_dump_filename = NULL;
+    ppm_save(filename, s->ds->surface);
 }

> cheers,
>   Gerd

Re: [Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Alon Levy]

On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> It's not obvious to me how the non-flipped case (qxl_stride > 0) is
> handled now.  Have you tested this with both windows+linux guests?

It isn't handled. The simplest way is just to if on the stride and do a
single memcpy instead of individual line memcpy. This of course means we
are doing a redundant copy, since using our own DisplayAllocator or just
the existing deallocate + our own allocate of ds->surface->data removes
one copy. Since the main use case is screendump, I don't think it's a
big deal.

How does that sound?

> 
> cheers,
>   Gerd
> 
> 

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

[Alon Levy]

On Wed, Feb 22, 2012 at 12:41:01PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > +    qxl->render_update_redraw_area.left   = 0;
> > +    qxl->render_update_redraw_area.right  =
> > +        qxl->guest_primary.surface.width;
> > +    qxl->render_update_redraw_area.top    = 0;
> > +    qxl->render_update_redraw_area.bottom =
> > +        qxl->guest_primary.surface.height;
> 
> Are there cases where render_update_redraw_area != full screen?

I don't think so. I'll drop the area after making sure.

> 
> > +    qemu_mutex_lock(&qxl->ssd.lock);
> > +    if (qxl->render_update_redraw) {
> > +        /* don't bother copying them over since we will ignore them */
> > +        qxl->num_dirty_rects += num_updated_rects;
> > +        dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n",
> > +               __func__, qxl->num_dirty_rects);
> > +        qemu_bh_schedule(qxl->update_area_bh);
> > +        qemu_mutex_unlock(&qxl->ssd.lock);
> > +        return;
> > +    }
> > +    if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) {
> > +        /*
> > +         * overflow - merge all remaining rects. Hoping this is not
> > +         * common so doesn't need to be optimized
> > +         */
> > +    }
> 
> Another easy way out is to simply set qxl->render_update_redraw = 1.

Yes, I agree, wasn't sure what is better. It would avoid a lot of code.
I'll do it.

> 
> cheers,
>   Gerd

Re: [Qemu-devel] [RFC v4 9/9] qxl-render: call ppm_save on bh

[Alon Levy]

On Wed, Feb 22, 2012 at 12:46:28PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, 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.
> 
> I still think we should defer that and figure how to fix that properly,
> either using (internally) async monitor commands via qapi, or using an
> event.
> 
> > +typedef struct QXLPPMSaveBHData {
> > +    PCIQXLDevice *qxl;
> > +    QXLCookie *cookie;
> > +} QXLPPMSaveBHData;
> 
> Is there a need for a separate struct?  I think you can just stick the
> filename into the QXLCookie struct, then write out screen shots in the
> update area bottom half, no?

You don't get the cookie in the update_area bottom half. It's solemnly
for update_area_complete. It's not easy to associate a cookie with it,
since it may be called by the server without previous provocation from
qemu.

I'll take another look.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC v4 9/9] qxl-render: call ppm_save on bh

[Alon Levy]

On Wed, Feb 22, 2012 at 12:46:28PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, 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.
> 
> I still think we should defer that and figure how to fix that properly,
> either using (internally) async monitor commands via qapi, or using an
> event.

I'm looking at QAPI. But in general I think it's better to have a
correct image then a timely image. Without this patch you get an old
image, unless you do what autotest does and request one every few
seconds, then you won't really notice the difference (it would be one
frame offset, kinda).

> 
> > +typedef struct QXLPPMSaveBHData {
> > +    PCIQXLDevice *qxl;
> > +    QXLCookie *cookie;
> > +} QXLPPMSaveBHData;
> 
> Is there a need for a separate struct?  I think you can just stick the
> filename into the QXLCookie struct, then write out screen shots in the
> update area bottom half, no?
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Gerd Hoffmann]

> And the only user of screen_dump_filename is:
> 
> static void vga_save_dpy_update(DisplayState *ds,
>                                 int x, int y, int w, int h)
> {
>     if (screen_dump_filename) {
>         ppm_save(screen_dump_filename, ds->surface);

upstream/master this here:

          screen_dump_filename = NULL;

>     }
> }

> This seems to work, only tested with -vga qxl and in vga mode:

The corner case where this fails is when console switching is needed,
i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
command there ...

Re: [Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Gerd Hoffmann]

On 02/22/12 13:28, Alon Levy wrote:
> On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>> It's not obvious to me how the non-flipped case (qxl_stride > 0) is
>> handled now.  Have you tested this with both windows+linux guests?
> 
> It isn't handled. The simplest way is just to if on the stride and do a
> single memcpy instead of individual line memcpy.

Single memcpy works only for full scanlines.  qxl_flip can be extended
to handle both cases (and should probably also renamed then).

> This of course means we
> are doing a redundant copy,

No.  You can wrap the qxl_flip call into ...

  if (is_shared_buffer()) { ... }.

... to skip the copy if it isn't needed.

> since using our own DisplayAllocator or just
> the existing deallocate + our own allocate of ds->surface->data removes
> one copy.

I would just do

  if (qxl_stride > 0) {
     qemu_free_displaysurface
     qemu_create_displaysurface_from
  } else {
     qemu_resize_displaysurface
  }

cheers,
  Gerd

Re: [Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Alon Levy]

On Wed, Feb 22, 2012 at 03:09:09PM +0100, Gerd Hoffmann wrote:
> On 02/22/12 13:28, Alon Levy wrote:
> > On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >> It's not obvious to me how the non-flipped case (qxl_stride > 0) is
> >> handled now.  Have you tested this with both windows+linux guests?
> > 
> > It isn't handled. The simplest way is just to if on the stride and do a
> > single memcpy instead of individual line memcpy.
> 
> Single memcpy works only for full scanlines.  qxl_flip can be extended
> to handle both cases (and should probably also renamed then).
> 
> > This of course means we
> > are doing a redundant copy,
> 
> No.  You can wrap the qxl_flip call into ...
> 
>   if (is_shared_buffer()) { ... }.
> 
> ... to skip the copy if it isn't needed.
> 
> > since using our own DisplayAllocator or just
> > the existing deallocate + our own allocate of ds->surface->data removes
> > one copy.
> 
> I would just do
> 
>   if (qxl_stride > 0) {
>      qemu_free_displaysurface
>      qemu_create_displaysurface_from
>   } else {
>      qemu_resize_displaysurface
>   }

hmm. Yes, I know we can do that since it already does it right now. I
guess with the console_select call gone it should be ok.

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Alon Levy]

On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
> > And the only user of screen_dump_filename is:
> > 
> > static void vga_save_dpy_update(DisplayState *ds,
> >                                 int x, int y, int w, int h)
> > {
> >     if (screen_dump_filename) {
> >         ppm_save(screen_dump_filename, ds->surface);
> 
> upstream/master this here:
> 
>           screen_dump_filename = NULL;
> 

That's wrong, you'll get the screenshot after the first update, who's to
say it is fully rendered?

> >     }
> > }
> 
> > This seems to work, only tested with -vga qxl and in vga mode:
> 
> The corner case where this fails is when console switching is needed,
> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
> command there ...

Are you talking about sdl console or linux guest console? ctrl-alt-2 or
ctrl-alt-F2? I can try both.

> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [Spice-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Gerd Hoffmann]

On 02/22/12 15:25, Alon Levy wrote:
> On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
>>> And the only user of screen_dump_filename is:
>>>
>>> static void vga_save_dpy_update(DisplayState *ds,
>>>                                 int x, int y, int w, int h)
>>> {
>>>     if (screen_dump_filename) {
>>>         ppm_save(screen_dump_filename, ds->surface);
>>
>> upstream/master this here:
>>
>>           screen_dump_filename = NULL;
>>
> 
> That's wrong, you'll get the screenshot after the first update, who's to
> say it is fully rendered?

vga code actually does a single, fullscreen update after
vga_invalidate_display(), so it should work fine.

>> The corner case where this fails is when console switching is needed,
>> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
>> command there ...
> 
> Are you talking about sdl console or linux guest console? ctrl-alt-2 or
> ctrl-alt-F2? I can try both.

ctrl-alt-2 sdl console.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Alon Levy]

On Wed, Feb 22, 2012 at 03:37:46PM +0100, Gerd Hoffmann wrote:
> On 02/22/12 15:25, Alon Levy wrote:
> > On Wed, Feb 22, 2012 at 02:58:10PM +0100, Gerd Hoffmann wrote:
> >>> And the only user of screen_dump_filename is:
> >>>
> >>> static void vga_save_dpy_update(DisplayState *ds,
> >>>                                 int x, int y, int w, int h)
> >>> {
> >>>     if (screen_dump_filename) {
> >>>         ppm_save(screen_dump_filename, ds->surface);
> >>
> >> upstream/master this here:
> >>
> >>           screen_dump_filename = NULL;
> >>
> > 
> > That's wrong, you'll get the screenshot after the first update, who's to
> > say it is fully rendered?
> 
> vga code actually does a single, fullscreen update after
> vga_invalidate_display(), so it should work fine.

ok, so I'll just use your one liner and see.

> 
> >> The corner case where this fails is when console switching is needed,
> >> i.e. switch to monitor console via ctrl-alt-2, then type the screenshot
> >> command there ...
> > 
> > Are you talking about sdl console or linux guest console? ctrl-alt-2 or
> > ctrl-alt-F2? I can try both.
> 
> ctrl-alt-2 sdl console.
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC v4 9/9] qxl-render: call ppm_save on bh

[Alon Levy]

On Wed, Feb 22, 2012 at 12:46:28PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, 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.
> 
> I still think we should defer that and figure how to fix that properly,
> either using (internally) async monitor commands via qapi, or using an
> event.
> 
> > +typedef struct QXLPPMSaveBHData {
> > +    PCIQXLDevice *qxl;
> > +    QXLCookie *cookie;
> > +} QXLPPMSaveBHData;
> 
> Is there a need for a separate struct?  I think you can just stick the
> filename into the QXLCookie struct, then write out screen shots in the
> update area bottom half, no?

The bh is there so I don't have to reset the opaque for the update_area
bh - only way to do that is to create a new bh everytime, since there
can be multiple update_area bh's scheduled at the same time.

The struct can go, if I change qxl -> qxl0, which I guess is fine for
this command.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC v4 6/9] qxl: remove flipped

[Alon Levy]

On Wed, Feb 22, 2012 at 03:09:09PM +0100, Gerd Hoffmann wrote:
> On 02/22/12 13:28, Alon Levy wrote:
> > On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >> It's not obvious to me how the non-flipped case (qxl_stride > 0) is
> >> handled now.  Have you tested this with both windows+linux guests?
> > 
> > It isn't handled. The simplest way is just to if on the stride and do a
> > single memcpy instead of individual line memcpy.
> 
> Single memcpy works only for full scanlines.  qxl_flip can be extended
> to handle both cases (and should probably also renamed then).
> 
> > This of course means we
> > are doing a redundant copy,
> 
> No.  You can wrap the qxl_flip call into ...
> 
>   if (is_shared_buffer()) { ... }.
> 
> ... to skip the copy if it isn't needed.
> 
> > since using our own DisplayAllocator or just
> > the existing deallocate + our own allocate of ds->surface->data removes
> > one copy.
> 
> I would just do
> 
>   if (qxl_stride > 0) {
>      qemu_free_displaysurface
>      qemu_create_displaysurface_from
>   } else {
>      qemu_resize_displaysurface
>   }
> 

Ok, this all works fine, thanks, just one note - linux qxl also uses
negative stride, so actually there is no user for positive stride (and
hence that code path is untested).

> cheers,
>   Gerd
>