[Piglit] [PATCH v2 2/2] util: implement eventloop for wayland platform

[Tapani Pälli]

On 11/15/2016 10:29 AM, Pekka Paalanen wrote:
> On Wed,  2 Nov 2016 13:16:27 +0200
> Tapani Pälli <tapani.palli at intel.com> wrote:
>
>> v2: lots of fixes based on review from Pekka Paalanen
>>
>>     - fix leak of fd in keymap handler
>>     - fix leak of wayland registry
>>     - just call wl_display_dispatch in process_events
>>     - act when key pressed (matches x11 backend behavior)
>>     - just pass any keys, now tests that do special stuff
>>       with keys work (like fbo-clear-formats)
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>  .../util/piglit-framework-gl/piglit_wl_framework.c | 253 +++++++++++++++++++--
>>  1 file changed, 237 insertions(+), 16 deletions(-)
>
> Moi Tapani,
>
> some comments inline.
>
>> diff --git a/tests/util/piglit-framework-gl/piglit_wl_framework.c b/tests/util/piglit-framework-gl/piglit_wl_framework.c
>> index 78ffea6..8bf2afb 100644
>> --- a/tests/util/piglit-framework-gl/piglit_wl_framework.c
>> +++ b/tests/util/piglit-framework-gl/piglit_wl_framework.c
>> @@ -1,5 +1,7 @@
>>  /*
>>   * Copyright © 2012 Intel Corporation
>> + * Copyright © 2008 Kristian Høgsberg
>> + * Copyright © 2012-2013 Collabora, Ltd.
>>   *
>>   * Permission is hereby granted, free of charge, to any person obtaining a
>>   * copy of this software and associated documentation files (the "Software"),
>> @@ -28,10 +30,208 @@
>>  #include "piglit-util-gl.h"
>>  #include "piglit_wl_framework.h"
>>
>> +#include <wayland-client.h>
>> +#include <waffle_wayland.h>
>> +#include <xkbcommon/xkbcommon.h>
>> +#include <sys/mman.h>
>> +
>> +struct piglit_wl_framework {
>> +        struct piglit_winsys_framework winsys_fw;
>> +
>> +	struct wl_display *dpy;
>> +	struct wl_registry *registry;
>> +
>> +	struct {
>> +		struct xkb_context *context;
>> +		struct xkb_keymap *keymap;
>> +		struct xkb_state *state;
>> +	} xkb;
>> +};
>> +
>> +static struct wl_seat *seat = NULL;
>> +static struct wl_keyboard *keyboard = NULL;
>> +
>> +/* Most of the code here taken from window.c in Weston source code. */
>> +static void keymap(void *data,
>> +	struct wl_keyboard *wl_keyboard,
>> +	uint32_t format,
>> +	int32_t fd,
>> +	uint32_t size)
>> +{
>> +	struct piglit_wl_framework *wl_fw =
>> +		(struct piglit_wl_framework *) data;
>> +	struct xkb_keymap *keymap;
>> +	struct xkb_state *state;
>> +	char *locale;
>> +	char *map_str;
>> +
>> +	if (!data || format != WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1) {
>> +		close(fd);
>> +		return;
>> +	}
>> +
>> +	map_str = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
>> +	if (map_str == MAP_FAILED) {
>> +		close(fd);
>> +		return;
>> +	}
>> +
>> +	/* Set up XKB keymap */
>> +	keymap = xkb_keymap_new_from_string(wl_fw->xkb.context,
>> +		map_str,
>> +		XKB_KEYMAP_FORMAT_TEXT_V1,
>> +		0);
>> +
>> +	if (!keymap) {
>> +		fprintf(stderr, "failed to compile keymap\n");
>
> Leaks fd. Move munmap and close above this and be happy!

fixed

>> +		return;
>> +	}
>> +
>> +	munmap(map_str, size);
>> +	close(fd);
>> +
>> +	/* Set up XKB state */
>> +	state = xkb_state_new(keymap);
>> +	if (!state) {
>> +		fprintf(stderr, "failed to create XKB state\n");
>> +		xkb_keymap_unref(keymap);
>> +		return;
>> +	}
>> +
>> +	/* Look up the preferred locale, falling back to "C" as default */
>> +	if (!(locale = getenv("LC_ALL")))
>> +		if (!(locale = getenv("LC_CTYPE")))
>> +			if (!(locale = getenv("LANG")))
>> +				locale = "C";
>> +
>> +	xkb_keymap_unref(wl_fw->xkb.keymap);
>> +	xkb_state_unref(wl_fw->xkb.state);
>> +	wl_fw->xkb.keymap = keymap;
>> +	wl_fw->xkb.state = state;
>> +}
>> +
>> +static void enter(void *data,
>> +	struct wl_keyboard *wl_keyboard,
>> +	uint32_t serial,
>> +	struct wl_surface *surface,
>> +	struct wl_array *keys)
>> +{
>> +}
>> +
>> +static void leave(void *data,
>> +	struct wl_keyboard *wl_keyboard,
>> +	uint32_t serial,
>> +	struct wl_surface *surface)
>> +{
>> +}
>> +
>> +static void key(void *data,
>> +	struct wl_keyboard *wl_keyboard,
>> +	uint32_t serial,
>> +	uint32_t time,
>> +	uint32_t key,
>> +	uint32_t state)
>> +{
>> +	struct piglit_wl_framework *wl_fw =
>> +		(struct piglit_wl_framework *) data;
>> +	struct piglit_winsys_framework *winsys_fw = &wl_fw->winsys_fw;
>> +	const struct piglit_gl_test_config *test_config =
>> +		winsys_fw->wfl_fw.gl_fw.test_config;
>> +
>> +	const xkb_keysym_t *syms;
>> +	xkb_keysym_t sym;
>> +
>> +	uint32_t code = key + 8;
>> +	uint32_t num_syms;
>> +
>> +	if (!wl_fw->xkb.state)
>> +		return;
>> +
>> +	num_syms = xkb_state_key_get_syms(wl_fw->xkb.state, code, &syms);
>> +	sym = XKB_KEY_NoSymbol;
>> +	if (num_syms == 1)
>> +		sym = syms[0];
>> +
>> +	winsys_fw->need_redisplay = true;
>> +
>> +	if (state != WL_KEYBOARD_KEY_STATE_PRESSED)
>> +		return;
>> +
>> +	if (winsys_fw->user_keyboard_func)
>> +		winsys_fw->user_keyboard_func(sym, 0, 0);
>> +
>> +	if (winsys_fw->need_redisplay) {
>> +		enum piglit_result result = PIGLIT_PASS;
>> +		if (test_config->display)
>> +			result = test_config->display();
>> +		if (piglit_automatic)
>> +			piglit_report_result(result);
>> +		winsys_fw->need_redisplay = false;
>> +	}
>> +}
>> +
>> +static void modifiers(void *data,
>> +	struct wl_keyboard *wl_keyboard,
>> +	uint32_t serial,
>> +	uint32_t mods_depressed,
>> +	uint32_t mods_latched,
>> +	uint32_t mods_locked,
>> +	uint32_t group)
>> +{
>
> I suspect leaving this empty would make you miss... modifiers' effects?
> Up to you. :-)

current Piglit keyboard usage is very simple so I did not want to 
implement 'extra', when/if there will be modifier usage then this can be 
filled in

>> +}
>> +
>> +static const struct wl_keyboard_listener keyboard_listener = {
>> +	keymap,
>> +	enter,
>> +	leave,
>> +	key,
>> +	modifiers
>> +};
>> +
>> +static void
>> +process_events(struct wl_display *dpy)
>> +{
>> +	while (1) {
>> +		if (wl_display_dispatch(dpy) == -1)
>> +			break;
>
> How do you break out from this gracefully? Is there some piglit infra
> e.g. reacting to some key and calling exit() or something? Just
> wondering how to quit this program.

Piglit calls exit(0) when escape key gets pressed and key handler calls 
user_keyboard_func.

>> +	}
>> +}
>> +
>> +static void global(void *data,
>> +	struct wl_registry *wl_registry,
>> +	uint32_t name,
>> +	const char *interface,
>> +	uint32_t version)
>> +{
>> +	if (strcmp(interface, "wl_seat") != 0)
>> +		return;
>> +
>> +	seat = wl_registry_bind(wl_registry, name, &wl_seat_interface, 1);
>> +	keyboard = wl_seat_get_keyboard(seat);
>> +
>> +	if (keyboard)
>> +		wl_keyboard_add_listener(keyboard, &keyboard_listener, data);
>> +}
>> +
>> +static void global_remove(void *data,
>> +	struct wl_registry *wl_registry,
>> +	uint32_t name)
>> +{
>> +}
>> +
>> +static const struct wl_registry_listener registry_listener = {
>> +	global,
>> +	global_remove
>> +};
>> +
>>  static void
>>  enter_event_loop(struct piglit_winsys_framework *winsys_fw)
>>  {
>> -	const struct piglit_gl_test_config *test_config = winsys_fw->wfl_fw.gl_fw.test_config;
>> +	struct piglit_wl_framework *wl_fw =
>> +		(struct piglit_wl_framework *) winsys_fw;
>> +	const struct piglit_gl_test_config *test_config =
>> +		winsys_fw->wfl_fw.gl_fw.test_config;
>> +	enum piglit_result result = PIGLIT_PASS;
>>
>>  	/* The Wayland window fails to appear on the first call to
>>  	 * swapBuffers (which occured in display_cb above). This is
>> @@ -40,14 +240,13 @@ enter_event_loop(struct piglit_winsys_framework *winsys_fw)
>>  	 * redraw as a workaround.
>>  	 */
>>  	if (test_config->display)
>> -		test_config->display();
>> +		result = test_config->display();
>>
>> -	/* FINISHME: Write event loop for Wayland.
>> -	 *
>> -	 * Until we have proper Wayland support, give the user enough time
>> -	 * to view the window by sleeping.
>> -	 */
>> -	sleep(8);
>> +	/* Do not proceed to event loop in case of piglit_automatic. */
>> +	if (piglit_automatic)
>> +		piglit_report_result(result);
>> +
>> +	process_events(wl_fw->dpy);
>>  }
>>
>>  static void
>> @@ -59,30 +258,52 @@ show_window(struct piglit_winsys_framework *winsys_fw)
>>  static void
>>  destroy(struct piglit_gl_framework *gl_fw)
>>  {
>> -	struct piglit_winsys_framework *winsys_fw= piglit_winsys_framework(gl_fw);
>> +	struct piglit_wl_framework *wl_fw =
>> +		(struct piglit_wl_framework *) gl_fw;
>>
>> -	if (winsys_fw == NULL)
>> +	if (wl_fw == NULL)
>>  		return;
>>
>> -	piglit_winsys_framework_teardown(winsys_fw);
>> -	free(winsys_fw);
>> +	xkb_context_unref(wl_fw->xkb.context);
>
> Does this also free xkb.keymap and xkb.state? I'd kind of expect to see
> them freed here, but I'm not sure.

fixed

>> +
>> +	wl_registry_destroy(wl_fw->registry);
>> +
>> +	piglit_winsys_framework_teardown(&wl_fw->winsys_fw);
>> +	free(wl_fw);
>>  }
>>
>>  struct piglit_gl_framework*
>>  piglit_wl_framework_create(const struct piglit_gl_test_config *test_config)
>>  {
>> +	struct piglit_wl_framework *wl_fw = NULL;
>>  	struct piglit_winsys_framework *winsys_fw = NULL;
>>  	struct piglit_gl_framework *gl_fw = NULL;
>>  	bool ok = true;
>>
>> -	winsys_fw = calloc(1, sizeof(*winsys_fw));
>> -	gl_fw = &winsys_fw->wfl_fw.gl_fw;
>> +	wl_fw = calloc(1, sizeof(*wl_fw));
>> +	winsys_fw = &wl_fw->winsys_fw;
>> +	gl_fw = &wl_fw->winsys_fw.wfl_fw.gl_fw;
>>
>> -	ok = piglit_winsys_framework_init(winsys_fw, test_config,
>> -	                           WAFFLE_PLATFORM_WAYLAND);
>> +        ok = piglit_winsys_framework_init(&wl_fw->winsys_fw,
>> +                                          test_config, WAFFLE_PLATFORM_WAYLAND);
>>  	if (!ok)
>>  		goto fail;
>>
>> +	wl_fw->xkb.context = xkb_context_new(0);
>> +	wl_fw->xkb.keymap = NULL;
>> +	wl_fw->xkb.state = NULL;
>> +
>> +	if (!wl_fw->xkb.context)
>> +		goto fail;
>> +
>> +	union waffle_native_window *n_window =
>> +		waffle_window_get_native(winsys_fw->wfl_fw.window);
>> +
>> +	wl_fw->dpy = n_window->wayland->display.wl_display;
>> +	wl_fw->registry = wl_display_get_registry(wl_fw->dpy);
>> +
>> +	wl_registry_add_listener(wl_fw->registry, &registry_listener, wl_fw);
>> +
>>  	winsys_fw->show_window = show_window;
>>  	winsys_fw->enter_event_loop = enter_event_loop;
>>  	gl_fw->destroy = destroy;
>
> It all looks pretty good to me, just the couple freeing issues, and few
> questions. I didn't compare this to what we have in Weston repository
> nor do I know anything about Piglit's framework, but with that
> disclaimer, I have nothing else to complain about. :-)
>
> Not sure that really counts as R-b...

Thanks for taking a look!

If you want to still check/test I've pushed a v3 here with fixes to 
issues you found:

https://cgit.freedesktop.org/~tpalli/piglit/log/?h=wayland

If nobody complains, I'll push this in after some days.

// Tapani