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 <kra...@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
>
>
