* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-27 6:26 ` [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize Fam Zheng
@ 2016-07-27 23:23 ` Colin Lord
2016-07-28 5:17 ` Fam Zheng
2016-08-01 10:31 ` Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Colin Lord @ 2016-07-27 23:23 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Paolo Bonzini, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-27 23:23 ` Colin Lord
@ 2016-07-28 5:17 ` Fam Zheng
2016-07-28 7:43 ` Fam Zheng
2016-08-01 10:33 ` Paolo Bonzini
0 siblings, 2 replies; 14+ messages in thread
From: Fam Zheng @ 2016-07-28 5:17 UTC (permalink / raw)
To: Colin Lord; +Cc: qemu-devel, Paolo Bonzini, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-28 5:17 ` Fam Zheng
@ 2016-07-28 7:43 ` Fam Zheng
2016-08-01 10:33 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2016-07-28 7:43 UTC (permalink / raw)
To: Colin Lord; +Cc: Paolo Bonzini, qemu-devel, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-28 5:17 ` Fam Zheng
2016-07-28 7:43 ` Fam Zheng
@ 2016-08-01 10:33 ` Paolo Bonzini
1 sibling, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2016-08-01 10:33 UTC (permalink / raw)
To: Fam Zheng, Colin Lord; +Cc: qemu-devel, Gerd Hoffmann
On 28/07/2016 07:17, Fam Zheng wrote:
> 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.
SDL Audio wants SDL, but it was reported to be broken last year.
Baum can place $(SDL_LIBS) in baum.o-libs, I guess.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-27 6:26 ` [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize Fam Zheng
2016-07-27 23:23 ` Colin Lord
@ 2016-08-01 10:31 ` Paolo Bonzini
2016-08-02 1:28 ` Fam Zheng
2016-08-01 10:39 ` Daniel P. Berrange
2016-08-01 11:53 ` Gerd Hoffmann
3 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2016-08-01 10:31 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Colin Lord, Gerd Hoffmann
On 27/07/2016 08:26, 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);
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.
Thanks,
Paolo
> 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) {
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-08-01 10:31 ` Paolo Bonzini
@ 2016-08-02 1:28 ` Fam Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2016-08-02 1:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Colin Lord, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-27 6:26 ` [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize Fam Zheng
2016-07-27 23:23 ` Colin Lord
2016-08-01 10:31 ` Paolo Bonzini
@ 2016-08-01 10:39 ` Daniel P. Berrange
2016-08-01 11:53 ` Gerd Hoffmann
3 siblings, 0 replies; 14+ messages in thread
From: Daniel P. Berrange @ 2016-08-01 10:39 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, Paolo Bonzini, Gerd Hoffmann, Colin Lord
On Wed, Jul 27, 2016 at 02:26:17PM +0800, 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
[snip]
> 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);
> + }
> }
SDL has some nasty stuff where it tries to replace main() with
a call to SDL_main on certain platforms. Merely #include'ing
SDL.h in vl.c causes this to happen on Win32 at least, perhaps
other platforms too. I'm unclear if your modularization here
will apply to all platforms or just Linux - g_module at least
can work on Win32.
Anyway, to fully support modularization without nasty surprises,
we probably want to stop this main() replacement.
I sent a patch for that last year
https://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg01631.html
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-07-27 6:26 ` [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize Fam Zheng
` (2 preceding siblings ...)
2016-08-01 10:39 ` Daniel P. Berrange
@ 2016-08-01 11:53 ` Gerd Hoffmann
2016-08-02 1:22 ` Fam Zheng
3 siblings, 1 reply; 14+ messages in thread
From: Gerd Hoffmann @ 2016-08-01 11:53 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, Colin Lord, Paolo Bonzini
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.
Will sdl be loaded unconditionally?
Or only with -display sdl?
What happens 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?
Can modules have dependencies on other modules? When it comes to
modularizing spice we will need that as qxl has a spice dependency ...
cheers,
Gerd
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize
2016-08-01 11:53 ` Gerd Hoffmann
@ 2016-08-02 1:22 ` Fam Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2016-08-02 1:22 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Colin Lord, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 14+ messages in thread