[2/4] spice: don't create updates in spice server context.

This patch moves the creation of spice screen updates from the spice
server context to qemu iothread context (display refresh timer to be
exact).  This way we avoid accessing qemu internals (display surface)
from spice thread context which in turn allows us to simplify locking.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl.c           |   17 +++++++++++------
 ui/spice-display.c |   45 ++++++++++++++++++++++++++++-----------------
 ui/spice-display.h |   21 ++++++++++++++++-----
 3 files changed, 55 insertions(+), 28 deletions(-)

On Fri, Apr 29, 2011 at 11:38:30AM +0200, Gerd Hoffmann wrote:
> This patch moves the creation of spice screen updates from the spice
> server context to qemu iothread context (display refresh timer to be
> exact).  This way we avoid accessing qemu internals (display surface)
> from spice thread context which in turn allows us to simplify locking.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qxl.c           |   17 +++++++++++------
>  ui/spice-display.c |   45 ++++++++++++++++++++++++++++-----------------
>  ui/spice-display.h |   21 ++++++++++++++++-----
>  3 files changed, 55 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..bd250db 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -343,18 +343,22 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>      SimpleSpiceUpdate *update;
>      QXLCommandRing *ring;
>      QXLCommand *cmd;
> -    int notify;
> +    int notify, ret;
>  
>      switch (qxl->mode) {
>      case QXL_MODE_VGA:
>          dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
> -        update = qemu_spice_create_update(&qxl->ssd);
> -        if (update == NULL) {
> -            return false;
> +        ret = false;
> +        qemu_mutex_lock(&qxl->ssd.lock);
> +        if (qxl->ssd.update != NULL) {
> +            update = qxl->ssd.update;
> +            qxl->ssd.update = NULL;
> +            *ext = update->ext;
> +            ret = true;
>          }
> -        *ext = update->ext;
> +        qemu_mutex_unlock(&qxl->ssd.lock);
>          qxl_log_command(qxl, "vga", ext);
> -        return true;
> +        return ret;
>      case QXL_MODE_COMPAT:
>      case QXL_MODE_NATIVE:
>      case QXL_MODE_UNDEFINED:
> @@ -1304,6 +1308,7 @@ static int qxl_init_primary(PCIDevice *dev)
>      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
>                                     qxl_hw_screen_dump, qxl_hw_text_update, qxl);
>      qxl->ssd.ds = vga->ds;
> +    qemu_mutex_init(&qxl->ssd.lock);
>      qxl->ssd.bufsize = (16 * 1024 * 1024);
>      qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
>  
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 020b423..39c0ba1 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -27,7 +27,7 @@
>  
>  #include "spice-display.h"
>  
> -static int debug = 0;
> +static int debug = 1;
Accident?

>  
>  static void GCC_FMT_ATTR(2, 3) dprint(int level, const char *fmt, ...)
>  {
> @@ -62,14 +62,7 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
>      dest->right = MAX(dest->right, r->right);
>  }
>  
> -/*
> - * Called from spice server thread context (via interface_get_command).
> - *
> - * We must aquire the global qemu mutex here to make sure the
> - * DisplayState (+DisplaySurface) we are accessing doesn't change
> - * underneath us.
> - */
> -SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
> +static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>  {
>      SimpleSpiceUpdate *update;
>      QXLDrawable *drawable;
> @@ -78,9 +71,7 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>      uint8_t *src, *dst;
>      int by, bw, bh;
>  
> -    qemu_mutex_lock_iothread();
>      if (qemu_spice_rect_is_empty(&ssd->dirty)) {
> -        qemu_mutex_unlock_iothread();
>          return NULL;
>      };
>  
> @@ -141,7 +132,6 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>      cmd->data = (intptr_t)drawable;
>  
>      memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> -    qemu_mutex_unlock_iothread();
>      return update;
>  }
>  
> @@ -241,6 +231,12 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
>      qemu_pf_conv_put(ssd->conv);
>      ssd->conv = NULL;
>  
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update != NULL) {
> +        qemu_spice_destroy_update(ssd, ssd->update);
> +        ssd->update = NULL;
> +    }
> +    qemu_mutex_unlock(&ssd->lock);
>      qemu_spice_destroy_host_primary(ssd);
>      qemu_spice_create_host_primary(ssd);
>  
> @@ -252,6 +248,14 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
>  {
>      dprint(3, "%s:\n", __FUNCTION__);
>      vga_hw_update();
> +
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update == NULL) {
> +        ssd->update = qemu_spice_create_update(ssd);
> +        ssd->notify++;
> +    }
> +    qemu_mutex_unlock(&ssd->lock);
> +
>      if (ssd->notify) {
>          ssd->notify = 0;
>          ssd->worker->wakeup(ssd->worker);
> @@ -298,14 +302,20 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>  {
>      SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
>      SimpleSpiceUpdate *update;
> +    int ret = false;
>  
>      dprint(3, "%s:\n", __FUNCTION__);
> -    update = qemu_spice_create_update(ssd);
> -    if (update == NULL) {
> -        return false;
> +
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update != NULL) {
> +        update = ssd->update;
> +        ssd->update = NULL;
> +        *ext = update->ext;
> +        ret = true;
>      }
> -    *ext = update->ext;
> -    return true;
> +    qemu_mutex_unlock(&ssd->lock);
> +
> +    return ret;
>  }
>  
>  static int interface_req_cmd_notification(QXLInstance *sin)
> @@ -398,6 +408,7 @@ void qemu_spice_display_init(DisplayState *ds)
>  {
>      assert(sdpy.ds == NULL);
>      sdpy.ds = ds;
> +    qemu_mutex_init(&sdpy.lock);
>      sdpy.bufsize = (16 * 1024 * 1024);
>      sdpy.buf = qemu_malloc(sdpy.bufsize);
>      register_displaychangelistener(ds, &display_listener);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..e0cc46e 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -19,6 +19,7 @@
>  #include <spice/enums.h>
>  #include <spice/qxl_dev.h>
>  
> +#include "qemu-thread.h"
>  #include "pflib.h"
>  
>  #define NUM_MEMSLOTS 8
> @@ -31,7 +32,10 @@
>  
>  #define NUM_SURFACES 1024
>  
> -typedef struct SimpleSpiceDisplay {
> +typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
> +typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
> +
> +struct SimpleSpiceDisplay {
>      DisplayState *ds;
>      void *buf;
>      int bufsize;
> @@ -43,19 +47,26 @@ typedef struct SimpleSpiceDisplay {
>      QXLRect dirty;
>      int notify;
>      int running;
> -} SimpleSpiceDisplay;
>  
> -typedef struct SimpleSpiceUpdate {
> +    /*
> +     * All struct members below this comment can be accessed from
> +     * both spice server and qemu (iothread) context and any access
> +     * to them must be protected by the lock.
> +     */
> +    QemuMutex lock;
> +    SimpleSpiceUpdate *update;
> +};
> +
> +struct SimpleSpiceUpdate {
>      QXLDrawable drawable;
>      QXLImage image;
>      QXLCommandExt ext;
>      uint8_t *bitmap;
> -} SimpleSpiceUpdate;
> +};
>  
>  int qemu_spice_rect_is_empty(const QXLRect* r);
>  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
>  
> -SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *sdpy);
>  void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update);
>  void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd);
>  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd);
> -- 
> 1.7.1
> 
>

Hi,

>> -static int debug = 0;
>> +static int debug = 1;
> Accident?

Oops.  That one wasn't intentional, will fix for the pull request.

cheers,
   Gerd