| Message ID | 20210127100206.277698-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui: fix spice display regression | expand |
Hi On Wed, Jan 27, 2021 at 2:03 PM <marcandre.lureau@redhat.com> wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Since commit b4e1a342112e50e05b609e857f38c1f2b7aafdc4 ("vl: remove > separate preconfig main_loop"), spice initialization is a bit dodgy, and > the client get stuck waiting for server events. > > The initialization order changed, so that qemu_spice_display_start() is > called before the display interfaces are added. The new interfaces > aren't started by spice-server automatically (yet?), so we have to tell > the server explicitely when we are already running. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > ui/spice-core.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/ui/spice-core.c b/ui/spice-core.c > index 5746d0aae7..6eebf12e3c 100644 > --- a/ui/spice-core.c > +++ b/ui/spice-core.c > @@ -830,6 +830,8 @@ static void qemu_spice_init(void) > > static int qemu_spice_add_interface(SpiceBaseInstance *sin) > { > + int ret; > + > if (!spice_server) { > if (QTAILQ_FIRST(&qemu_spice_opts.head) != NULL) { > error_report("Oops: spice configured but not active"); > @@ -848,7 +850,13 @@ static int qemu_spice_add_interface(SpiceBaseInstance *sin) > qemu_add_vm_change_state_handler(vm_change_state_handler, NULL); > } > > - return spice_server_add_interface(spice_server, sin); > + ret = spice_server_add_interface(spice_server, sin); > + /* make sure the newly added interface is started */ > + if (ret == 0 && spice_display_is_running) { > + spice_server_vm_start(spice_server); > + } > + > + return ret; > } > > static GSList *spice_consoles; > -- > 2.29.0 > > Oops, it doesn't work reliably. There is some race in spice server now. spice_server_vm_start() sends RED_WORKER_MESSAGE_START to the QXL worker thread. But if two of those come, it will assert... It should probably not, I will send a patch to spice. I am looking for other options for QEMU though.
On 27/01/21 11:18, Marc-André Lureau wrote: > Hi > > On Wed, Jan 27, 2021 at 2:03 PM <marcandre.lureau@redhat.com> wrote: >> >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> Since commit b4e1a342112e50e05b609e857f38c1f2b7aafdc4 ("vl: remove >> separate preconfig main_loop"), spice initialization is a bit dodgy, and >> the client get stuck waiting for server events. >> >> The initialization order changed, so that qemu_spice_display_start() is >> called before the display interfaces are added. The new interfaces >> aren't started by spice-server automatically (yet?), so we have to tell >> the server explicitely when we are already running. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> What is the exact different in initialization order once you add commit facf7c60ee ("vl: initialize displays _after_ exiting preconfiguration", 2021-01-02)? Thanks, Paolo >> --- >> ui/spice-core.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/ui/spice-core.c b/ui/spice-core.c >> index 5746d0aae7..6eebf12e3c 100644 >> --- a/ui/spice-core.c >> +++ b/ui/spice-core.c >> @@ -830,6 +830,8 @@ static void qemu_spice_init(void) >> >> static int qemu_spice_add_interface(SpiceBaseInstance *sin) >> { >> + int ret; >> + >> if (!spice_server) { >> if (QTAILQ_FIRST(&qemu_spice_opts.head) != NULL) { >> error_report("Oops: spice configured but not active"); >> @@ -848,7 +850,13 @@ static int qemu_spice_add_interface(SpiceBaseInstance *sin) >> qemu_add_vm_change_state_handler(vm_change_state_handler, NULL); >> } >> >> - return spice_server_add_interface(spice_server, sin); >> + ret = spice_server_add_interface(spice_server, sin); >> + /* make sure the newly added interface is started */ >> + if (ret == 0 && spice_display_is_running) { >> + spice_server_vm_start(spice_server); >> + } >> + >> + return ret; >> } >> >> static GSList *spice_consoles; >> -- >> 2.29.0 >> >> > > Oops, it doesn't work reliably. There is some race in spice server now. > > spice_server_vm_start() sends RED_WORKER_MESSAGE_START to the QXL > worker thread. But if two of those come, it will assert... It should > probably not, I will send a patch to spice. > > I am looking for other options for QEMU though. >
Hi On Wed, Jan 27, 2021 at 2:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 27/01/21 11:18, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jan 27, 2021 at 2:03 PM <marcandre.lureau@redhat.com> wrote: > >> > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > >> Since commit b4e1a342112e50e05b609e857f38c1f2b7aafdc4 ("vl: remove > >> separate preconfig main_loop"), spice initialization is a bit dodgy, and > >> the client get stuck waiting for server events. > >> > >> The initialization order changed, so that qemu_spice_display_start() is > >> called before the display interfaces are added. The new interfaces > >> aren't started by spice-server automatically (yet?), so we have to tell > >> the server explicitely when we are already running. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > What is the exact different in initialization order once you add commit > facf7c60ee ("vl: initialize displays _after_ exiting preconfiguration", > 2021-01-02)? > We used to run qemu_spice.display_init() before vm_start() (in 5.2). Actually that commit didn't help in this case! It's a bit hard to track when things broke and how, since various commits created different issues. The current initialization order looks better, I am sending another patch solving this by delaying starting Spice. > Thanks, > > Paolo > > >> --- > >> ui/spice-core.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/ui/spice-core.c b/ui/spice-core.c > >> index 5746d0aae7..6eebf12e3c 100644 > >> --- a/ui/spice-core.c > >> +++ b/ui/spice-core.c > >> @@ -830,6 +830,8 @@ static void qemu_spice_init(void) > >> > >> static int qemu_spice_add_interface(SpiceBaseInstance *sin) > >> { > >> + int ret; > >> + > >> if (!spice_server) { > >> if (QTAILQ_FIRST(&qemu_spice_opts.head) != NULL) { > >> error_report("Oops: spice configured but not active"); > >> @@ -848,7 +850,13 @@ static int qemu_spice_add_interface(SpiceBaseInstance *sin) > >> qemu_add_vm_change_state_handler(vm_change_state_handler, NULL); > >> } > >> > >> - return spice_server_add_interface(spice_server, sin); > >> + ret = spice_server_add_interface(spice_server, sin); > >> + /* make sure the newly added interface is started */ > >> + if (ret == 0 && spice_display_is_running) { > >> + spice_server_vm_start(spice_server); > >> + } > >> + > >> + return ret; > >> } > >> > >> static GSList *spice_consoles; > >> -- > >> 2.29.0 > >> > >> > > > > Oops, it doesn't work reliably. There is some race in spice server now. > > > > spice_server_vm_start() sends RED_WORKER_MESSAGE_START to the QXL > > worker thread. But if two of those come, it will assert... It should > > probably not, I will send a patch to spice. > > > > I am looking for other options for QEMU though. > > >
diff --git a/ui/spice-core.c b/ui/spice-core.c index 5746d0aae7..6eebf12e3c 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -830,6 +830,8 @@ static void qemu_spice_init(void) static int qemu_spice_add_interface(SpiceBaseInstance *sin) { + int ret; + if (!spice_server) { if (QTAILQ_FIRST(&qemu_spice_opts.head) != NULL) { error_report("Oops: spice configured but not active"); @@ -848,7 +850,13 @@ static int qemu_spice_add_interface(SpiceBaseInstance *sin) qemu_add_vm_change_state_handler(vm_change_state_handler, NULL); } - return spice_server_add_interface(spice_server, sin); + ret = spice_server_add_interface(spice_server, sin); + /* make sure the newly added interface is started */ + if (ret == 0 && spice_display_is_running) { + spice_server_vm_start(spice_server); + } + + return ret; } static GSList *spice_consoles;