On Mon, Jun 20, 2011 at 06:32:30PM +0200, Alon Levy wrote:
> On Mon, Jun 20, 2011 at 05:50:32PM +0200, Gerd Hoffmann wrote:
> > On 06/20/11 17:11, Alon Levy wrote:
> > >On Mon, Jun 20, 2011 at 04:07:59PM +0200, Gerd Hoffmann wrote:
> > >>>>What is the difference to one worker->stop() + worker->start() cycle?
> > >>>>
> > >>>
> > >>>ok, stop+start won't disconnect any clients either. But does stop render
> > >>>all waiting commands?
> > >>>I'll have to look, I don't know if it does.
> > >>
> > >>It does. This is what qemu uses to flush all spice server state to
> > >>device memory on migration.
> > >>
> > >>What is the reason for deleting all surfaces?
> > >
> > >Making sure all references are dropped to pci memory in devram.
> >
> > Ah, because the spice server keeps a reference to the create command
> > until the surface is destroyed, right?
>
> Actually right, so my correction stands corrected.
>
> >
> > There is is QXL_IO_DESTROY_ALL_SURFACES + worker->destroy_surfaces() ...
> >
>
> Regarding QXL_IO_DESTROY_ALL_SURFACES, it destroys the primary surface too,
> which is a little special, that's another difference - update_mem destroys
> everything except the primary. I know I tried to destroy the primary but it
> didn't work right, don't recall why right now, so I guess I'll have to retry.
>
> > The QXL_IO_UPDATE_MEM command does too much special stuff IMHO.
> > I also think we don't need to extend the libspice-server API.
> >
> > We can add a I/O command which renders everything to device memory
> > via stop+start. We can zap all surfaces with the existing command +
> Yes, start+stop work nicely, didn't realize (saw it before, assumed
> it wouldn't be good enough), just need to destroy the surfaces too.
>
ok, it all works nicely except with the current driver patches I get a double
destroy for the primary surface. Removing it with the following patch makes
everything (resolution change/suspend/hibernate) work. I would really suggest
we remove that PANIC_ON, besides of course fixing the driver patches (I'll do a
v2 for the affected patche, the last series of qxl, I didn't cc you since I
didn't assume you'd want to review, but you probably saw it). Something like:
diff --git a/server/red_worker.c b/server/red_worker.c
index f0a8dfc..3b53a3f 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9684,7 +9684,11 @@ static inline void
handle_dev_destroy_primary_surface(RedWorker *worker)
receive_data(worker->channel, &surface_id, sizeof(uint32_t));
PANIC_ON(surface_id != 0);
- PANIC_ON(!worker->surfaces[surface_id].context.canvas);
+
+ if (!worker->surfaces[surface_id].context.canvas) {
+ red_printf("warning: double destroy of primary surface\n");
+ goto end;
+ }
if (worker->cursor) {
red_release_cursor(worker, worker->cursor);
@@ -9711,6 +9715,7 @@ static inline void
handle_dev_destroy_primary_surface(RedWorker *worker)
worker->cursor_position.x = worker->cursor_position.y = 0;
worker->cursor_trail_length = worker->cursor_trail_frequency = 0;
+end:
message = RED_WORKER_MESSAGE_READY;
write_message(worker->channel, &message);
}
> > worker call. We can add a I/O command to ask qxl to push the
> > release queue head to the release ring.
>
> So you suggest to replace QXL_IO_UPDATE_MEM with what, two io commands instead
> of using the val parameter?
> QXL_IO_UPDATE_MEM
> QXL_IO_FLUSH_RELEASE
> ?
>
> >
> > Comments?
> >
> > cheers,
> > Gerd
> >
>
