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

[Dave Airlie]

On Tue, Nov 12, 2013 at 12:07 AM, Anthony Liguori <address@hidden> wrote:
>
> 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.

It does right now, but since I can't add multi-head support to SDL1.2
it'll rapidly start diverging in order to add features that SDL1.2
can't support.

btw here is my old sdl.c implementation its pretty ugly, later commit
in that branch have the input bits.

http://cgit.freedesktop.org/~airlied/qemu/commit/?h=virtio-gpu&id=ee44399a3dbce8da810329230f0a439a3b88cd67

As I said I suspect this will just get uglier as time goes on, as we
add SDL2 only features.
>
>>
>> >
>> > 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,

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

Correctness and future maintainability,
there exists a table in SDL2 source code going from evdev->SDL2,
I wrote trivial code to reverse the contents of that table so I know
its correct, if it changes in future I can just do the same simple
reversal and import it. Though there is probably nothing stopping me
adding the extra step at that stage I wasn't sure it was a good idea
going forward.

Dave.