Re: [Qemu-devel] [PATCH 2/2] qxl: add QXL_IO_UPDATE_MEM for guest S3&S4 support

[Alon Levy]

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
> > 
>