Re: [Qemu-devel] [PATCH] ui/sdl2 : initial port to SDL 2.0

[Anthony Liguori]

On Nov 11, 2013 1:10 AM, "Dave Airlie" <address@hidden> wrote:
>
> On Mon, Nov 11, 2013 at 2:02 PM, Anthony Liguori <address@hidden> wrote:
> > On Sun, Nov 10, 2013 at 3:15 PM, Dave Airlie <address@hidden> wrote:
> >> From: Dave Airlie <address@hidden>
> >>
> >> I've ported the SDL1.2 code over, and rewritten it to use the SDL2 interface.
> >>
> >> The biggest changes were in the input handling, where SDL2 has done a major
> >> overhaul, and I've had to include a generated translation file to get from
> >> SDL2 codes back to qemu compatible ones. I'm still not sure how the keyboard
> >> layout code works in qemu, so there may be further work if someone can point
> >> me a test case that works with SDL1.2 and doesn't with SDL2.
> >>
> >> Some SDL env vars we used to set are no longer used by SDL2,
> >> Windows, OSX support is untested,
> >>
> >> I don't think we can link to SDL1.2 and SDL2 at the same time, so I felt
> >> using --with-sdlabi=2.0 to select the new code should be fine, like how
> >> gtk does it.
> >>
> >> Signed-off-by: Dave Airlie <address@hidden>
> >> ---
> >>  configure                    |  23 +-
> >>  ui/Makefile.objs             |   4 +-
> >>  ui/sdl.c                     |   3 +
> >>  ui/sdl2.c                    | 889 +++++++++++++++++++++++++++++++++++++++++++
> >
> > Can we refactor this to not duplicate everything and instead have
> > function hooks or even #ifdefs for the things that are different?  We
> > try to guess the right SDL to use in configure.  See how we handle
> > GTK2 vs. GTK3.
> >
> > It's very hard to review ATM due to the split.
>
> No I talked to enough people at kvmforum and everyone said I should
> split this into a separate file, please don't make me undo that now, I
> originally did it with ifdefs and just spent a few days redoing it the
> other way!

Perhaps whoever you spoke with should speal up then.  Forking sdl.c seems like a pretty bad idea to me.

>
> >
> > Regarding the keycodes, danpb has a great write up on his blog:
> >
> > https://www.berrange.com/posts/2010/07/04/a-summary-of-scan-code-key-codes-sets-used-in-the-pc-virtualization-stack/
>
> Okay I'll read that later,
>
> >
> > Internally, we use a variant of raw XT scancodes.  We have a
> > keymapping routine that translates from a symbolic key (i.e. "capital
> > A") to the appropriate XT scancode.
> >
> > SDL 1.x at least lets you get at raw X11 scancodes which are either
> > evdev or PS/2 codes depending on the version of X11.  So for SDL 1.x
> > we have two translations mechanisms 1) X11 scancodes to XT scancodes
> > and 2) SDL keysyms to internal QEMU keysyms.
> >
> > From what I can tell, SDL2 has moved from just passing through raw X11
> > scancodes (which like I said, can be different depending on your X
> > configuration) to having an intermediate translation layer.  See
> > comments inline:
> >
> >>  ui/sdl2_scancode_translate.h | 260 +++++++++++++
> >>  ui/sdl_keysym.h              |   3 +-
> >>  6 files changed, 1175 insertions(+), 7 deletions(-)
> >>  create mode 100644 ui/sdl2.c
> >>  create mode 100644 ui/sdl2_scancode_translate.h
> >>
> >> diff --git a/configure b/configure
> >> index 9addff1..bf3be37 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -158,6 +158,7 @@ docs=""
> >>  fdt=""
> >>  pixman=""
> >>  sdl=""
> >> +sdlabi="1.2"
> >>  virtfs=""
> >>  vnc="yes"
> >>  sparse="no"
> >> @@ -310,6 +311,7 @@ query_pkg_config() {
> >>  }
> >>  pkg_config=query_pkg_config
> >>  sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
> >> +sdl2_config="${SDL2_CONFIG-${cross_prefix}sdl2-config}"
> >>
> >>  # default flags for all hosts
> >>  QEMU_CFLAGS="-fno-strict-aliasing $QEMU_CFLAGS"
> >> @@ -710,6 +712,8 @@ for opt do
> >>    ;;
> >>    --enable-sdl) sdl="yes"
> >>    ;;
> >> +  --with-sdlabi=*) sdlabi="$optarg"
> >> +  ;;
> >>    --disable-qom-cast-debug) qom_cast_debug="no"
> >>    ;;
> >>    --enable-qom-cast-debug) qom_cast_debug="yes"
> >> @@ -1092,6 +1096,7 @@ echo "  --disable-strip          disable stripping binaries"
> >>  echo "  --disable-werror         disable compilation abort on warning"
> >>  echo "  --disable-sdl            disable SDL"
> >>  echo "  --enable-sdl             enable SDL"
> >> +echo "  --with-sdlabi            select preferred SDL ABI 1.2 or 2.0"
> >>  echo "  --disable-gtk            disable gtk UI"
> >>  echo "  --enable-gtk             enable gtk UI"
> >>  echo "  --disable-virtfs         disable VirtFS"
> >> @@ -1751,12 +1756,22 @@ fi
> >>
> >>  # Look for sdl configuration program (pkg-config or sdl-config).  Try
> >>  # sdl-config even without cross prefix, and favour pkg-config over sdl-config.
> >> -if test "`basename $sdl_config`" != sdl-config && ! has ${sdl_config}; then
> >> -  sdl_config=sdl-config
> >> +
> >> +if test $sdlabi == "2.0"; then
> >> +    sdl_config=$sdl2_config
> >> +    sdlname=sdl2
> >> +    sdlconfigname=sdl2_config
> >> +else
> >> +    sdlname=sdl
> >> +    sdlconfigname=sdl_config
> >> +fi
> >> +
> >> +if test "`basename $sdl_config`" != $sdlconfigname && ! has ${sdl_config}; then
> >> +  sdl_config=$sdlconfigname
> >>  fi
> >>
> >> -if $pkg_config sdl --exists; then
> >> -  sdlconfig="$pkg_config sdl"
> >> +if $pkg_config $sdlname --exists; then
> >> +  sdlconfig="$pkg_config $sdlname"
> >>    _sdlversion=`$sdlconfig --modversion 2>/dev/null | sed 's/[^0-9]//g'`
> >>  elif has ${sdl_config}; then
> >>    sdlconfig="$sdl_config"
> >> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> >> index f33be47..721ad37 100644
> >> --- a/ui/Makefile.objs
> >> +++ b/ui/Makefile.objs
> >> @@ -9,12 +9,12 @@ vnc-obj-y += vnc-jobs.o
> >>
> >>  common-obj-y += keymaps.o console.o cursor.o input.o qemu-pixman.o
> >>  common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
> >> -common-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o
> >> +common-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o sdl2.o
> >>  common-obj-$(CONFIG_COCOA) += cocoa.o
> >>  common-obj-$(CONFIG_CURSES) += curses.o
> >>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
> >>  common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
> >>
> >> -$(obj)/sdl.o $(obj)/sdl_zoom.o: QEMU_CFLAGS += $(SDL_CFLAGS)
> >> +$(obj)/sdl.o $(obj)/sdl_zoom.o $(obj)/sdl2.o: QEMU_CFLAGS += $(SDL_CFLAGS)
> >>
> >>  $(obj)/gtk.o: QEMU_CFLAGS += $(GTK_CFLAGS) $(VTE_CFLAGS)
> >> diff --git a/ui/sdl.c b/ui/sdl.c
> >> index 9d8583c..736bb95 100644
> >> --- a/ui/sdl.c
> >> +++ b/ui/sdl.c
> >> @@ -26,6 +26,8 @@
> >>  #undef WIN32_LEAN_AND_MEAN
> >>
> >>  #include <SDL.h>
> >> +
> >> +#if SDL_MAJOR_VERSION == 1
> >>  #include <SDL_syswm.h>
> >>
> >>  #include "qemu-common.h"
> >> @@ -966,3 +968,4 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> >>
> >>      atexit(sdl_cleanup);
> >>  }
> >> +#endif
> >> diff --git a/ui/sdl2.c b/ui/sdl2.c
> >> new file mode 100644
> >> index 0000000..1a71f59
> >> --- /dev/null
> >> +++ b/ui/sdl2.c
> >> @@ -0,0 +1,889 @@
> >> +/*
> >> + * QEMU SDL display driver
> >> + *
> >> + * Copyright (c) 2003 Fabrice Bellard
> >> + *
> >> + * 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.
> >> + */
> >> +/* Ported SDL 1.2 code to 2.0 by Dave Airlie. */
> >> +
> >> +/* Avoid compiler warning because macro is redefined in SDL_syswm.h. */
> >> +#undef WIN32_LEAN_AND_MEAN
> >> +
> >> +#include <SDL.h>
> >> +
> >> +#if SDL_MAJOR_VERSION == 2
> >> +#include <SDL_syswm.h>
> >> +
> >> +#include "qemu-common.h"
> >> +#include "ui/console.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "x_keymap.h"
> >> +#include "sdl_zoom.h"
> >> +
> >> +#include "sdl2_scancode_translate.h"
> >> +
> >> +static DisplayChangeListener *dcl;
> >> +static DisplaySurface *surface;
> >> +static SDL_Window *real_window;
> >> +static SDL_Renderer *real_renderer;
> >> +static SDL_Texture *guest_texture;
> >> +
> >> +static int gui_grab; /* if true, all keyboard/mouse events are grabbed */
> >> +static int last_vm_running;
> >> +static bool gui_saved_scaling;
> >> +static int gui_saved_width;
> >> +static int gui_saved_height;
> >> +static int gui_saved_grab;
> >> +static int gui_fullscreen;
> >> +static int gui_noframe;
> >> +static int gui_key_modifier_pressed;
> >> +static int gui_keysym;
> >> +static int gui_grab_code = KMOD_LALT | KMOD_LCTRL;
> >> +static uint8_t modifiers_state[256];
> >> +static SDL_Cursor *sdl_cursor_normal;
> >> +static SDL_Cursor *sdl_cursor_hidden;
> >> +static int absolute_enabled = 0;
> >> +static int guest_cursor = 0;
> >> +static int guest_x, guest_y;
> >> +static SDL_Cursor *guest_sprite = NULL;
> >> +static int scaling_active = 0;
> >> +static Notifier mouse_mode_notifier;
> >> +
> >> +static void sdl_update_caption(void);
> >> +
> >> +static void sdl_update(DisplayChangeListener *dcl,
> >> +                       int x, int y, int w, int h)
> >> +{
> >> +    if (!surface)
> >> +        return;
> >> +
> >> +    SDL_UpdateTexture(guest_texture, NULL, surface_data(surface),
> >> +                      surface_stride(surface));
> >> +    SDL_RenderCopy(real_renderer, guest_texture, NULL, NULL);
> >> +    SDL_RenderPresent(real_renderer);
> >> +}
> >> +
> >> +static void do_sdl_resize(int width, int height, int bpp)
> >> +{
> >> +    int flags;
> >> +
> >> +    if (real_window) {
> >> +        SDL_SetWindowSize(real_window, width, height);
> >> +        SDL_RenderSetLogicalSize(real_renderer, width, height);
> >> +    } else {
> >> +        flags = 0;
> >> +        if (gui_fullscreen)
> >> +            flags |= SDL_WINDOW_FULLSCREEN;
> >> +        else
> >> +            flags |= SDL_WINDOW_RESIZABLE;
> >> +
> >> +        real_window = SDL_CreateWindow("", SDL_WINDOWPOS_UNDEFINED,
> >> +                                       SDL_WINDOWPOS_UNDEFINED,
> >> +                                       width, height, flags);
> >> +        real_renderer = SDL_CreateRenderer(real_window, -1, 0);
> >> +        sdl_update_caption();
> >> +    }
> >> +}
> >> +
> >> +static void sdl_switch(DisplayChangeListener *dcl,
> >> +                       DisplaySurface *new_surface)
> >> +{
> >> +    int format = 0;
> >> +    /* temporary hack: allows to call sdl_switch to handle scaling changes */
> >> +    if (new_surface) {
> >> +        surface = new_surface;
> >> +    }
> >> +
> >> +    if (!scaling_active) {
> >> +        do_sdl_resize(surface_width(surface), surface_height(surface), 0);
> >> +    } else
> >> +        do_sdl_resize(surface_width(surface), surface_height(surface), 0);
> >> +
> >> +    if (guest_texture)
> >> +        SDL_DestroyTexture(guest_texture);
> >> +
> >> +    if (surface_bits_per_pixel(surface) == 16)
> >> +        format = SDL_PIXELFORMAT_RGB565;
> >> +    else if (surface_bits_per_pixel(surface) == 32)
> >> +        format = SDL_PIXELFORMAT_ARGB8888;
> >> +
> >> +    if (!format)
> >> +        exit(1);
> >> +    guest_texture = SDL_CreateTexture(real_renderer, format,
> >> +                                      SDL_TEXTUREACCESS_STREAMING,
> >> +                                      surface_width(surface), surface_height(surface));
> >> +    SDL_RenderSetLogicalSize(real_renderer, surface_width(surface), surface_height(surface));
> >> +}
> >> +
> >> +/* generic keyboard conversion */
> >> +
> >> +#include "sdl_keysym.h"
> >> +
> >> +static kbd_layout_t *kbd_layout = NULL;
> >> +
> >> +static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev)
> >> +{
> >> +    int keysym;
> >> +    /* workaround for X11+SDL bug with AltGR */
> >> +    keysym = ev->keysym.sym;
> >> +    if (keysym == 0 && ev->keysym.scancode == 113)
> >> +        keysym = SDLK_MODE;
> >> +    /* For Japanese key '\' and '|' */
> >> +    if (keysym == 92 && ev->keysym.scancode == 133) {
> >> +        keysym = 0xa5;
> >> +    }
> >> +    return keysym2scancode(kbd_layout, keysym) & SCANCODE_KEYMASK;
> >> +}
> >> +
> >> +/* specific keyboard conversions from scan codes */
> >> +
> >> +#if defined(_WIN32)
> >> +
> >> +static uint8_t sdl_keyevent_to_keycode(const SDL_KeyboardEvent *ev)
> >> +{
> >> +    int keycode;
> >> +
> >> +    keycode = ev->keysym.scancode;
> >> +    keycode = sdl2_scancode_to_keycode[keycode];
> >> +    return keycode;
> >> +}
> >> +
> >> +#else
> >> +
> >> +static uint8_t sdl_keyevent_to_keycode(const SDL_KeyboardEvent *ev)
> >> +{
> >> +    int keycode;
> >> +
> >> +    keycode = ev->keysym.scancode;
> >> +
> >> +    keycode = sdl2_scancode_to_keycode[keycode];
> >> +    if (keycode >= 89 && keycode < 150) {
> >> +        keycode = translate_evdev_keycode(keycode - 89);
> >> +    }
> >
> > This doesn't seem right to me.  It just so happened that the low value
> > scan codes are the same for both X11, evdev, and SDL 1.x.  That's why
> > we only used the evdev/x11 translation tables for higher codes.  It
> > was just a table size optimization.
> >
> > Presumably, sdl2 doesn't do this though, or does it?  If it does,
> > don't we need to probe for evdev instead of assuming it's there?
>
> The table I wrote translates to evdev codes, then we use the evdev
> translator to translate to qemu ones, so its perfectly fine as is,
> SDL2 hides everything in its own table.

Why not translate to XT directly?  The intermediate evdev step seems unnecessary.

Regards,

Anthony Liguori

>
> Dave.