Re: [PATCH v1a] grub-emu: Add SDL2 support

[Daniel Kiper]

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.klode@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