[Qemu-devel] [BUG] Multiple graphical consoles are broken (black screen)

[Qemu-devel] [BUG] Multiple graphical consoles are broken (black screen)

[Hervé Poussineau]

Hi,

MIPS Magnum machine emulation is quite specific because it instanciates
two graphical consoles.

This has worked for a while, up to following commit which breaks it
badly (qemu crashes):

commit 3023f3329d87a6203d03a0e9ccb948772940da96
Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Fri Jan 16 19:04:14 2009 +0000

    graphical_console_init change (Stefano Stabellini)
    
    Patch 5/7
    
    This patch changes the graphical_console_init function to return an
    allocated DisplayState instead of a QEMUConsole.
    
    This patch contains just the graphical_console_init change and few other
    modifications mainly in console.c and vl.c.
    It was necessary to move the display frontends (e.g. sdl and vnc)
    initialization after machine->init in vl.c.
    
    This patch does *not* include any required changes to any device, these
    changes come with the following patches.
    
    Patch 6/7
    
    This patch changes the QEMUMachine init functions not to take a
    DisplayState as an argument because is not needed any more;
    
    In few places the graphic hardware initialization function was called
    only if DisplayState was not NULL, now they are always called.
    Apart from these cases, the rest are all mechanical substitutions.
    
    Patch 7/7
    
    This patch updates the graphic device code to use the new
    graphical_console_init function.
    
    As for the previous patch, in few places graphical_console_init was called
    only if DisplayState was not NULL, now it is always called.
    Apart from these cases, the rest are all mechanical substitutions.
    
    Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>


Few commits later, the following commit fixed the crash, but still kept the
second graphical console unusable: it stays blacks and the window size is not right.

commit 42aa98e8843aa8e3e1f35991f4be63eab2417e94
Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Fri Jan 16 21:01:48 2009 +0000

    Remove assumption about a single graphic console.
    
    This fixes a fault with the jazz_led since it has two graphic consoles.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>



Following patch is an attempt to work-around the problem, at least
for SDL display.

I hope someone will be able to pin point the problem, and find the right solution.
To reproduce the problem, run:

    mips64el-softmmu -M magnum -bios /path/to/NTPROM.raw

and try to switch to second console with Ctrl-Alt-2. You should see a small
console with some lines on it.

Hervé Poussineau (1):
  sdl: initialize all graphic consoles

 console.c |    3 +++
 vl.c      |   30 +++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

-- 
1.7.9

[Qemu-devel] [RFC] sdl: initialize all graphic consoles

[Hervé Poussineau]

MIPS Jazz emulation registers two graphical consoles, but second one stays black.
This patch repairs it.

Other display methods (cocoa, vnc...) also probably require the same kind of fix.
---
 console.c |    3 +++
 vl.c      |   30 +++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/console.c b/console.c
index 135394f..2c413a7 100644
--- a/console.c
+++ b/console.c
@@ -1376,6 +1376,9 @@ DisplayState *get_displaystate(void)
     if (!display_state) {
         dumb_display_init ();
     }
+    if (active_console && active_console->ds) {
+        return active_console->ds;
+    }
     return display_state;
 }
 
diff --git a/vl.c b/vl.c
index 7a8cc08..98e0091 100644
--- a/vl.c
+++ b/vl.c
@@ -3451,8 +3451,14 @@ int main(int argc, char **argv, char **envp)
 #endif
 #if defined(CONFIG_SDL)
     case DT_SDL:
-        sdl_display_init(ds, full_screen, no_frame);
+    {
+        DisplayState *ds2 = ds;
+        while (ds2) {
+            sdl_display_init(ds2, full_screen, no_frame);
+            ds2 = ds2->next;
+        }
         break;
+    }
 #elif defined(CONFIG_COCOA)
     case DT_SDL:
         cocoa_display_init(ds, full_screen);
@@ -3484,15 +3490,21 @@ int main(int argc, char **argv, char **envp)
 #endif
 
     /* display setup */
-    dpy_resize(ds);
-    dcl = ds->listeners;
-    while (dcl != NULL) {
-        if (dcl->dpy_refresh != NULL) {
-            ds->gui_timer = qemu_new_timer_ms(rt_clock, gui_update, ds);
-            qemu_mod_timer(ds->gui_timer, qemu_get_clock_ms(rt_clock));
-            break;
+    {
+        DisplayState *ds2 = ds;
+        while (ds2 != NULL) {
+            dpy_resize(ds2);
+            dcl = ds2->listeners;
+            while (dcl != NULL) {
+                if (dcl->dpy_refresh != NULL) {
+                    ds2->gui_timer = qemu_new_timer_ms(rt_clock, gui_update, ds2);
+                    qemu_mod_timer(ds2->gui_timer, qemu_get_clock_ms(rt_clock));
+                    break;
+                }
+                dcl = dcl->next;
+            }
+            ds2 = ds2->next;
         }
-        dcl = dcl->next;
     }
     text_consoles_set_display(ds);
 
-- 
1.7.9

Re: [Qemu-devel] [RFC] sdl: initialize all graphic consoles

[Anthony Liguori]

On 02/23/2012 03:39 PM, Hervé Poussineau wrote:
> MIPS Jazz emulation registers two graphical consoles, but second one stays black.
> This patch repairs it.
>
> Other display methods (cocoa, vnc...) also probably require the same kind of fix.

I don't think this is really the right way to solve this problem.

A bunch of assumptions are being made here and since this has been "broken" for 
some years now, I'm not sure that I really view this as a bug.

> ---
>   console.c |    3 +++
>   vl.c      |   30 +++++++++++++++++++++---------
>   2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/console.c b/console.c
> index 135394f..2c413a7 100644
> --- a/console.c
> +++ b/console.c
> @@ -1376,6 +1376,9 @@ DisplayState *get_displaystate(void)
>       if (!display_state) {
>           dumb_display_init ();
>       }
> +    if (active_console&&  active_console->ds) {
> +        return active_console->ds;
> +    }
>       return display_state;
>   }
>
> diff --git a/vl.c b/vl.c
> index 7a8cc08..98e0091 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3451,8 +3451,14 @@ int main(int argc, char **argv, char **envp)
>   #endif
>   #if defined(CONFIG_SDL)
>       case DT_SDL:
> -        sdl_display_init(ds, full_screen, no_frame);
> +    {
> +        DisplayState *ds2 = ds;
> +        while (ds2) {
> +            sdl_display_init(ds2, full_screen, no_frame);
> +            ds2 = ds2->next;

The fact that this works at all really surprises me.  You're registering double 
input event handlers and doing a number of things that have a global state.

sdl_display_init() isn't meant to be called twice.  I really think more 
substantial refactoring is needed such that we're not maintaining the UI state 
as globals and can independently instantiate a backend for a given DisplayState.

I had some patches that I posted a bit ago that started down this direction.

Regards,

Anthony Liguori

> +        }
>           break;
> +    }
>   #elif defined(CONFIG_COCOA)
>       case DT_SDL:
>           cocoa_display_init(ds, full_screen);
> @@ -3484,15 +3490,21 @@ int main(int argc, char **argv, char **envp)
>   #endif
>
>       /* display setup */
> -    dpy_resize(ds);
> -    dcl = ds->listeners;
> -    while (dcl != NULL) {
> -        if (dcl->dpy_refresh != NULL) {
> -            ds->gui_timer = qemu_new_timer_ms(rt_clock, gui_update, ds);
> -            qemu_mod_timer(ds->gui_timer, qemu_get_clock_ms(rt_clock));
> -            break;
> +    {
> +        DisplayState *ds2 = ds;
> +        while (ds2 != NULL) {
> +            dpy_resize(ds2);
> +            dcl = ds2->listeners;
> +            while (dcl != NULL) {
> +                if (dcl->dpy_refresh != NULL) {
> +                    ds2->gui_timer = qemu_new_timer_ms(rt_clock, gui_update, ds2);
> +                    qemu_mod_timer(ds2->gui_timer, qemu_get_clock_ms(rt_clock));
> +                    break;
> +                }
> +                dcl = dcl->next;
> +            }
> +            ds2 = ds2->next;
>           }
> -        dcl = dcl->next;
>       }
>       text_consoles_set_display(ds);
>

Re: [Qemu-devel] [RFC] sdl: initialize all graphic consoles

[Stefano Stabellini]

On Thu, 23 Feb 2012, Anthony Liguori wrote:
> > diff --git a/console.c b/console.c
> > index 135394f..2c413a7 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -1376,6 +1376,9 @@ DisplayState *get_displaystate(void)
> >       if (!display_state) {
> >           dumb_display_init ();
> >       }
> > +    if (active_console&&  active_console->ds) {
> > +        return active_console->ds;
> > +    }
> >       return display_state;
> >   }

You should be wary of all the callers of this function because they
probably assume that there is just one DisplayState and may not cope
well with a multiple DisplayState scenario.
It might be better to rename this function "get_active_displaystate" or
"get_current_displaystate". Even better would be to replace it entirely
with a for_each_display_state type of iterator, see for example
pci_for_each_device or irq_domain_for_each_irq in Linux.


> > diff --git a/vl.c b/vl.c
> > index 7a8cc08..98e0091 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3451,8 +3451,14 @@ int main(int argc, char **argv, char **envp)
> >   #endif
> >   #if defined(CONFIG_SDL)
> >       case DT_SDL:
> > -        sdl_display_init(ds, full_screen, no_frame);
> > +    {
> > +        DisplayState *ds2 = ds;
> > +        while (ds2) {
> > +            sdl_display_init(ds2, full_screen, no_frame);
> > +            ds2 = ds2->next;
> 
> The fact that this works at all really surprises me.  You're registering double 
> input event handlers and doing a number of things that have a global state.
> 
> sdl_display_init() isn't meant to be called twice.  I really think more 
> substantial refactoring is needed such that we're not maintaining the UI state 
> as globals and can independently instantiate a backend for a given DisplayState.
> 
> I had some patches that I posted a bit ago that started down this direction.

Like Anthony wrote, you probably need a more substantial refactoring to
make this work, but the basic idea that you can call
graphic_console_init twice to instantiate two DisplayState instances is
correct.
Then you should be able to call sdl_display_init on all of them independently.
If sdl_display_init (or any other display_init function) modifies a
single global state, that needs to be fixed.