Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async
Hi, >>> @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl) >> >> dpy_update() call here. Calling that one isn't safe without grabbing >> the qemu lock. > > About dpy_update, discovered it the hard way. You mean I need the lock > for dpy_update or also before? Any qemu code should be considered thread-unsafe unless proven otherwise. On a quick scan I havn't noticed anything but the dpy_update() call. It makes sense to wrap the whole loop though, so you grab the lock only once. cheers, Gerd
Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async
On Mon, Feb 20, 2012 at 12:10:38PM +0100, Gerd Hoffmann wrote: > Hi, > > > > +void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) > > +{ > > This is called from spice server thread context, correct? > > > -for (i = 0; i < ARRAY_SIZE(dirty); i++) { > > +for (i = 0; i < ARRAY_SIZE(data->dirty); i++) { > > if (qemu_spice_rect_is_empty(dirty+i)) { > > break; > > } > > @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl) > > dpy_update() call here. Calling that one isn't safe without grabbing > the qemu lock. About dpy_update, discovered it the hard way. You mean I need the lock for dpy_update or also before? > > > dirty[i].right - dirty[i].left, > > dirty[i].bottom - dirty[i].top); > > } > > > @@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, > > uint32_t surface_id, > > uint32_t clear_dirty_region, > > qxl_async_io async, QXLCookie *cookie) > > { > > +struct QXLRect *area_copy; > > if (async == QXL_SYNC) { > > qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, > > dirty_rects, num_dirty_rects, clear_dirty_region); > > } else { > > #if SPICE_INTERFACE_QXL_MINOR >= 1 > > if (cookie == NULL) { > > +area_copy = g_malloc0(sizeof(*area_copy)); > > +memcpy(area_copy, area, sizeof(*area)); > > +area = area_copy; > > cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO, > > QXL_IO_UPDATE_AREA_ASYNC, > > -0); > > +(uint64_t)area_copy); > > I still think this is the wrong place. Yes, I agree, I thought I removed this already, I'll fix. > > Also: How about making removing QXLCookie->data and adding a union > instead? It's not like we have to transparently pass through a pointer > for someone else, it's our own state data, so this extra indirection > doesn't make sense at all. ok, will do. > > cheers, > Gerd >
Re: [Qemu-devel] [RFC 4/7] qxl: make qxl_render_update async
Hi, > +void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) > +{ This is called from spice server thread context, correct? > -for (i = 0; i < ARRAY_SIZE(dirty); i++) { > +for (i = 0; i < ARRAY_SIZE(data->dirty); i++) { > if (qemu_spice_rect_is_empty(dirty+i)) { > break; > } > @@ -151,6 +171,7 @@ void qxl_render_update(PCIQXLDevice *qxl) dpy_update() call here. Calling that one isn't safe without grabbing the qemu lock. > dirty[i].right - dirty[i].left, > dirty[i].bottom - dirty[i].top); > } > @@ -145,15 +145,19 @@ void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t > surface_id, > uint32_t clear_dirty_region, > qxl_async_io async, QXLCookie *cookie) > { > +struct QXLRect *area_copy; > if (async == QXL_SYNC) { > qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, > dirty_rects, num_dirty_rects, clear_dirty_region); > } else { > #if SPICE_INTERFACE_QXL_MINOR >= 1 > if (cookie == NULL) { > +area_copy = g_malloc0(sizeof(*area_copy)); > +memcpy(area_copy, area, sizeof(*area)); > +area = area_copy; > cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO, > QXL_IO_UPDATE_AREA_ASYNC, > -0); > +(uint64_t)area_copy); I still think this is the wrong place. Also: How about making removing QXLCookie->data and adding a union instead? It's not like we have to transparently pass through a pointer for someone else, it's our own state data, so this extra indirection doesn't make sense at all. cheers, Gerd