Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize

[Fam Zheng <famz@redhat.com>]

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