diff mbox

[PATCHv3] qxl-render: use update_area_async and update_area_complete

Message ID 1310478932-25370-17-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy July 12, 2011, 1:55 p.m. UTC 
So now there are two implementations chosen based on QXL_INTERFACE_MINOR:
 * old (spice qxl minor == 0) - use update_area, no change.
 * new:
  1. keep an array of updated rectangles (ssd.dirty_rects)
  2. update it on callback (realloc)
  3. render the current one before issuing a new update_area_async
---
 hw/qxl-render.c    |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
 hw/qxl.c           |   12 ++++++++++++
 hw/qxl.h           |    2 ++
 ui/spice-display.h |    4 ++++
 4 files changed, 64 insertions(+), 2 deletions(-)

Comments

Gerd Hoffmann July 13, 2011, 7:51 a.m. UTC | #1 
Hi,

> +void qxl_render_primary_updated(PCIQXLDevice *qxl, QXLRect *dirty,
> +                                uint32_t num_dirty);

> @@ -65,6 +65,10 @@ struct SimpleSpiceDisplay {
>       int notify;
>       int running;
>
> +#if SPICE_INTERFACE_QXL_MINOR>= 1
> +    QXLRect *dirty_rects;
> +    uint32_t num_dirty_rects;
> +#endif

Why do you put this into SimpleSpiceDisplay instead of PCIQXLState?

I also wouldn't #ifdef the struct elements to reduce the #ifdef clutter. 
  #ifdefs should only be there in case the code wouldn't compile without 
them.

Additionally you can fill these struct elements in sync mode too and 
have qxl_render_primary_updated pick up the rectangles from the struct 
instead of getting them passed in as arguments, thereby reducing the 
code differences between sync and async mode.

cheers,
   Gerd
Alon Levy July 13, 2011, 9:30 a.m. UTC | #2 
On Wed, Jul 13, 2011 at 09:51:14AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >+void qxl_render_primary_updated(PCIQXLDevice *qxl, QXLRect *dirty,
> >+                                uint32_t num_dirty);
> 
> >@@ -65,6 +65,10 @@ struct SimpleSpiceDisplay {
> >      int notify;
> >      int running;
> >
> >+#if SPICE_INTERFACE_QXL_MINOR>= 1
> >+    QXLRect *dirty_rects;
> >+    uint32_t num_dirty_rects;
> >+#endif
> 
> Why do you put this into SimpleSpiceDisplay instead of PCIQXLState?
> 
> I also wouldn't #ifdef the struct elements to reduce the #ifdef
> clutter.  #ifdefs should only be there in case the code wouldn't
> compile without them.
> 
> Additionally you can fill these struct elements in sync mode too and
> have qxl_render_primary_updated pick up the rectangles from the
> struct instead of getting them passed in as arguments, thereby
> reducing the code differences between sync and async mode.
> 

ok, will do.

> cheers,
>   Gerd
>
diff mbox

Patch

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index f440fe8..4862c35 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -103,8 +103,11 @@  static void qxl_render_update_dirty_rectangles(PCIQXLDevice *qxl, QXLRect *dirty
 void qxl_render_update(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
-    QXLRect dirty[32], update;
+    QXLRect update;
     void *ptr;
+#if SPICE_INTERFACE_QXL_MINOR < 1
+    QXLRect dirty[32];
+#endif
 
     if (!qxl->ssd.running) {
         return;
@@ -112,7 +115,13 @@  void qxl_render_update(PCIQXLDevice *qxl)
 
     if (qxl->guest_primary.resized) {
         qxl->guest_primary.resized = 0;
-
+        qemu_mutex_lock(&qxl->ssd.lock);
+        if (qxl->ssd.num_dirty_rects > 0) {
+            free(qxl->ssd.dirty_rects);
+            qxl->ssd.dirty_rects = NULL;
+            qxl->ssd.num_dirty_rects = 0;
+        }
+        qemu_mutex_unlock(&qxl->ssd.lock);
         if (qxl->guest_primary.flipped) {
             qemu_free(qxl->guest_primary.flipped);
             qxl->guest_primary.flipped = NULL;
@@ -146,6 +155,19 @@  void qxl_render_update(PCIQXLDevice *qxl)
         dpy_resize(vga->ds);
     }
 
+#if SPICE_INTERFACE_QXL_MINOR >= 1
+    /* render rectangles from last update_area_async */
+    qemu_mutex_lock(&qxl->ssd.lock);
+    if (qxl->ssd.num_dirty_rects > 0) {
+        qxl_render_update_dirty_rectangles(qxl, qxl->ssd.dirty_rects,
+                                           qxl->ssd.num_dirty_rects);
+        free(qxl->ssd.dirty_rects);
+        qxl->ssd.dirty_rects = NULL;
+        qxl->ssd.num_dirty_rects = 0;
+    }
+    qemu_mutex_unlock(&qxl->ssd.lock);
+#endif
+
     if (!qxl->guest_primary.commands) {
         return;
     }
@@ -156,11 +178,33 @@  void qxl_render_update(PCIQXLDevice *qxl)
     update.top    = 0;
     update.bottom = qxl->guest_primary.surface.height;
 
+#if SPICE_INTERFACE_QXL_MINOR >= 1
+    /* do a new update_area */
+    qxl_spice_update_area_async(qxl, 0, &update, 1, 1);
+#else
     memset(dirty, 0, sizeof(dirty));
     qxl_spice_update_area(qxl, 0, &update,
                           dirty, ARRAY_SIZE(dirty), 1);
     qxl_render_update_dirty_rectangles(qxl, dirty, ARRAY_SIZE(dirty));
+#endif
+}
+
+#if SPICE_INTERFACE_QXL_MINOR >= 1
+void qxl_render_primary_updated(PCIQXLDevice *qxl, QXLRect *dirty,
+                                uint32_t num_dirty)
+{
+    if (num_dirty == 0) {
+        return;
+    }
+    qemu_mutex_lock(&qxl->ssd.lock);
+    qxl->ssd.num_dirty_rects += num_dirty;
+    qxl->ssd.dirty_rects = qemu_realloc(qxl->ssd.dirty_rects,
+                             sizeof(QXLRect) * qxl->ssd.num_dirty_rects);
+    memcpy(&qxl->ssd.dirty_rects[qxl->ssd.num_dirty_rects - num_dirty],
+           dirty, num_dirty * sizeof(QXLRect));
+    qemu_mutex_unlock(&qxl->ssd.lock);
 }
+#endif /* SPICE_INTERFACE_QXL_MINOR >= 1 */
 
 static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
 {
diff --git a/hw/qxl.c b/hw/qxl.c
index de93efa..8a9463e 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -780,6 +780,17 @@  static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
     qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD);
 }
 
+static void interface_update_area_complete(QXLInstance *sin,
+            uint32_t surface_id, struct QXLRect *updated_rects,
+            uint32_t num_updated_rects)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    dprint(qxl, 3, "%s: %d\n", __FUNCTION__, surface_id);
+    if (surface_id == 0) {
+        qxl_render_primary_updated(qxl, updated_rects, num_updated_rects);
+    }
+}
 #endif
 
 static const QXLInterface qxl_interface = {
@@ -803,6 +814,7 @@  static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     .async_complete          = interface_async_complete,
+    .update_area_complete    = interface_update_area_complete,
 #endif
 };
 
diff --git a/hw/qxl.h b/hw/qxl.h
index 2c7f94a..fad01c6 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -139,4 +139,6 @@  void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
                                  struct QXLRect *area,
                                  uint32_t clear_dirty_region,
                                  int is_vga);
+void qxl_render_primary_updated(PCIQXLDevice *qxl, QXLRect *dirty,
+                                uint32_t num_dirty);
 #endif
diff --git a/ui/spice-display.h b/ui/spice-display.h
index d24cca9..115aae5 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -65,6 +65,10 @@  struct SimpleSpiceDisplay {
     int notify;
     int running;
 
+#if SPICE_INTERFACE_QXL_MINOR >= 1
+    QXLRect *dirty_rects;
+    uint32_t num_dirty_rects;
+#endif
     /*
      * All struct members below this comment can be accessed from
      * both spice server and qemu (iothread) context and any access