Re: [Qemu-devel] [PATCH v2] qxl: async I/O
On Fri, Jul 08, 2011 at 09:17:50AM +0200, Gerd Hoffmann wrote:
> >+void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
> >+ struct QXLRect *area, struct QXLRect
> >*dirty_rects,
> >+ uint32_t num_dirty_rects, uint32_t
> >clear_dirty_region,
> >+ int async)
> >+{
> >+if (async) {
> >+qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id,
> >area, dirty_rects,
> >+ num_dirty_rects, clear_dirty_region, 0);
>
> Fails to build with older libspice.
btw, I'm looking at "#if.*MINOR" code like
#if SPICE_INTERFACE_CORE_MINOR >= 3
(ui/spice-core.c)
Shouldn't that be checking the MAJOR as well?
[snip]
Re: [Qemu-devel] [PATCH v2] qxl: async I/O
On Fri, Jul 08, 2011 at 09:17:50AM +0200, Gerd Hoffmann wrote:
> >+void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
> >+ struct QXLRect *area, struct QXLRect
> >*dirty_rects,
> >+ uint32_t num_dirty_rects, uint32_t
> >clear_dirty_region,
> >+ int async)
> >+{
> >+if (async) {
> >+qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id,
> >area, dirty_rects,
> >+ num_dirty_rects, clear_dirty_region, 0);
>
> Fails to build with older libspice.
>
> >+} else {
> >+qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
> >dirty_rects,
> >+ num_dirty_rects, clear_dirty_region);
> >+}
> >+}
> >
> > void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
> > struct QXLRect *area, struct QXLRect
> > *dirty_rects,
> > uint32_t num_dirty_rects, uint32_t
> > clear_dirty_region)
> > {
> >-qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
> >dirty_rects,
> >- num_dirty_rects, clear_dirty_region);
> >+qxl_spice_update_area_async(qxl, surface_id, area, dirty_rects,
> >+num_dirty_rects, clear_dirty_region, 0);
> > }
>
> Pretty pointless wrapper IMHO.
The above two lines change was a mistake. What about:
qxl_spice_update_area_async(...)
{
#ifdef ..
if (async) {
qxl->ssd.worker->update_area_async(...)
} else {
qxl_spice_update_area(...)
}
#else
qxl_spice_update_area(...)
#endif
}
>
> >-void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id)
> >+static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl)
> > {
> > qemu_mutex_lock(&qxl->track_lock);
> >-PANIC_ON(id>= NUM_SURFACES);
> >-qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> >-qxl->guest_surfaces.cmds[id] = 0;
> >+qxl->guest_surfaces.cmds[qxl->io_data.surface_id] = 0;
>
> I'd suggest to pass in the surface id as argument instead.
I can use the cookie if that's what you mean (which I guess means it will make
more sense to define it as a void pointer).
>
> > qxl->guest_surfaces.count--;
> > qemu_mutex_unlock(&qxl->track_lock);
> > }
> >
> >+static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl,
> >uint32_t id, int async)
> >+{
> >+qxl->io_data.surface_id = id;
> >+if (async) {
> >+qxl->ssd.worker->destroy_surface_wait_async(qxl->ssd.worker, id, 0);
> >+} else {
> >+qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> >+qxl_spice_destroy_surface_wait_complete(qxl);
>
> qxl_spice_destroy_surface_wait_complete(qxl, id);
and use the cookie on the async_complete with appropriate casting and free, got
it.
>
> >+}
> >+}
> >+
> > void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt
> > *ext,
> > uint32_t count)
> > {
> >@@ -171,15 +193,29 @@ void qxl_spice_reset_memslots(PCIQXLDevice *qxl)
> > qxl->ssd.worker->reset_memslots(qxl->ssd.worker);
> > }
> >
> >-void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> >+static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
> > {
> > qemu_mutex_lock(&qxl->track_lock);
> >-qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
> > memset(&qxl->guest_surfaces.cmds, 0, sizeof(qxl->guest_surfaces.cmds));
> > qxl->guest_surfaces.count = 0;
> > qemu_mutex_unlock(&qxl->track_lock);
> > }
> >
> >+static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> >+{
> >+qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
> >+qxl_spice_destroy_surfaces_complete(qxl);
> >+}
> >+
> >+static void qxl_spice_destroy_surfaces_async(PCIQXLDevice *qxl, int async)
> >+{
> >+if (async) {
> >+qxl->ssd.worker->destroy_surfaces_async(qxl->ssd.worker, 0);
> >+} else {
> >+qxl_spice_destroy_surfaces(qxl);
> >+}
> >+}
>
> I'd combine those into one function simliar to
> qxl_spice_destroy_surface_wait_async (and we don't need the _async
> suffix if we have a single version only which gets passed in async
> as argument).
ok, I'll ditch the suffix.
>
> >+
> > void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
> > {
> > qxl->ssd.worker->reset_image_cache(qxl->ssd.worker);
> >@@ -706,6 +742,38 @@ static int interface_flush_resources(QXLInstance *sin)
> > return ret;
> > }
> >
> >+static void qxl_add_memslot_complete(PCIQXLDevice *d);
> >+static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
> >+
> >+/* called from spice server thread context only */
> >+static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
> >+{
> >+PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> >+uint32_t current_async;
> >+
> >+qemu_mutex_lock(&qxl->async_lock);
> >+current_async = q
Re: [Qemu-devel] [PATCH v2] qxl: async I/O
btw, I'm looking at "#if.*MINOR" code like #if SPICE_INTERFACE_CORE_MINOR>= 3 (ui/spice-core.c) Shouldn't that be checking the MAJOR as well? major changing means a incompatible change. I doubt we ever will do that. But if you feel better checking that it probably should just be a #if SPICE_INTERFACE_CORE_MAJOR != 1 #error incompatible spice core interface #endif at the top of the file. cheers, Gerd
Re: [Qemu-devel] [PATCH v2] qxl: async I/O
The above two lines change was a mistake. What about:
qxl_spice_update_area_async(...)
{
#ifdef ..
if (async) {
qxl->ssd.worker->update_area_async(...)
} else {
qxl_spice_update_area(...)
}
#else
qxl_spice_update_area(...)
#endif
}
I would do
if (async) {
#if ...
worker->foo_async()
#else
abort() /* should hot happen */
#endif
} else {
worker->foo
}
yeah, I'll throw that, malloc something, cast to cookie, pass it, cast back,
free.
cookie should be big enougth to store the info directly. malloc works
too though.
Doing a runtime check here is pointless, just use
#if SPICE_INTERFACE_QXL_MINOR>= 1
...
#endif
this is a runtime check - what's preventing someone from compiling with 3.1 and
running with 3.0?
that we will require a newer library version? (which I am yet to send a patch
for)
Yes, thats why the minor version of the shared library needs to be raised.
cheers,
Gerd
Re: [Qemu-devel] [PATCH v2] qxl: async I/O
+void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
+ struct QXLRect *area, struct QXLRect *dirty_rects,
+ uint32_t num_dirty_rects, uint32_t
clear_dirty_region,
+ int async)
+{
+if (async) {
+qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id, area,
dirty_rects,
+ num_dirty_rects, clear_dirty_region, 0);
Fails to build with older libspice.
+} else {
+qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
dirty_rects,
+ num_dirty_rects, clear_dirty_region);
+}
+}
void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
struct QXLRect *area, struct QXLRect *dirty_rects,
uint32_t num_dirty_rects, uint32_t
clear_dirty_region)
{
-qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
dirty_rects,
- num_dirty_rects, clear_dirty_region);
+qxl_spice_update_area_async(qxl, surface_id, area, dirty_rects,
+num_dirty_rects, clear_dirty_region, 0);
}
Pretty pointless wrapper IMHO.
-void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id)
+static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl)
{
qemu_mutex_lock(&qxl->track_lock);
-PANIC_ON(id>= NUM_SURFACES);
-qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
-qxl->guest_surfaces.cmds[id] = 0;
+qxl->guest_surfaces.cmds[qxl->io_data.surface_id] = 0;
I'd suggest to pass in the surface id as argument instead.
qxl->guest_surfaces.count--;
qemu_mutex_unlock(&qxl->track_lock);
}
+static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl, uint32_t
id, int async)
+{
+qxl->io_data.surface_id = id;
+if (async) {
+qxl->ssd.worker->destroy_surface_wait_async(qxl->ssd.worker, id, 0);
+} else {
+qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
+qxl_spice_destroy_surface_wait_complete(qxl);
qxl_spice_destroy_surface_wait_complete(qxl, id);
+}
+}
+
void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
uint32_t count)
{
@@ -171,15 +193,29 @@ void qxl_spice_reset_memslots(PCIQXLDevice *qxl)
qxl->ssd.worker->reset_memslots(qxl->ssd.worker);
}
-void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
+static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
{
qemu_mutex_lock(&qxl->track_lock);
-qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
memset(&qxl->guest_surfaces.cmds, 0, sizeof(qxl->guest_surfaces.cmds));
qxl->guest_surfaces.count = 0;
qemu_mutex_unlock(&qxl->track_lock);
}
+static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
+{
+qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
+qxl_spice_destroy_surfaces_complete(qxl);
+}
+
+static void qxl_spice_destroy_surfaces_async(PCIQXLDevice *qxl, int async)
+{
+if (async) {
+qxl->ssd.worker->destroy_surfaces_async(qxl->ssd.worker, 0);
+} else {
+qxl_spice_destroy_surfaces(qxl);
+}
+}
I'd combine those into one function simliar to
qxl_spice_destroy_surface_wait_async (and we don't need the _async
suffix if we have a single version only which gets passed in async as
argument).
+
void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
{
qxl->ssd.worker->reset_image_cache(qxl->ssd.worker);
@@ -706,6 +742,38 @@ static int interface_flush_resources(QXLInstance *sin)
return ret;
}
+static void qxl_add_memslot_complete(PCIQXLDevice *d);
+static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
+
+/* called from spice server thread context only */
+static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
+{
+PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+uint32_t current_async;
+
+qemu_mutex_lock(&qxl->async_lock);
+current_async = qxl->current_async;
+qxl->current_async = QXL_UNDEFINED_IO;
+qemu_mutex_unlock(&qxl->async_lock);
I'd tend to use the cookie to pass that information (also the stuff in
io_data).
-static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta)
+static void qxl_add_memslot_complete(PCIQXLDevice *d)
I think it isn't needed to move that to the completion callback. Memory
slots can be created and destroyed with I/O commands only, so there is
no need to care about the ordering like we have to with surfaces.
qemu_mutex_init(&qxl->track_lock);
+qemu_mutex_init(&qxl->async_lock);
Do we really need two locks?
When passing info via cookie, doesn't the need for the async lock go
away completely?
index af10ae8..b7bc0de 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -62,6 +62,20 @@ void qemu_spice_rect_union(QXLRect *dest, cons
[Qemu-devel] [PATCH v2] qxl: async I/O
Some of the QXL port i/o commands are waiting for the spice server to
complete certain actions. Add async versions for these commands, so we
don't block the vcpu while the spice server processses the command.
Instead the qxl device will raise an IRQ when done.
The async command processing relies on an added QXLInterface::async_complete
and added QXLWorker::*_async additions, in spice server qxl >= 3.1
Signed-off-by: Gerd Hoffmann
Signed-off-by: Alon Levy
---
hw/qxl.c | 244 ++-
hw/qxl.h | 21 -
ui/spice-display.c | 33 +++
ui/spice-display.h |8 ++
4 files changed, 261 insertions(+), 45 deletions(-)
diff --git a/hw/qxl.c b/hw/qxl.c
index 935bac0..f72d5b8 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -136,25 +136,47 @@ void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg,
...)
}
}
+void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
+ struct QXLRect *area, struct QXLRect *dirty_rects,
+ uint32_t num_dirty_rects, uint32_t
clear_dirty_region,
+ int async)
+{
+if (async) {
+qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id, area,
dirty_rects,
+ num_dirty_rects, clear_dirty_region, 0);
+} else {
+qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
dirty_rects,
+ num_dirty_rects, clear_dirty_region);
+}
+}
void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
struct QXLRect *area, struct QXLRect *dirty_rects,
uint32_t num_dirty_rects, uint32_t
clear_dirty_region)
{
-qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area,
dirty_rects,
- num_dirty_rects, clear_dirty_region);
+qxl_spice_update_area_async(qxl, surface_id, area, dirty_rects,
+num_dirty_rects, clear_dirty_region, 0);
}
-void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id)
+static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl)
{
qemu_mutex_lock(&qxl->track_lock);
-PANIC_ON(id >= NUM_SURFACES);
-qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
-qxl->guest_surfaces.cmds[id] = 0;
+qxl->guest_surfaces.cmds[qxl->io_data.surface_id] = 0;
qxl->guest_surfaces.count--;
qemu_mutex_unlock(&qxl->track_lock);
}
+static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl, uint32_t
id, int async)
+{
+qxl->io_data.surface_id = id;
+if (async) {
+qxl->ssd.worker->destroy_surface_wait_async(qxl->ssd.worker, id, 0);
+} else {
+qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
+qxl_spice_destroy_surface_wait_complete(qxl);
+}
+}
+
void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
uint32_t count)
{
@@ -171,15 +193,29 @@ void qxl_spice_reset_memslots(PCIQXLDevice *qxl)
qxl->ssd.worker->reset_memslots(qxl->ssd.worker);
}
-void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
+static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
{
qemu_mutex_lock(&qxl->track_lock);
-qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
memset(&qxl->guest_surfaces.cmds, 0, sizeof(qxl->guest_surfaces.cmds));
qxl->guest_surfaces.count = 0;
qemu_mutex_unlock(&qxl->track_lock);
}
+static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
+{
+qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
+qxl_spice_destroy_surfaces_complete(qxl);
+}
+
+static void qxl_spice_destroy_surfaces_async(PCIQXLDevice *qxl, int async)
+{
+if (async) {
+qxl->ssd.worker->destroy_surfaces_async(qxl->ssd.worker, 0);
+} else {
+qxl_spice_destroy_surfaces(qxl);
+}
+}
+
void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
{
qxl->ssd.worker->reset_image_cache(qxl->ssd.worker);
@@ -706,6 +742,38 @@ static int interface_flush_resources(QXLInstance *sin)
return ret;
}
+static void qxl_add_memslot_complete(PCIQXLDevice *d);
+static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
+
+/* called from spice server thread context only */
+static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
+{
+PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+uint32_t current_async;
+
+qemu_mutex_lock(&qxl->async_lock);
+current_async = qxl->current_async;
+qxl->current_async = QXL_UNDEFINED_IO;
+qemu_mutex_unlock(&qxl->async_lock);
+
+dprint(qxl, 1, "async_complete: %d (%ld) done\n", current_async, cookie);
+switch (current_async) {
+case QXL_IO_MEMSLOT_ADD_ASYNC:
+qxl_add_memslot_complete(qxl);
+break;
+case QXL_IO_CREATE_PRIMARY_ASYNC:
+qxl_create_guest_primary_complete(qxl);
+break;
+
