Patchwork [v2,3/5] qxl-render: call ppm_save on bh

login
register
mail settings
Submitter Alon Levy
Date March 11, 2012, 7:26 p.m.
Message ID <1331494004-26177-4-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/146010/
State New
Headers show

Comments

Alon Levy - March 11, 2012, 7:26 p.m.
With this change ppm_save is called after rendering, and not before.
There are two lose ends:
 hmp: monitor will be active before ppm_save is complete.
 qmp: return will be emitted before ppm_save is complete.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl-render.c    |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/qxl.c           |    5 +--
 hw/qxl.h           |    2 +-
 trace-events       |    2 +
 ui/spice-display.h |    2 +
 5 files changed, 76 insertions(+), 11 deletions(-)
Luiz Capitulino - March 13, 2012, 1:22 p.m.
On Sun, 11 Mar 2012 21:26:42 +0200
Alon Levy <alevy@redhat.com> wrote:

> With this change ppm_save is called after rendering, and not before.
> There are two lose ends:
>  hmp: monitor will be active before ppm_save is complete.

The plan is to lock hmp's shell until rendering completes and the file is
saved. Looks ok to me.

>  qmp: return will be emitted before ppm_save is complete.

Let see if I got this right, please correct me with I'm wrong:

 o Before this commit: when screendump returns, there's an out of date
   screendump file available in the FS

 o After this commit: when screendump returns, there's no screendump
   file available yet

There's a behavior change, which is better done via a new command, which
would need to be async and we don't support that today. Also, having an
out of date screendump is not exactly useful anyway.

Honestly, I don't know what's the best thing to do in this case, but I'll be
fine with it if Gerd acks this patch.

Patch

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 74e7ea3..b281766 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -19,6 +19,7 @@ 
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "console.h"
 #include "qxl.h"
 
 static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
@@ -142,12 +143,68 @@  static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
 }
 
 /*
+ * struct used just for ppm save bh. We don't actually support multiple qxl
+ * screendump yet, but a) we will, and b) exporting qxl0 from qxl.c looks
+ * uglier imo.
+ */
+typedef struct QXLPPMSaveBHData {
+    PCIQXLDevice *qxl;
+    QXLCookie *cookie;
+} QXLPPMSaveBHData;
+
+static void qxl_render_ppm_save_bh(void *opaque);
+
+static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename)
+{
+    QXLPPMSaveBHData *ppm_save_bh_data;
+    QEMUBH *ppm_save_bh;
+    QXLCookie *cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
+                                       0);
+
+    qxl_set_rect_to_surface(qxl, &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;
+    }
+    return cookie;
+}
+
+static void qxl_cookie_render_free(PCIQXLDevice *qxl, QXLCookie *cookie)
+{
+    g_free(cookie->u.render.filename);
+    g_free(cookie);
+    --qxl->render_update_cookie_num;
+}
+
+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;
+
+    qemu_mutex_lock(&qxl->ssd.lock);
+    trace_qxl_render_ppm_save_bh(
+           qxl->ssd.ds->surface->data, qxl->guest_primary.data);
+    qxl_render_update_area_unlocked(qxl);
+    ppm_save(cookie->u.render.filename, qxl->ssd.ds->surface);
+    qxl_cookie_render_free(qxl, cookie);
+    qemu_mutex_unlock(&qxl->ssd.lock);
+    g_free(data);
+    qemu_bh_delete(bh);
+}
+
+/*
  * 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)
+void qxl_render_update(PCIQXLDevice *qxl, const char *filename)
 {
     QXLCookie *cookie;
 
@@ -155,6 +212,10 @@  void qxl_render_update(PCIQXLDevice *qxl)
 
     if (!runstate_is_running() || !qxl->guest_primary.commands) {
         qxl_render_update_area_unlocked(qxl);
+        if (filename) {
+            trace_qxl_render_update_screendump_no_update();
+            ppm_save(filename, qxl->ssd.ds->surface);
+        }
         qemu_mutex_unlock(&qxl->ssd.lock);
         return;
     }
@@ -162,9 +223,7 @@  void qxl_render_update(PCIQXLDevice *qxl)
     qxl->guest_primary.commands = 0;
     qxl->render_update_cookie_num++;
     qemu_mutex_unlock(&qxl->ssd.lock);
-    cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
-                            0);
-    qxl_set_rect_to_surface(qxl, &cookie->u.render.area);
+    cookie = qxl_cookie_render_new(qxl, filename);
     qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL,
                           0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie);
 }
@@ -182,10 +241,13 @@  void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie)
 {
     qemu_mutex_lock(&qxl->ssd.lock);
     trace_qxl_render_update_area_done(cookie);
-    qemu_bh_schedule(qxl->update_area_bh);
-    qxl->render_update_cookie_num--;
+    if (cookie->u.render.filename) {
+        qemu_bh_schedule(cookie->u.render.ppm_save_bh);
+    } else {
+        qemu_bh_schedule(qxl->update_area_bh);
+        qxl_cookie_render_free(qxl, 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 7857731..bcfd661 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1471,7 +1471,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;
@@ -1494,8 +1494,7 @@  static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
     switch (qxl->mode) {
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
-        qxl_render_update(qxl);
-        ppm_save(filename, qxl->ssd.ds->surface);
+        qxl_render_update(qxl, filename);
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename, cswitch);
diff --git a/hw/qxl.h b/hw/qxl.h
index 11a0db3..417ab28 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -147,7 +147,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/trace-events b/trace-events
index a66aee8..2f045c4 100644
--- a/trace-events
+++ b/trace-events
@@ -718,3 +718,5 @@  qxl_blit_guest_primary_initialized(void) ""
 qxl_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
 qxl_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
 qxl_render_update_area_done(void *cookie) "%p"
+qxl_render_update_screendump_no_update(void) ""
+qxl_render_ppm_save_bh(void *data, void *primary) "%p (primary %p)"
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 12e50b6..ec1fc24 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;