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

[Dave Airlie]

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!

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

Dave.