On Mon, Jun 27, 2011 at 09:28:45AM +0300, yhalperi wrote: > On 06/26/2011 08:47 PM, Alon Levy wrote: > >On Sun, Jun 26, 2011 at 12:59:06PM -0400, Yonit Halperin wrote: > >>Sorry for the late response, wasn't available. > >>I'm afraid that (1) and (2) will indeed wakeup the worker, but will not > >>assure emptying the command ring, as it depends on the client pipe size. > >> > > > >I actually can't figure out what wakeup does (that's what both NOTIFY and > >NOTIFY_CURSOR do, see hw/qxl.c). > It turns on an event the worker is waiting for on epoll_wait. Ah, ok. So it will only read up to the pipe size like you said.
> > What we did in prepare_to_sleep before was > >call flush_all_qxl_commands, which reads the command and cursor rings until > >they are empty (calling flush_cursor_commands and flush_display_commands), > >waiting > >whenever the pipe is too large. (avoiding this wait still needs to be done, > >but > >after ensuring suspend is correct). > > > >More to the point this flush is done from handle_dev_destroy_surfaces, but > >this is not good enough since this also destroys the surfaces, before we have > >a chance to actually render the surfaces. > > > >Perhaps RED_WORKER_MESSAGE_STOP should do a flush_all_qxl_commands? > > > We can do it as long as it doesn't affect migration - does STOP > happens before or after savevm? If it happens after - we can't touch > the command ring, i.e., we can't call flush. And even if it happens > before - do we really want to call flush during migration and > presumably slow it down? But if we don't then the client connected before migration doesn't get some of the messages it was supposed to get. stop is called before hw/qxl.c:qxl_pre_save, from ui/spice-display.c:qemu_spice_vm_change_state_handler > > Cheers, > Yonit. > > >> > >>----- Original Message ----- > >>From: "Alon Levy"<al...@redhat.com> > >>To: "Gerd Hoffmann"<kra...@redhat.com> > >>Cc: qemu-devel@nongnu.org, yhalp...@redhat.com > >>Sent: Wednesday, June 22, 2011 11:57:54 AM > >>Subject: Re: [Qemu-devel] [PATCH 2/2] qxl: add QXL_IO_UPDATE_MEM for guest > >>S3&S4 support > >> > >>On Wed, Jun 22, 2011 at 11:13:19AM +0200, Gerd Hoffmann wrote: > >>> Hi, > >>> > >>>>>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? > >>> > >>>I'd like to (a) avoid updating the libspice-server API if possible > >>>and (b) have one I/O command for each logical step. So going into > >>>S3 could look like this: > >>> > >>> (0) stop putting new commands into the rings > >>> (1) QXL_IO_NOTIFY_CMD > >>> (2) QXL_IO_NOTIFY_CURSOR > >>> qxl calls notify(), to make the worker thread empty the command > >>> rings before it processes the next dispatcher request. > >>> (3) QXl_IO_FLUSH_SURFACES (to be implemented) > >>> qxl calls stop()+start(), spice-server renders all surfaces, > >>> thereby flushing state to device memory. > >>> (4) QXL_IO_DESTROY_ALL_SURFACES > >>> zap surfaces > >>> (5) QXL_IO_FLUSH_RELEASE (to be implemented) > >>> push release queue head into the release ring, so the guest > >>> will see it and can release everything. > >>> > >>>(1)+(2)+(4) exist already. > >>>(3)+(5) can be done without libspice-server changes. > >>> > >>>Looks workable? > >> > >>yeah. Yonit? > >> > >>> > >>>cheers, > >>> Gerd > >>> > >>> > >> > >