All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ui: fix spice display regression
@ 2021-01-27 10:02 marcandre.lureau
  2021-01-27 10:18 ` Marc-André Lureau
  0 siblings, 1 reply; 4+ messages in thread
From: marcandre.lureau @ 2021-01-27 10:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, kraxel, Marc-André Lureau

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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] ui: fix spice display regression
  2021-01-27 10:02 [PATCH] ui: fix spice display regression marcandre.lureau
@ 2021-01-27 10:18 ` Marc-André Lureau
  2021-01-27 10:54   ` Paolo Bonzini
  0 siblings, 1 reply; 4+ messages in thread
From: Marc-André Lureau @ 2021-01-27 10:18 UTC (permalink / raw)
  To: QEMU; +Cc: Paolo Bonzini, Gerd Hoffmann

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.

-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ui: fix spice display regression
  2021-01-27 10:18 ` Marc-André Lureau
@ 2021-01-27 10:54   ` Paolo Bonzini
  2021-01-28 11:12     ` Marc-André Lureau
  0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2021-01-27 10:54 UTC (permalink / raw)
  To: Marc-André Lureau, QEMU; +Cc: Gerd Hoffmann

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ui: fix spice display regression
  2021-01-27 10:54   ` Paolo Bonzini
@ 2021-01-28 11:12     ` Marc-André Lureau
  0 siblings, 0 replies; 4+ messages in thread
From: Marc-André Lureau @ 2021-01-28 11:12 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU, Gerd Hoffmann

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


-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-01-28 11:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 10:02 [PATCH] ui: fix spice display regression marcandre.lureau
2021-01-27 10:18 ` Marc-André Lureau
2021-01-27 10:54   ` Paolo Bonzini
2021-01-28 11:12     ` Marc-André Lureau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.