diff mbox

[RFC,4/7] qxl: make qxl_render_update async

Message ID 1329686886-6853-5-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy Feb. 19, 2012, 9:28 p.m. UTC
RHBZ# 747011

Removes the last user of QXL_SYNC when using update drivers that use the
_ASYNC io ports.

The last user is qxl_render_update, it is called both by qxl_hw_update
which is the vga_hw_update_ptr passed to graphic_console_init, and by
qxl_hw_screen_dump.

At the same time the QXLRect area being passed to the red_worker thread
is passed as a copy, allocated and copied before passing, deallocated on
the interface_async_complete callback.

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

Comments

Gerd Hoffmann Feb. 20, 2012, 11:10 a.m. UTC | #1
Hi,


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

This is called from spice server thread context, correct?

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

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

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

> @@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
>                             uint32_t clear_dirty_region,
>                             qxl_async_io async, QXLCookie *cookie)
>  {
> +    struct QXLRect *area_copy;
>      if (async == QXL_SYNC) {
>          qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
>                          dirty_rects, num_dirty_rects, clear_dirty_region);
>      } else {
>  #if SPICE_INTERFACE_QXL_MINOR >= 1
>          if (cookie == NULL) {
> +            area_copy = g_malloc0(sizeof(*area_copy));
> +            memcpy(area_copy, area, sizeof(*area));
> +            area = area_copy;
>              cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO,
>                                      QXL_IO_UPDATE_AREA_ASYNC,
> -                                    0);
> +                                    (uint64_t)area_copy);

I still think this is the wrong place.

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

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

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

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

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

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

ok, will do.

> 
> cheers,
>   Gerd
>
Gerd Hoffmann Feb. 20, 2012, 12:45 p.m. UTC | #3
Hi,

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

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

cheers,
  Gerd
diff mbox

Patch

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