diff mbox

[for,2.8,3/3] sdl: Modularize

Message ID 1469600777-30413-4-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 27, 2016, 6:26 a.m. UTC
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile.objs         |  1 +
 configure             |  4 +--
 include/qemu/module.h |  2 ++
 include/ui/console.h  |  5 ++--
 ui/Makefile.objs      |  2 ++
 ui/sdl-init.c         | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++
 ui/sdl.c              | 19 ++++++-------
 ui/sdl2.c             | 26 ++++++------------
 util/module.c         |  6 +++++
 vl.c                  |  5 +++-
 10 files changed, 111 insertions(+), 34 deletions(-)
 create mode 100644 ui/sdl-init.c

Comments

clord@redhat.com July 27, 2016, 11:23 p.m. UTC | #1
On 07/27/2016 02:26 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  Makefile.objs         |  1 +
>  configure             |  4 +--
>  include/qemu/module.h |  2 ++
>  include/ui/console.h  |  5 ++--
>  ui/Makefile.objs      |  2 ++
>  ui/sdl-init.c         | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  ui/sdl.c              | 19 ++++++-------
>  ui/sdl2.c             | 26 ++++++------------
>  util/module.c         |  6 +++++
>  vl.c                  |  5 +++-
>  10 files changed, 111 insertions(+), 34 deletions(-)
>  create mode 100644 ui/sdl-init.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 6d5ddcf..08c5746 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -62,6 +62,7 @@ common-obj-y += accel.o
>  common-obj-y += replay/
>  
>  common-obj-y += ui/
> +common-obj-m += ui/
>  common-obj-y += bt-host.o bt-vhci.o
>  bt-host.o-cflags := $(BLUEZ_CFLAGS)
>  
> diff --git a/configure b/configure
> index e04e59f..1b9be8f 100755
> --- a/configure
> +++ b/configure
> @@ -2533,7 +2533,6 @@ EOF
>      sdl_cflags="$sdl_cflags $x11_cflags"
>      sdl_libs="$sdl_libs $x11_libs"
>    fi
> -  libs_softmmu="$sdl_libs $libs_softmmu"
>  fi
>  
>  ##########################################
> @@ -5065,9 +5064,10 @@ if test "$modules" = "yes"; then
>    echo "CONFIG_MODULES=y" >> $config_host_mak
>  fi
>  if test "$sdl" = "yes" ; then
> -  echo "CONFIG_SDL=y" >> $config_host_mak
> +  echo "CONFIG_SDL=m" >> $config_host_mak
>    echo "CONFIG_SDLABI=$sdlabi" >> $config_host_mak
>    echo "SDL_CFLAGS=$sdl_cflags" >> $config_host_mak
> +  echo "SDL_LIBS=$sdl_libs" >> $config_host_mak
>  fi
>  if test "$sdlabi" = "2.0"; then
>    echo "CONFIG_SDL2=y" >> $config_host_mak
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 2370708..f5e012b 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -44,6 +44,7 @@ typedef enum {
>      MODULE_INIT_OPTS,
>      MODULE_INIT_QAPI,
>      MODULE_INIT_QOM,
> +    MODULE_INIT_SDL,
>      MODULE_INIT_MAX
>  } module_init_type;
>  
> @@ -51,6 +52,7 @@ typedef enum {
>  #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
>  #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
>  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> +#define sdl_init(function) module_init(function, MODULE_INIT_SDL)
>  
>  void register_module_init(void (*fn)(void), module_init_type type);
>  void register_dso_module_init(void (*fn)(void), module_init_type type);
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 2703a3a..1d9b0bb 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -425,10 +425,11 @@ void surface_gl_setup_viewport(ConsoleGLState *gls,
>  
>  /* sdl.c */
>  #ifdef CONFIG_SDL
> -void sdl_display_early_init(int opengl);
> +bool sdl_display_early_init(int opengl);
>  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
> +void sdl_register_init_fun(void *fn);
>  #else
> -static inline void sdl_display_early_init(int opengl)
> +static inline bool sdl_display_early_init(int opengl)
>  {
>      /* This must never be called if CONFIG_SDL is disabled */
>      error_report("SDL support is disabled");
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index dc936f1..0b82650 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -26,7 +26,9 @@ ifeq ($(CONFIG_OPENGL),y)
>  sdl.mo-objs += sdl2-gl.o
>  endif
>  endif
> +common-obj-y += sdl-init.o
>  sdl.mo-cflags := $(SDL_CFLAGS)
> +sdl.mo-libs := $(SDL_LIBS)
>  
>  ifeq ($(CONFIG_OPENGL),y)
>  common-obj-y += shader.o
> diff --git a/ui/sdl-init.c b/ui/sdl-init.c
> new file mode 100644
> index 0000000..6a780a4
> --- /dev/null
> +++ b/ui/sdl-init.c
> @@ -0,0 +1,75 @@
> +/*
> + * QEMU SDL display driver init function
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + * Copyright (c) 2016 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "ui/console.h"
> +#include "qemu/module.h"
> +
> +static void (*init_fn)(DisplayState *ds, int full_screen, int no_frame);
> +void sdl_register_init_fun(void *fn)
> +{
> +    assert(!init_fn);
> +    init_fn = fn;
> +}
> +
> +bool sdl_display_early_init(int opengl)
> +{
> +
> +#ifdef CONFIG_SDL2
> +    switch (opengl) {
> +    case -1: /* default */
> +    case 0:  /* off */
> +        break;
> +    case 1: /* on */
> +#ifdef CONFIG_OPENGL
> +        display_opengl = 1;
> +#endif
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +#else
> +    if (opengl == 1 /* on */) {
> +        fprintf(stderr,
> +                "SDL1 display code has no opengl support.\n"
> +                "Please recompile qemu with SDL2, using\n"
> +                "./configure --enable-sdl --with-sdlabi=2.0\n");
> +        /* XXX: Should we return false here? */
> +    }
> +#endif
> +
> +    module_call_init(MODULE_INIT_SDL);
> +    if (!init_fn) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +{
> +    assert(init_fn);
> +    init_fn(ds, full_screen, no_frame);
> +}
> diff --git a/ui/sdl.c b/ui/sdl.c
> index d8cf5bc..a30f442 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -932,17 +932,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_cursor_define    = sdl_mouse_define,
>  };
>  
> -void sdl_display_early_init(int opengl)
> -{
> -    if (opengl == 1 /* on */) {
> -        fprintf(stderr,
> -                "SDL1 display code has no opengl support.\n"
> -                "Please recompile qemu with SDL2, using\n"
> -                "./configure --enable-sdl --with-sdlabi=2.0\n");
> -    }
> -}
> -
> -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
>  {
>      int flags;
>      uint8_t data = 0;
> @@ -1025,3 +1015,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
>  
>      atexit(sdl_cleanup);
>  }
> +
> +static void sdl_init_fn(void)
> +{
> +    sdl_register_init_fun(sdl_display_init_do);
> +}
> +
> +sdl_init(sdl_init_fn);
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index 30d2a3c..c2b4049 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -738,24 +738,7 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
>  };
>  #endif
>  
> -void sdl_display_early_init(int opengl)
> -{
> -    switch (opengl) {
> -    case -1: /* default */
> -    case 0:  /* off */
> -        break;
> -    case 1: /* on */
> -#ifdef CONFIG_OPENGL
> -        display_opengl = 1;
> -#endif
> -        break;
> -    default:
> -        g_assert_not_reached();
> -        break;
> -    }
> -}
> -
> -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
>  {
>      int flags;
>      uint8_t data = 0;
> @@ -842,3 +825,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
>  
>      atexit(sdl_cleanup);
>  }
> +
> +static void sdl_init_fn(void)
> +{
> +    sdl_register_init_fun(sdl_display_init_do);
> +}
> +
> +sdl_init(sdl_init_fn);
> diff --git a/util/module.c b/util/module.c
> index 86e3f7a..b95d4fa 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -167,6 +167,9 @@ static void module_load(module_init_type type)
>      static const char *block_modules[] = {
>          CONFIG_BLOCK_MODULES
>      };
> +    static const char *sdl_modules[] = {
> +        "ui-sdl",
> +    };
>      char *exec_dir;
>      char *dirs[3];
>      int i = 0;
> @@ -181,6 +184,9 @@ static void module_load(module_init_type type)
>      case MODULE_INIT_BLOCK:
>          mp = block_modules;
>          break;
> +    case MODULE_INIT_SDL:
> +        mp = sdl_modules;
> +        break;
>      default:
>          /* no other types have dynamic modules for now*/
>          return;
> diff --git a/vl.c b/vl.c
> index a455947..fdbbe47 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4221,7 +4221,10 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (display_type == DT_SDL) {
> -        sdl_display_early_init(request_opengl);
> +        if (!sdl_display_early_init(request_opengl)) {
> +            error_report("Failed to initialize SDL");
> +            exit(1);
> +        }
>      }
>  
>      if (request_opengl == 1 && display_opengl == 0) {
> 
Maybe I'm doing something wrong, but when I apply this third patch
(along with the first two) to master it doesn't seem to build when
modules are enabled in the configuration:

  LINK  x86_64-softmmu/qemu-system-x86_64
../backends/baum.o: In function `chr_baum_init':
/home/bos/clord/Documents/qemu/backends/baum.c:616: undefined reference
to `SDL_GetWMInfo'
collect2: error: ld returned 1 exit status
Makefile:197: recipe for target 'qemu-system-x86_64' failed
make[1]: *** [qemu-system-x86_64] Error 1
Makefile:204: recipe for target 'subdir-x86_64-softmmu' failed
make: *** [subdir-x86_64-softmmu] Error 2

Colin
Fam Zheng July 28, 2016, 5:17 a.m. UTC | #2
On Wed, 07/27 19:23, Colin Lord wrote:
> On 07/27/2016 02:26 AM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  Makefile.objs         |  1 +
> >  configure             |  4 +--
> >  include/qemu/module.h |  2 ++
> >  include/ui/console.h  |  5 ++--
> >  ui/Makefile.objs      |  2 ++
> >  ui/sdl-init.c         | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  ui/sdl.c              | 19 ++++++-------
> >  ui/sdl2.c             | 26 ++++++------------
> >  util/module.c         |  6 +++++
> >  vl.c                  |  5 +++-
> >  10 files changed, 111 insertions(+), 34 deletions(-)
> >  create mode 100644 ui/sdl-init.c
> > 
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 6d5ddcf..08c5746 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -62,6 +62,7 @@ common-obj-y += accel.o
> >  common-obj-y += replay/
> >  
> >  common-obj-y += ui/
> > +common-obj-m += ui/
> >  common-obj-y += bt-host.o bt-vhci.o
> >  bt-host.o-cflags := $(BLUEZ_CFLAGS)
> >  
> > diff --git a/configure b/configure
> > index e04e59f..1b9be8f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2533,7 +2533,6 @@ EOF
> >      sdl_cflags="$sdl_cflags $x11_cflags"
> >      sdl_libs="$sdl_libs $x11_libs"
> >    fi
> > -  libs_softmmu="$sdl_libs $libs_softmmu"
> >  fi
> >  
> >  ##########################################
> > @@ -5065,9 +5064,10 @@ if test "$modules" = "yes"; then
> >    echo "CONFIG_MODULES=y" >> $config_host_mak
> >  fi
> >  if test "$sdl" = "yes" ; then
> > -  echo "CONFIG_SDL=y" >> $config_host_mak
> > +  echo "CONFIG_SDL=m" >> $config_host_mak
> >    echo "CONFIG_SDLABI=$sdlabi" >> $config_host_mak
> >    echo "SDL_CFLAGS=$sdl_cflags" >> $config_host_mak
> > +  echo "SDL_LIBS=$sdl_libs" >> $config_host_mak
> >  fi
> >  if test "$sdlabi" = "2.0"; then
> >    echo "CONFIG_SDL2=y" >> $config_host_mak
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 2370708..f5e012b 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -44,6 +44,7 @@ typedef enum {
> >      MODULE_INIT_OPTS,
> >      MODULE_INIT_QAPI,
> >      MODULE_INIT_QOM,
> > +    MODULE_INIT_SDL,
> >      MODULE_INIT_MAX
> >  } module_init_type;
> >  
> > @@ -51,6 +52,7 @@ typedef enum {
> >  #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
> >  #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
> >  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > +#define sdl_init(function) module_init(function, MODULE_INIT_SDL)
> >  
> >  void register_module_init(void (*fn)(void), module_init_type type);
> >  void register_dso_module_init(void (*fn)(void), module_init_type type);
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 2703a3a..1d9b0bb 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -425,10 +425,11 @@ void surface_gl_setup_viewport(ConsoleGLState *gls,
> >  
> >  /* sdl.c */
> >  #ifdef CONFIG_SDL
> > -void sdl_display_early_init(int opengl);
> > +bool sdl_display_early_init(int opengl);
> >  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
> > +void sdl_register_init_fun(void *fn);
> >  #else
> > -static inline void sdl_display_early_init(int opengl)
> > +static inline bool sdl_display_early_init(int opengl)
> >  {
> >      /* This must never be called if CONFIG_SDL is disabled */
> >      error_report("SDL support is disabled");
> > diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> > index dc936f1..0b82650 100644
> > --- a/ui/Makefile.objs
> > +++ b/ui/Makefile.objs
> > @@ -26,7 +26,9 @@ ifeq ($(CONFIG_OPENGL),y)
> >  sdl.mo-objs += sdl2-gl.o
> >  endif
> >  endif
> > +common-obj-y += sdl-init.o
> >  sdl.mo-cflags := $(SDL_CFLAGS)
> > +sdl.mo-libs := $(SDL_LIBS)
> >  
> >  ifeq ($(CONFIG_OPENGL),y)
> >  common-obj-y += shader.o
> > diff --git a/ui/sdl-init.c b/ui/sdl-init.c
> > new file mode 100644
> > index 0000000..6a780a4
> > --- /dev/null
> > +++ b/ui/sdl-init.c
> > @@ -0,0 +1,75 @@
> > +/*
> > + * QEMU SDL display driver init function
> > + *
> > + * Copyright (c) 2003 Fabrice Bellard
> > + * Copyright (c) 2016 Red Hat, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "ui/console.h"
> > +#include "qemu/module.h"
> > +
> > +static void (*init_fn)(DisplayState *ds, int full_screen, int no_frame);
> > +void sdl_register_init_fun(void *fn)
> > +{
> > +    assert(!init_fn);
> > +    init_fn = fn;
> > +}
> > +
> > +bool sdl_display_early_init(int opengl)
> > +{
> > +
> > +#ifdef CONFIG_SDL2
> > +    switch (opengl) {
> > +    case -1: /* default */
> > +    case 0:  /* off */
> > +        break;
> > +    case 1: /* on */
> > +#ifdef CONFIG_OPENGL
> > +        display_opengl = 1;
> > +#endif
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +        break;
> > +    }
> > +#else
> > +    if (opengl == 1 /* on */) {
> > +        fprintf(stderr,
> > +                "SDL1 display code has no opengl support.\n"
> > +                "Please recompile qemu with SDL2, using\n"
> > +                "./configure --enable-sdl --with-sdlabi=2.0\n");
> > +        /* XXX: Should we return false here? */
> > +    }
> > +#endif
> > +
> > +    module_call_init(MODULE_INIT_SDL);
> > +    if (!init_fn) {
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> > +void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> > +{
> > +    assert(init_fn);
> > +    init_fn(ds, full_screen, no_frame);
> > +}
> > diff --git a/ui/sdl.c b/ui/sdl.c
> > index d8cf5bc..a30f442 100644
> > --- a/ui/sdl.c
> > +++ b/ui/sdl.c
> > @@ -932,17 +932,7 @@ static const DisplayChangeListenerOps dcl_ops = {
> >      .dpy_cursor_define    = sdl_mouse_define,
> >  };
> >  
> > -void sdl_display_early_init(int opengl)
> > -{
> > -    if (opengl == 1 /* on */) {
> > -        fprintf(stderr,
> > -                "SDL1 display code has no opengl support.\n"
> > -                "Please recompile qemu with SDL2, using\n"
> > -                "./configure --enable-sdl --with-sdlabi=2.0\n");
> > -    }
> > -}
> > -
> > -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> > +static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
> >  {
> >      int flags;
> >      uint8_t data = 0;
> > @@ -1025,3 +1015,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> >  
> >      atexit(sdl_cleanup);
> >  }
> > +
> > +static void sdl_init_fn(void)
> > +{
> > +    sdl_register_init_fun(sdl_display_init_do);
> > +}
> > +
> > +sdl_init(sdl_init_fn);
> > diff --git a/ui/sdl2.c b/ui/sdl2.c
> > index 30d2a3c..c2b4049 100644
> > --- a/ui/sdl2.c
> > +++ b/ui/sdl2.c
> > @@ -738,24 +738,7 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
> >  };
> >  #endif
> >  
> > -void sdl_display_early_init(int opengl)
> > -{
> > -    switch (opengl) {
> > -    case -1: /* default */
> > -    case 0:  /* off */
> > -        break;
> > -    case 1: /* on */
> > -#ifdef CONFIG_OPENGL
> > -        display_opengl = 1;
> > -#endif
> > -        break;
> > -    default:
> > -        g_assert_not_reached();
> > -        break;
> > -    }
> > -}
> > -
> > -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> > +static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
> >  {
> >      int flags;
> >      uint8_t data = 0;
> > @@ -842,3 +825,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> >  
> >      atexit(sdl_cleanup);
> >  }
> > +
> > +static void sdl_init_fn(void)
> > +{
> > +    sdl_register_init_fun(sdl_display_init_do);
> > +}
> > +
> > +sdl_init(sdl_init_fn);
> > diff --git a/util/module.c b/util/module.c
> > index 86e3f7a..b95d4fa 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -167,6 +167,9 @@ static void module_load(module_init_type type)
> >      static const char *block_modules[] = {
> >          CONFIG_BLOCK_MODULES
> >      };
> > +    static const char *sdl_modules[] = {
> > +        "ui-sdl",
> > +    };
> >      char *exec_dir;
> >      char *dirs[3];
> >      int i = 0;
> > @@ -181,6 +184,9 @@ static void module_load(module_init_type type)
> >      case MODULE_INIT_BLOCK:
> >          mp = block_modules;
> >          break;
> > +    case MODULE_INIT_SDL:
> > +        mp = sdl_modules;
> > +        break;
> >      default:
> >          /* no other types have dynamic modules for now*/
> >          return;
> > diff --git a/vl.c b/vl.c
> > index a455947..fdbbe47 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4221,7 +4221,10 @@ int main(int argc, char **argv, char **envp)
> >      }
> >  
> >      if (display_type == DT_SDL) {
> > -        sdl_display_early_init(request_opengl);
> > +        if (!sdl_display_early_init(request_opengl)) {
> > +            error_report("Failed to initialize SDL");
> > +            exit(1);
> > +        }
> >      }
> >  
> >      if (request_opengl == 1 && display_opengl == 0) {
> > 
> Maybe I'm doing something wrong, but when I apply this third patch
> (along with the first two) to master it doesn't seem to build when
> modules are enabled in the configuration:
> 
>   LINK  x86_64-softmmu/qemu-system-x86_64
> ../backends/baum.o: In function `chr_baum_init':
> /home/bos/clord/Documents/qemu/backends/baum.c:616: undefined reference
> to `SDL_GetWMInfo'
> collect2: error: ld returned 1 exit status
> Makefile:197: recipe for target 'qemu-system-x86_64' failed
> make[1]: *** [qemu-system-x86_64] Error 1
> Makefile:204: recipe for target 'subdir-x86_64-softmmu' failed
> make: *** [subdir-x86_64-softmmu] Error 2

You are right, looks like audio and baum both want SDL library. I need to take
another look.

Fam
Fam Zheng July 28, 2016, 7:43 a.m. UTC | #3
On Thu, 07/28 13:17, Fam Zheng wrote:
> > Maybe I'm doing something wrong, but when I apply this third patch
> > (along with the first two) to master it doesn't seem to build when
> > modules are enabled in the configuration:
> > 
> >   LINK  x86_64-softmmu/qemu-system-x86_64
> > ../backends/baum.o: In function `chr_baum_init':
> > /home/bos/clord/Documents/qemu/backends/baum.c:616: undefined reference
> > to `SDL_GetWMInfo'
> > collect2: error: ld returned 1 exit status
> > Makefile:197: recipe for target 'qemu-system-x86_64' failed
> > make[1]: *** [qemu-system-x86_64] Error 1
> > Makefile:204: recipe for target 'subdir-x86_64-softmmu' failed
> > make: *** [subdir-x86_64-softmmu] Error 2
> 
> You are right, looks like audio and baum both want SDL library. I need to take
> another look.

We need to modularize backends/baum.c and add appropriate loading code
somewhere (probably when it is specifically selected).

In addition, audio/sdl.o should probably to be bundled together with ui/sdl.mo.

I'll leave this series for now and you can pick up patch 1 into your series if
it helps. Also feel free to pick up modularizing SDL and baum, if you want.
Otherwise, I'll revisit these when your "load on demand" work settles down.

Fam
Fam Zheng Aug. 2, 2016, 1:22 a.m. UTC | #4
On Mon, 08/01 13:53, Gerd Hoffmann wrote:
> On Mi, 2016-07-27 at 14:26 +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> more verbose commit message please, especially for those (like me) who
> have not worked yet with modules.

Yeah, my bad. Maybe I should have put RFC to the subject too.

> 
> Will sdl be loaded unconditionally?
> Or only with -display sdl?
> What happens if the module is not present?

Only with -display sdl, and report error + exit if the module is not present.

> 
> The main benefit I see in modularizing the ui is that we can move the ui
> modules (and the UI libs dependency chain) to a separate rpm subpackage,
> can we start that with this series applied?

Yes, that is the intention, but we need v2 to fix SDL audio and baum to achieve
that.

Colin also tested that not loading gui shared libraries (gtk?) speeds up
starting up.

> 
> Can modules have dependencies on other modules?  When it comes to
> modularizing spice we will need that as qxl has a spice dependency ...

Yes. spice's modules init function can call module_call_init(MODULE_INIT_QXL),
then check if QXL is available after that.

Fam
Fam Zheng Aug. 2, 2016, 1:28 a.m. UTC | #5
On Mon, 08/01 12:31, Paolo Bonzini wrote:
> > +
> > +static void sdl_init_fn(void)
> > +{
> > +    sdl_register_init_fun(sdl_display_init_do);
> > +}
> > +
> > +sdl_init(sdl_init_fn);
> 
> Can you use __attribute__((constructor)) instead of going through the
> burden of defining a new type?  The registration function only does a
> single assignment, so it doesn't really have ordering dependencies.

Yeah, it makes sense! Thanks.

Fam
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 6d5ddcf..08c5746 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -62,6 +62,7 @@  common-obj-y += accel.o
 common-obj-y += replay/
 
 common-obj-y += ui/
+common-obj-m += ui/
 common-obj-y += bt-host.o bt-vhci.o
 bt-host.o-cflags := $(BLUEZ_CFLAGS)
 
diff --git a/configure b/configure
index e04e59f..1b9be8f 100755
--- a/configure
+++ b/configure
@@ -2533,7 +2533,6 @@  EOF
     sdl_cflags="$sdl_cflags $x11_cflags"
     sdl_libs="$sdl_libs $x11_libs"
   fi
-  libs_softmmu="$sdl_libs $libs_softmmu"
 fi
 
 ##########################################
@@ -5065,9 +5064,10 @@  if test "$modules" = "yes"; then
   echo "CONFIG_MODULES=y" >> $config_host_mak
 fi
 if test "$sdl" = "yes" ; then
-  echo "CONFIG_SDL=y" >> $config_host_mak
+  echo "CONFIG_SDL=m" >> $config_host_mak
   echo "CONFIG_SDLABI=$sdlabi" >> $config_host_mak
   echo "SDL_CFLAGS=$sdl_cflags" >> $config_host_mak
+  echo "SDL_LIBS=$sdl_libs" >> $config_host_mak
 fi
 if test "$sdlabi" = "2.0"; then
   echo "CONFIG_SDL2=y" >> $config_host_mak
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 2370708..f5e012b 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -44,6 +44,7 @@  typedef enum {
     MODULE_INIT_OPTS,
     MODULE_INIT_QAPI,
     MODULE_INIT_QOM,
+    MODULE_INIT_SDL,
     MODULE_INIT_MAX
 } module_init_type;
 
@@ -51,6 +52,7 @@  typedef enum {
 #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
 #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
 #define type_init(function) module_init(function, MODULE_INIT_QOM)
+#define sdl_init(function) module_init(function, MODULE_INIT_SDL)
 
 void register_module_init(void (*fn)(void), module_init_type type);
 void register_dso_module_init(void (*fn)(void), module_init_type type);
diff --git a/include/ui/console.h b/include/ui/console.h
index 2703a3a..1d9b0bb 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -425,10 +425,11 @@  void surface_gl_setup_viewport(ConsoleGLState *gls,
 
 /* sdl.c */
 #ifdef CONFIG_SDL
-void sdl_display_early_init(int opengl);
+bool sdl_display_early_init(int opengl);
 void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
+void sdl_register_init_fun(void *fn);
 #else
-static inline void sdl_display_early_init(int opengl)
+static inline bool sdl_display_early_init(int opengl)
 {
     /* This must never be called if CONFIG_SDL is disabled */
     error_report("SDL support is disabled");
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index dc936f1..0b82650 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -26,7 +26,9 @@  ifeq ($(CONFIG_OPENGL),y)
 sdl.mo-objs += sdl2-gl.o
 endif
 endif
+common-obj-y += sdl-init.o
 sdl.mo-cflags := $(SDL_CFLAGS)
+sdl.mo-libs := $(SDL_LIBS)
 
 ifeq ($(CONFIG_OPENGL),y)
 common-obj-y += shader.o
diff --git a/ui/sdl-init.c b/ui/sdl-init.c
new file mode 100644
index 0000000..6a780a4
--- /dev/null
+++ b/ui/sdl-init.c
@@ -0,0 +1,75 @@ 
+/*
+ * QEMU SDL display driver init function
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2016 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "ui/console.h"
+#include "qemu/module.h"
+
+static void (*init_fn)(DisplayState *ds, int full_screen, int no_frame);
+void sdl_register_init_fun(void *fn)
+{
+    assert(!init_fn);
+    init_fn = fn;
+}
+
+bool sdl_display_early_init(int opengl)
+{
+
+#ifdef CONFIG_SDL2
+    switch (opengl) {
+    case -1: /* default */
+    case 0:  /* off */
+        break;
+    case 1: /* on */
+#ifdef CONFIG_OPENGL
+        display_opengl = 1;
+#endif
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+#else
+    if (opengl == 1 /* on */) {
+        fprintf(stderr,
+                "SDL1 display code has no opengl support.\n"
+                "Please recompile qemu with SDL2, using\n"
+                "./configure --enable-sdl --with-sdlabi=2.0\n");
+        /* XXX: Should we return false here? */
+    }
+#endif
+
+    module_call_init(MODULE_INIT_SDL);
+    if (!init_fn) {
+        return false;
+    }
+    return true;
+}
+
+void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
+{
+    assert(init_fn);
+    init_fn(ds, full_screen, no_frame);
+}
diff --git a/ui/sdl.c b/ui/sdl.c
index d8cf5bc..a30f442 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -932,17 +932,7 @@  static const DisplayChangeListenerOps dcl_ops = {
     .dpy_cursor_define    = sdl_mouse_define,
 };
 
-void sdl_display_early_init(int opengl)
-{
-    if (opengl == 1 /* on */) {
-        fprintf(stderr,
-                "SDL1 display code has no opengl support.\n"
-                "Please recompile qemu with SDL2, using\n"
-                "./configure --enable-sdl --with-sdlabi=2.0\n");
-    }
-}
-
-void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
+static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
 {
     int flags;
     uint8_t data = 0;
@@ -1025,3 +1015,10 @@  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
 
     atexit(sdl_cleanup);
 }
+
+static void sdl_init_fn(void)
+{
+    sdl_register_init_fun(sdl_display_init_do);
+}
+
+sdl_init(sdl_init_fn);
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 30d2a3c..c2b4049 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -738,24 +738,7 @@  static const DisplayChangeListenerOps dcl_gl_ops = {
 };
 #endif
 
-void sdl_display_early_init(int opengl)
-{
-    switch (opengl) {
-    case -1: /* default */
-    case 0:  /* off */
-        break;
-    case 1: /* on */
-#ifdef CONFIG_OPENGL
-        display_opengl = 1;
-#endif
-        break;
-    default:
-        g_assert_not_reached();
-        break;
-    }
-}
-
-void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
+static void sdl_display_init_do(DisplayState *ds, int full_screen, int no_frame)
 {
     int flags;
     uint8_t data = 0;
@@ -842,3 +825,10 @@  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
 
     atexit(sdl_cleanup);
 }
+
+static void sdl_init_fn(void)
+{
+    sdl_register_init_fun(sdl_display_init_do);
+}
+
+sdl_init(sdl_init_fn);
diff --git a/util/module.c b/util/module.c
index 86e3f7a..b95d4fa 100644
--- a/util/module.c
+++ b/util/module.c
@@ -167,6 +167,9 @@  static void module_load(module_init_type type)
     static const char *block_modules[] = {
         CONFIG_BLOCK_MODULES
     };
+    static const char *sdl_modules[] = {
+        "ui-sdl",
+    };
     char *exec_dir;
     char *dirs[3];
     int i = 0;
@@ -181,6 +184,9 @@  static void module_load(module_init_type type)
     case MODULE_INIT_BLOCK:
         mp = block_modules;
         break;
+    case MODULE_INIT_SDL:
+        mp = sdl_modules;
+        break;
     default:
         /* no other types have dynamic modules for now*/
         return;
diff --git a/vl.c b/vl.c
index a455947..fdbbe47 100644
--- a/vl.c
+++ b/vl.c
@@ -4221,7 +4221,10 @@  int main(int argc, char **argv, char **envp)
     }
 
     if (display_type == DT_SDL) {
-        sdl_display_early_init(request_opengl);
+        if (!sdl_display_early_init(request_opengl)) {
+            error_report("Failed to initialize SDL");
+            exit(1);
+        }
     }
 
     if (request_opengl == 1 && display_opengl == 0) {