On Fri, Jun 16, 2023 at 01:52:27PM +0200, Julian Andres Klode wrote: > So all we did with the surface in SDL1 was split into window, > surface, renderer, and texture. Instead of drawing into the > surface and then flipping, you build your pixels, then update > a texture, and then copy the texture to the renderer. > > Here we use an empty RGB surface to hold our Pixels, which enables > us to keep most of the code the same. The SDL1 code has been adjusted > to refer to `surface` instead of `window` when trying to access the > properties of the surface. > > This approaches the configuration by adding a new --enable-grub-emu-sdl2 > argument. If set to yes, or auto detected, it disables SDL1 support > automatically.
I think I prefer this approach. > This duplicates the `sdl` module block in Makefile.core.def which may > be something to be aware of, but we also don't want to build separate > module. > > Bug-Debian: https://bugs.debian.org/1038035 > Signed-off-by: Julian Andres Klode <julian.kl...@canonical.com> > --- > configure.ac | 34 ++++++++++++ > grub-core/Makefile.am | 3 + > grub-core/Makefile.core.def | 12 +++- > grub-core/video/emu/sdl.c | 108 +++++++++++++++++++++++++++++------- > include/grub/sdl.h | 16 +++++- > 5 files changed, 150 insertions(+), 23 deletions(-) > > diff --git a/configure.ac b/configure.ac > index d9f088d12..7747582df 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1563,6 +1563,10 @@ else > fi > AC_SUBST([BOOT_TIME_STATS]) > > +AC_ARG_ENABLE([grub-emu-sdl2], > + [AS_HELP_STRING([--enable-grub-emu-sdl2], > + [build and install the `grub-emu' debugging > utility with SDL2 support (default=guessed)])]) > + > AC_ARG_ENABLE([grub-emu-sdl], > [AS_HELP_STRING([--enable-grub-emu-sdl], > [build and install the `grub-emu' debugging > utility with SDL support (default=guessed)])]) > @@ -1572,6 +1576,28 @@ AC_ARG_ENABLE([grub-emu-pci], > [build and install the `grub-emu' debugging > utility with PCI support (potentially dangerous) (default=no)])]) > > if test "$platform" = emu; then > + if test x"$enable_grub_emu_sdl2" = xno ; then > + grub_emu_sdl2_excuse="explicitly disabled" > + fi > + [if [ x"$grub_emu_sdl2_excuse" = x ]; then > + # Check for libSDL libraries.] > + PKG_CHECK_MODULES([SDL2], [sdl2], [ > + AC_DEFINE([HAVE_SDL2], [1], [Define to 1 if you have SDL2 > library.]) > + AC_SUBST(HAVE_SDL2)], > + [grub_emu_sdl2_excuse="libSDL2 libraries are required to build > \`grub-emu' with SDL2 support"]) Something is wrong with indention. > + [fi] > + if test x"enable_grub_emu_sdl2" = xyes && test x"$grub_emu_sdl2_excuse" != > x ; then > + AC_MSG_ERROR([SDL2 support for grub-emu was explicitly requested but can't > be compiled ($grub_emu_sdl2_excuse)]) Ditto. > + fi > + if test x"$grub_emu_sdl2_excuse" = x ; then > + enable_grub_emu_sdl2=yes > + else > + enable_grub_emu_sdl2=no > + fi > + if test x"$enable_grub_emu_sdl2" = xyes ; then > + grub_emu_sdl_excuse="disabled by sdl2" > + fi > + > > if test x"$enable_grub_emu_sdl" = xno ; then > grub_emu_sdl_excuse="explicitly disabled" > @@ -1620,12 +1646,14 @@ AC_CHECK_LIB([SDL], [SDL_Init], [LIBSDL="-lSDL"], > enable_grub_emu_pci=no > fi > > + AC_SUBST([enable_grub_emu_sdl2]) > AC_SUBST([enable_grub_emu_sdl]) > AC_SUBST([enable_grub_emu_pci]) > > else > > # Ignore --enable-emu-* if platform is not emu > + enable_grub_emu_sdl2=no > enable_grub_emu_sdl=no > enable_grub_emu_pci=no > fi > @@ -2052,6 +2080,7 @@ AM_CONDITIONAL([COND_HOST_XNU], [test x$host_kernel = > xxnu]) > AM_CONDITIONAL([COND_HOST_ILLUMOS], [test x$host_kernel = xillumos]) > > AM_CONDITIONAL([COND_MAN_PAGES], [test x$cross_compiling = xno -a x$HELP2MAN > != x]) > +AM_CONDITIONAL([COND_GRUB_EMU_SDL2], [test x$enable_grub_emu_sdl2 = xyes]) > AM_CONDITIONAL([COND_GRUB_EMU_SDL], [test x$enable_grub_emu_sdl = xyes]) > AM_CONDITIONAL([COND_GRUB_EMU_PCI], [test x$enable_grub_emu_pci = xyes]) > AM_CONDITIONAL([COND_GRUB_MKFONT], [test x$enable_grub_mkfont = xyes]) > @@ -2130,6 +2159,11 @@ echo > "*******************************************************" > echo GRUB2 will be compiled with following components: > echo Platform: "$target_cpu"-"$platform" > if [ x"$platform" = xemu ]; then > +if [ x"$grub_emu_sdl2_excuse" = x ]; then > +echo SDL2 support for grub-emu: Yes > +else > +echo SDL2 support for grub-emu: No "($grub_emu_sdl2_excuse)" > +fi > if [ x"$grub_emu_sdl_excuse" = x ]; then > echo SDL support for grub-emu: Yes > else > diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am > index d32f2b662..f0cb2f2cc 100644 > --- a/grub-core/Makefile.am > +++ b/grub-core/Makefile.am > @@ -317,6 +317,9 @@ KERNEL_HEADER_FILES += > $(top_srcdir)/include/grub/emu/exec.h > if COND_GRUB_EMU_SDL > KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h > endif > +if COND_GRUB_EMU_SDL2 > +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h > +endif > if COND_GRUB_EMU_PCI > KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/libpciaccess.h > endif > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def > index e458aa665..d2cf29584 100644 > --- a/grub-core/Makefile.core.def > +++ b/grub-core/Makefile.core.def > @@ -418,7 +418,7 @@ program = { > > ldadd = 'kernel.exec$(EXEEXT)'; > ldadd = '$(MODULE_FILES)'; > - ldadd = 'lib/gnulib/libgnu.a $(LIBINTL) $(LIBUTIL) $(LIBSDL) $(LIBUSB) > $(LIBPCIACCESS) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)'; > + ldadd = 'lib/gnulib/libgnu.a $(LIBINTL) $(LIBUTIL) $(LIBSDL) $(SDL2_LIBS) > $(LIBUSB) $(LIBPCIACCESS) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)'; s/SDL2_LIBS/LIBSDL2/? If possible stick to the naming convention here. > enable = emu; > }; > @@ -430,7 +430,7 @@ program = { > emu_nodist = symlist.c; > > ldadd = 'kernel.exec$(EXEEXT)'; > - ldadd = 'lib/gnulib/libgnu.a $(LIBINTL) $(LIBUTIL) $(LIBSDL) $(LIBUSB) > $(LIBPCIACCESS) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)'; > + ldadd = 'lib/gnulib/libgnu.a $(LIBINTL) $(LIBUTIL) $(LIBSDL) $(SDL2_LIBS) > $(LIBUSB) $(LIBPCIACCESS) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)'; Ditto. > enable = emu; > }; > @@ -2325,6 +2325,14 @@ module = { > condition = COND_GRUB_EMU_SDL; > }; > > +module = { > + name = sdl; > + emu = video/emu/sdl.c; > + enable = emu; > + condition = COND_GRUB_EMU_SDL2; > + cflags = '$(SDL2_CFLAGS)'; > +}; > + > module = { > name = datehook; > common = hook/datehook.c; > diff --git a/grub-core/video/emu/sdl.c b/grub-core/video/emu/sdl.c > index 0ebab6f57..93fb83da0 100644 > --- a/grub-core/video/emu/sdl.c > +++ b/grub-core/video/emu/sdl.c > @@ -18,6 +18,9 @@ > > #define grub_video_render_target grub_video_fbrender_target > > +#include <config-util.h> > +#include <config.h> > + > #include <grub/err.h> > #include <grub/types.h> > #include <grub/dl.h> > @@ -25,11 +28,23 @@ > #include <grub/mm.h> > #include <grub/video.h> > #include <grub/video_fb.h> > +#ifdef HAVE_SDL2 > +#include <SDL2/SDL.h> > +#else > #include <SDL/SDL.h> > +#endif > > GRUB_MOD_LICENSE ("GPLv3+"); > > +#ifdef HAVE_SDL2 > +static SDL_Window *window = 0; > +static SDL_Surface *surface = 0; > +static SDL_Texture *texture = 0; > +static SDL_Renderer *renderer = 0; > +#else > static SDL_Surface *window = 0; > +static SDL_Surface *surface = 0; > +#endif Please use NULL instead of 0... ... and you can move surface definition out of ifdefery. > static struct grub_video_render_target *sdl_render_target; > static struct grub_video_mode_info mode_info; > > @@ -91,6 +106,34 @@ grub_video_sdl_setup (unsigned int width, unsigned int > height, > height = 600; > } > > +#ifdef HAVE_SDL2 > + (void) mode_mask; // We can't specify this in SDL2 If you mark this argument as __attribute__ ((unused)) then you can drop this thing. > + window = SDL_CreateWindow ("grub-emu", > + SDL_WINDOWPOS_UNDEFINED, > + SDL_WINDOWPOS_UNDEFINED, > + width, height, flags); > + if (! window) "if (window == NULL)" please. Here and below... > + return grub_error (GRUB_ERR_BAD_DEVICE, "Couldn't open window: %s", > + SDL_GetError ()); All error messages should start with lowercase, e.g. s/Couldn't/couldn't/. > + renderer = SDL_CreateRenderer (window, -1, 0); > + if (! renderer) > + return grub_error (GRUB_ERR_BAD_DEVICE, "Couldn't open renderer: %s", > + SDL_GetError ()); Ditto and below please... > + texture = SDL_CreateTexture (renderer, > + SDL_PIXELFORMAT_ARGB8888, > + SDL_TEXTUREACCESS_STREAMING, > + width, height); > + if (! texture) > + return grub_error (GRUB_ERR_BAD_DEVICE, "Couldn't create texture: %s", > + SDL_GetError ()); > + > + // An empty surface that acts as the pixel buffer, the texture will > receive the pixels > + // from here. Please stick to comments conventions [1]. > + surface = SDL_CreateRGBSurface (0, width, height, depth, 0, 0, 0, 0); > + if (! surface) > + return grub_error (GRUB_ERR_BAD_DEVICE, "Couldn't open surface: %s", > + SDL_GetError ()); > +#else Daniel [1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
