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 = 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).
yeah, I'll throw that, malloc something, cast to cookie, pass it, cast back,
free.
>
> >-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.
ok.
>
> > 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?
the current_async still gets changed from two threads (on [vcpu]ioport_write
and [worker]async_complete)
>
> >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, const QXLRect
> >*r)
> > dest->right = MAX(dest->right, r->right);
> > }
> >
> >+int qemu_spice_supports_async(SimpleSpiceDisplay *ssd)
> >+{
> >+ return (ssd->worker->major_version> 3 ||
> >+ (ssd->worker->major_version == 3&&
> >ssd->worker->minor_version>= 1));
> >+}
>
> 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)
>
> > void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t
> > id,
> > QXLDevSurfaceCreate *surface)
> > {
> > ssd->worker->create_primary_surface(ssd->worker, id, surface);
> > }
> >
> >+void qemu_spice_destroy_primary_surface_async(SimpleSpiceDisplay *ssd,
> >uint32_t id, int async)
> >+{
> >+ if (async) {
> >+ ssd->worker->destroy_primary_surface_async(ssd->worker, id, 0);
> >+ } else {
> >+ qemu_spice_destroy_primary_surface(ssd, id);
> >+ }
> >+}
>
> Like for all others: one only please.
ok
>
> cheers,
> Gerd
