[Spice-devel] [PATCH] server: spice_qxl_update_area_dirty_async
Alon Levy
alevy at redhat.com
Mon Feb 20 05:59:50 PST 2012
On Mon, Feb 20, 2012 at 03:37:45PM +0200, Alon Levy wrote:
> 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.
>
Gerd,
Can you contribute your opinion on this? Do you think it's better to:
- add the new API spice_qxl_update_area_dirty_async
- use update_area_complete always
- set update_area_complete when it is required in qxl_render_update,
and unset it in qxl_render_update_complete
- something else
?
Alon
> >
> > 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
> >
> _______________________________________________
> 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