[Spice-devel] [PATCH] server: spice_qxl_update_area_dirty_async
Alon Levy
alevy at redhat.com
Mon Feb 20 05:37:45 PST 2012
On Mon, Feb 20, 2012 at 02:52:51PM +0200, Yonit Halperin wrote:
> Hi,
> On 02/19/2012 02:53 PM, Alon Levy wrote:
> >On Sun, Feb 19, 2012 at 02:37:16PM +0200, Alon Levy wrote:
> >>Bumps spice to 0.9.2, since it adds a new symbol. This will be 0.8.4
> >>in 0.8 branch - the syms file is updated to contain 0.8.3 and 0.8.4.
> >
> >Forgot to change this comment. I'll drop the 0.10.1->0.10.2 change since
> >Marc Andre sent a patch that does that already.
> >
> >>
> >>Add an asynchronous version of update_area that updates the dirty
> >>rectangles, similarily to the sync one. The existing async version
> >>doesn't pass the dirty list at all, which is good for
> >>QXL_IO_UPDATE_AREA, but bad for the monitor command screen_dump and
> >>vnc/sdl interoperability.
> >>
> (1) It would be nice to explain why the call to the sync version
> caused deadlock.
Right.
>
> (2) Is it necessary to add another async version of update_area?
> Can't we use the update_area_complete callback instead?
Ok, I completely forgot about that. There is the downside that the way
it is currently written any implementation of update_area_complete will
be called always, and causes a little work in
handle_dev_update_async_helper. It also causes the need to note when we
start a qxl_render_update and complete it, to know when we want to act
on the update_area_complete call. Not sure it's better then introducing
a new API, but I agree it isn't really a must.
>
> Regards,
> Yonit.
> >>RHBZ: 747011
> >>FDBZ: 41622
> >>---
> >> configure.ac | 2 +-
> >> server/red_dispatcher.c | 42 +++++++++++++++++++++++++++----
> >> server/red_dispatcher.h | 9 +++++++
> >> server/red_worker.c | 60 +++++++++++++++++++++++++++++++++-------------
> >> server/red_worker.h | 3 ++
> >> server/spice-server.syms | 5 ++++
> >> server/spice.h | 6 ++++-
> >> 7 files changed, 102 insertions(+), 25 deletions(-)
> >>
> >>diff --git a/configure.ac b/configure.ac
> >>index 1c15e74..a617096 100644
> >>--- a/configure.ac
> >>+++ b/configure.ac
> >>@@ -2,7 +2,7 @@ AC_PREREQ([2.57])
> >>
> >> m4_define([SPICE_MAJOR], 0)
> >> m4_define([SPICE_MINOR], 10)
> >>-m4_define([SPICE_MICRO], 1)
> >>+m4_define([SPICE_MICRO], 2)
> >>
> >> AC_INIT(spice, [SPICE_MAJOR.SPICE_MINOR.SPICE_MICRO], [], spice)
> >>
> >>diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> >>index 321232b..5b8097c 100644
> >>--- a/server/red_dispatcher.c
> >>+++ b/server/red_dispatcher.c
> >>@@ -334,6 +334,28 @@ static void red_dispatcher_update_area_async(RedDispatcher *dispatcher,
> >> &payload);
> >> }
> >>
> >>+static void red_dispatcher_update_area_dirty_async(RedDispatcher *dispatcher,
> >>+ uint32_t surface_id,
> >>+ struct QXLRect *qxl_area,
> >>+ struct QXLRect *qxl_dirty_rects,
> >>+ uint32_t num_dirty_rects,
> >>+ uint32_t clear_dirty_region,
> >>+ uint64_t cookie)
> >>+{
> >>+ RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC;
> >>+ RedWorkerMessageUpdateDirtyAsync payload;
> >>+
> >>+ payload.base.cmd = async_command_alloc(dispatcher, message, cookie);
> >>+ payload.surface_id = surface_id;
> >>+ payload.qxl_area = *qxl_area;
> >>+ payload.qxl_dirty_rects = qxl_dirty_rects;
> >>+ payload.num_dirty_rects = num_dirty_rects;
> >>+ payload.clear_dirty_region = clear_dirty_region;
> >>+ dispatcher_send_message(&dispatcher->dispatcher,
> >>+ message,
> >>+&payload);
> >>+}
> >>+
> >> static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> >> QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> >> uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> >>@@ -870,6 +892,17 @@ void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext,
> >> }
> >>
> >> SPICE_GNUC_VISIBLE
> >>+void spice_qxl_update_area_dirty_async(QXLInstance *instance, uint32_t surface_id,
> >>+ struct QXLRect *qxl_area, struct QXLRect *dirty_rects,
> >>+ uint32_t num_dirty_rects, uint32_t clear_dirty_region,
> >>+ uint64_t cookie)
> >>+{
> >>+ red_dispatcher_update_area_dirty_async(instance->st->dispatcher, surface_id, qxl_area,
> >>+ dirty_rects, num_dirty_rects, clear_dirty_region,
> >>+ cookie);
> >>+}
> >>+
> >>+SPICE_GNUC_VISIBLE
> >> void spice_qxl_update_area_async(QXLInstance *instance, uint32_t surface_id, QXLRect *qxl_area,
> >> uint32_t clear_dirty_region, uint64_t cookie)
> >> {
> >>@@ -926,10 +959,11 @@ void red_dispatcher_async_complete(struct RedDispatcher *dispatcher,
> >> pthread_mutex_unlock(&dispatcher->async_lock);
> >> switch (async_command->message) {
> >> case RED_WORKER_MESSAGE_UPDATE_ASYNC:
> >>- break;
> >>+ case RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC:
> >> case RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC:
> >>- break;
> >> case RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC:
> >>+ case RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC:
> >>+ case RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC:
> >> break;
> >> case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC:
> >> red_dispatcher_create_primary_surface_complete(dispatcher);
> >>@@ -937,10 +971,6 @@ void red_dispatcher_async_complete(struct RedDispatcher *dispatcher,
> >> case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC:
> >> red_dispatcher_destroy_primary_surface_complete(dispatcher);
> >> break;
> >>- case RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC:
> >>- break;
> >>- case RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC:
> >>- break;
> >> default:
> >> WARN("unexpected message");
> >> }
> >>diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> >>index 36db4e3..8bf7433 100644
> >>--- a/server/red_dispatcher.h
> >>+++ b/server/red_dispatcher.h
> >>@@ -89,6 +89,15 @@ typedef struct RedWorkerMessageUpdateAsync {
> >> uint32_t clear_dirty_region;
> >> } RedWorkerMessageUpdateAsync;
> >>
> >>+typedef struct RedWorkerMessageUpdateDirtyAsync {
> >>+ RedWorkerMessageAsync base;
> >>+ uint32_t surface_id;
> >>+ QXLRect qxl_area;
> >>+ QXLRect *qxl_dirty_rects;
> >>+ uint32_t num_dirty_rects;
> >>+ uint32_t clear_dirty_region;
> >>+} RedWorkerMessageUpdateDirtyAsync;
> >>+
> >> typedef struct RedWorkerMessageAddMemslot {
> >> QXLDevMemSlot mem_slot;
> >> } RedWorkerMessageAddMemslot;
> >>diff --git a/server/red_worker.c b/server/red_worker.c
> >>index 4c73952..19a0632 100644
> >>--- a/server/red_worker.c
> >>+++ b/server/red_worker.c
> >>@@ -43,6 +43,7 @@
> >> #include<netinet/tcp.h>
> >> #include<setjmp.h>
> >> #include<openssl/ssl.h>
> >>+#include<inttypes.h>
> >>
> >> #include<spice/qxl_dev.h>
> >> #include "spice.h"
> >>@@ -10237,17 +10238,12 @@ static void surface_dirty_region_to_rects(RedSurface *surface,
> >> free(dirty_rects);
> >> }
> >>
> >>-void handle_dev_update_async(void *opaque, void *payload)
> >>+void handle_dev_update_async_helper(RedWorker *worker, uint32_t surface_id, QXLRect qxl_area,
> >>+ QXLRect *qxl_dirty_rects, uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> >> {
> >>- RedWorker *worker = opaque;
> >>- RedWorkerMessageUpdateAsync *msg = payload;
> >> SpiceRect rect;
> >>- QXLRect *qxl_dirty_rects;
> >>- uint32_t num_dirty_rects;
> >> RedSurface *surface;
> >>- uint32_t surface_id = msg->surface_id;
> >>- QXLRect qxl_area = msg->qxl_area;
> >>- uint32_t clear_dirty_region = msg->clear_dirty_region;
> >>+ int free_dirty = 0;
> >>
> >> red_get_rect_ptr(&rect,&qxl_area);
> >> flush_display_commands(worker);
> >>@@ -10256,20 +10252,45 @@ void handle_dev_update_async(void *opaque, void *payload)
> >>
> >> validate_surface(worker, surface_id);
> >> red_update_area(worker,&rect, surface_id);
> >>- if (!worker->qxl->st->qif->update_area_complete) {
> >>+ if (!worker->qxl->st->qif->update_area_complete&& !qxl_dirty_rects) {
> >> return;
> >> }
> >>+
> >> surface =&worker->surfaces[surface_id];
> >>- num_dirty_rects = pixman_region32_n_rects(&surface->draw_dirty_region);
> >>- if (num_dirty_rects == 0) {
> >>- return;
> >>+ if (!qxl_dirty_rects) {
> >>+ num_dirty_rects = pixman_region32_n_rects(&surface->draw_dirty_region);
> >>+ if (num_dirty_rects != 0) {
> >>+ qxl_dirty_rects = spice_new0(QXLRect, num_dirty_rects);
> >>+ free_dirty = 1;
> >>+ }
> >> }
> >>- qxl_dirty_rects = spice_new0(QXLRect, num_dirty_rects);
> >>- surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
> >>- clear_dirty_region);
> >>- worker->qxl->st->qif->update_area_complete(worker->qxl, surface_id,
> >>+ if (num_dirty_rects> 0) {
> >>+ surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
> >>+ clear_dirty_region);
> >>+ }
> >>+ if (worker->qxl->st->qif->update_area_complete) {
> >>+ worker->qxl->st->qif->update_area_complete(worker->qxl, surface_id,
> >> qxl_dirty_rects, num_dirty_rects);
> >>- free(qxl_dirty_rects);
> >>+ }
> >>+ if (free_dirty) {
> >>+ free(qxl_dirty_rects);
> >>+ }
> >>+}
> >>+
> >>+void handle_dev_update_dirty_async(void *opaque, void *payload)
> >>+{
> >>+ RedWorkerMessageUpdateDirtyAsync *msg = payload;
> >>+
> >>+ handle_dev_update_async_helper((RedWorker *)opaque, msg->surface_id,
> >>+ msg->qxl_area, msg->qxl_dirty_rects, msg->num_dirty_rects, msg->clear_dirty_region);
> >>+}
> >>+
> >>+void handle_dev_update_async(void *opaque, void *payload)
> >>+{
> >>+ RedWorkerMessageUpdateAsync *msg = payload;
> >>+
> >>+ handle_dev_update_async_helper((RedWorker *)opaque, msg->surface_id,
> >>+ msg->qxl_area, NULL, 0, msg->clear_dirty_region);
> >> }
> >>
> >> void handle_dev_update(void *opaque, void *payload)
> >>@@ -10914,6 +10935,11 @@ static void register_callbacks(Dispatcher *dispatcher)
> >> sizeof(RedWorkerMessageUpdateAsync),
> >> DISPATCHER_ASYNC);
> >> dispatcher_register_handler(dispatcher,
> >>+ RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC,
> >>+ handle_dev_update_dirty_async,
> >>+ sizeof(RedWorkerMessageUpdateDirtyAsync),
> >>+ DISPATCHER_ASYNC);
> >>+ dispatcher_register_handler(dispatcher,
> >> RED_WORKER_MESSAGE_ADD_MEMSLOT,
> >> handle_dev_add_memslot,
> >> sizeof(RedWorkerMessageAddMemslot),
> >>diff --git a/server/red_worker.h b/server/red_worker.h
> >>index 1f63d01..987563c 100644
> >>--- a/server/red_worker.h
> >>+++ b/server/red_worker.h
> >>@@ -85,6 +85,9 @@ enum {
> >> RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> >> RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> >>
> >>+ /* support monitor async screen dump */
> >>+ RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC,
> >>+
> >> RED_WORKER_MESSAGE_COUNT // LAST
> >> };
> >>
> >>diff --git a/server/spice-server.syms b/server/spice-server.syms
> >>index d9beec3..8b5a2ff 100644
> >>--- a/server/spice-server.syms
> >>+++ b/server/spice-server.syms
> >>@@ -101,3 +101,8 @@ global:
> >> spice_server_add_client;
> >> spice_server_add_ssl_client;
> >> } SPICE_SERVER_0.10.0;
> >>+
> >>+SPICE_SERVER_0.10.2 {
> >>+global:
> >>+ spice_qxl_update_area_dirty_async;
> >>+} SPICE_SERVER_0.10.1;
> >>diff --git a/server/spice.h b/server/spice.h
> >>index 7397655..a096fbc 100644
> >>--- a/server/spice.h
> >>+++ b/server/spice.h
> >>@@ -22,7 +22,7 @@
> >> #include<sys/socket.h>
> >> #include<spice/qxl_dev.h>
> >>
> >>-#define SPICE_SERVER_VERSION 0x000a01 /* release 0.10.1 */
> >>+#define SPICE_SERVER_VERSION 0x000a02 /* release 0.10.2 */
> >>
> >> /* interface base type */
> >>
> >>@@ -153,6 +153,10 @@ void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext,
> >> /* async versions of commands. when complete spice calls async_complete */
> >> void spice_qxl_update_area_async(QXLInstance *instance, uint32_t surface_id, QXLRect *qxl_area,
> >> uint32_t clear_dirty_region, uint64_t cookie);
> >>+void spice_qxl_update_area_dirty_async(QXLInstance *instance, uint32_t surface_id,
> >>+ struct QXLRect *qxl_area, struct QXLRect *dirty_rects,
> >>+ uint32_t num_dirty_rects, uint32_t clear_dirty_region,
> >>+ uint64_t cookie);
> >> void spice_qxl_add_memslot_async(QXLInstance *instance, QXLDevMemSlot *slot, uint64_t cookie);
> >> void spice_qxl_destroy_surfaces_async(QXLInstance *instance, uint64_t cookie);
> >> void spice_qxl_destroy_primary_surface_async(QXLInstance *instance, uint32_t surface_id, uint64_t cookie);
> >>--
> >>1.7.9
> >>
> >>_______________________________________________
> >>Spice-devel mailing list
> >>Spice-devel at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >_______________________________________________
> >Spice-devel mailing list
> >Spice-devel at lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list