From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qf5K7-0004Z2-6r for qemu-devel@nongnu.org; Fri, 08 Jul 2011 03:18:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qf5K3-00086D-7N for qemu-devel@nongnu.org; Fri, 08 Jul 2011 03:18:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31454) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qf5K2-00085h-GX for qemu-devel@nongnu.org; Fri, 08 Jul 2011 03:17:59 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p687HsnJ028418 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 8 Jul 2011 03:17:54 -0400 Message-ID: <4E16AF1E.4020406@redhat.com> Date: Fri, 08 Jul 2011 09:17:50 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <1310057455-18570-1-git-send-email-alevy@redhat.com> <1310057455-18570-10-git-send-email-alevy@redhat.com> In-Reply-To: <1310057455-18570-10-git-send-email-alevy@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] qxl: async I/O List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: yhalperi@redhat.com, qemu-devel@nongnu.org > +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, 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 > 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. cheers, Gerd