[RFC] sdl: initialize all graphic consoles

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

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

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.