Patchwork [RFC] sdl: initialize all graphic consoles

login
register
mail settings
Submitter Hervé Poussineau
Date Feb. 23, 2012, 9:39 p.m.
Message ID <1330033159-15860-2-git-send-email-hpoussin@reactos.org>
Download mbox | patch
Permalink /patch/142708/
State New
Headers show

Comments

Hervé Poussineau - Feb. 23, 2012, 9:39 p.m.
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(-)
Anthony Liguori - Feb. 23, 2012, 10:18 p.m.
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);
>
Stefano Stabellini - Feb. 24, 2012, 2:10 p.m.
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.

Patch

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