[Piglit] [PATCH] util: implement eventloop for wayland platform

Fri Oct 28 09:59:48 UTC 2016

On Tue, 25 Oct 2016 14:23:12 +0300
Tapani Pälli <tapani.palli at intel.com> wrote:

> On 10/25/2016 02:09 PM, Pekka Paalanen wrote:
> > On Mon, 24 Oct 2016 14:31:25 +0300
> > Tapani Pälli <tapani.palli at intel.com> wrote:
> >  
> >> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> >> ---
> >>  .../util/piglit-framework-gl/piglit_wl_framework.c | 137 +++++++++++++++++++--
> >>  1 file changed, 129 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tests/util/piglit-framework-gl/piglit_wl_framework.c b/tests/util/piglit-framework-gl/piglit_wl_framework.c
> >> index 78ffea6..6b832f6 100644
> >> --- a/tests/util/piglit-framework-gl/piglit_wl_framework.c
> >> +++ b/tests/util/piglit-framework-gl/piglit_wl_framework.c
> >> @@ -24,14 +24,136 @@
> >>  #include <assert.h>
> >>  #include <stdlib.h>
> >>  #include <unistd.h>
> >> +#include <poll.h>
> >>
> >>  #include "piglit-util-gl.h"
> >>  #include "piglit_wl_framework.h"
> >>
> >> +#include <wayland-client.h>
> >> +#include <waffle_wayland.h>
> >> +#include <linux/input-event-codes.h>
> >> +
> >> +static struct wl_seat *seat = NULL;
> >> +static struct wl_keyboard *keyboard = NULL;
> >> +
> >> +static void keymap(void *data,
> >> +	struct wl_keyboard *wl_keyboard,
> >> +	uint32_t format,
> >> +	int32_t fd,
> >> +	uint32_t size)
> >> +{

Oh, I forgot to say: you must the very least close the fd to not leak
it.

> >> +}
> >> +
> >> +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_winsys_framework *winsys_fw =
> >> +		(struct piglit_winsys_framework *) data;
> >> +
> >> +	winsys_fw->need_redisplay = true;
> >> +
> >> +	switch (key) {
> >> +	case KEY_ESC:
> >> +                if (winsys_fw->user_keyboard_func)
> >> +			winsys_fw->user_keyboard_func(27, 0, 0);  
> >
> > Hi,
> >
> > do you want this to trigger on both key down and key up?
> > If not, check 'state'.  
> 
> Yeah, I saw that there but wanted to just make the most simplest 
> implementation (at least for now). This could be improved to map event 
> codes to what user_keyboard_func eats and always just send that mapped 
> value, not just with KEY_ESC.
> 
> > I suppose all keyboards emit KEY_ESC when the user presses the key
> > labeled "esc" so you don't need to handle keymaps?  
> 
> Any suggestions here welcome, I did not find any examples of how keymap 
> handler (empty above) is supposed to work so ... what I did was this :)

Hi,

a proper handler pulls in a dependency to libxkbcommon.

Some sort of example is in
https://cgit.freedesktop.org/wayland/weston/tree/clients/window.c

keyboard_handle_keymap()
keyboard_handle_key()
keyboard_handle_modifiers()

Those are probably the most important ones.

Getting a leave event means that keyboard input was taken away; if keys
were down, they are no longer applicable, but it does not mean you got
key-up events. Similarly you can get a wl_keyboard enter event with keys
already down, but it does not mean you got a bunch of key-down events
(seems like the example ignores keys already down). What you do in each
case depends on the app.

Key repeat must be implemented in the client if wanted.

> 
> >> +		break;
> >> +	}
> >> +}
> >> +
> >> +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)
> >> +{
> >> +}
> >> +
> >> +static const struct wl_keyboard_listener keyboard_listener = {
> >> +	keymap,
> >> +	enter,
> >> +	leave,
> >> +	key,
> >> +	modifiers
> >> +};
> >> +
> >> +static void
> >> +process_events(struct wl_display *dpy)
> >> +{
> >> +	struct pollfd fds = {
> >> +		.fd = wl_display_get_fd(dpy),
> >> +		.events = POLLIN,
> >> +		.revents = 0
> >> +	};
> >> +
> >> +	wl_display_sync(dpy);  
> >
> > What did you intend to do here?
> >
> > wl_display_sync() sends the wl_display.sync request and creates a
> > wl_callback object which you leak.  
> 
> The intention here was to trigger flush for the event queue so that I 
> would receive those queued events.

For flush there is wl_display_flush():
https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1a8463b6e5f4cf9a2a3ad2d543aedcf429

Simply sending a request does not guarantee a flush.

Also see the doc for wl_display_dispatch():
https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1a30a9c4f020f3e77581c7a81ecdb4913d

> >> +
> >> +	while (1) {
> >> +		poll(&fds, 1, -1);
> >> +		if (wl_display_dispatch(dpy) < 0)
> >> +			break;
> >> +	}
> >> +}
> >> +
> >> +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);  
> >
> > There can be more than one wl_seat advertised, so if you only want
> > one (how do you pick which one?)...  
> 
> Heh .. same explanation as for the keymap handler! Is there somewhere a 
> 'wayland seats for dummies'?

A good question...

Ideally one would bind to all wl_seat objects, and handle every one of
them independently. It's like multi-pointer (multiple independent
pointer cursors), except you can have multiple independent keyboards
etc. all possibly typing into the same window at the same time.

Mind, that getting such a configuration requires more from the user
than just plugging in an additional keyboard. I believe compositors
will usually just add new keyboards to the existing wl_seat by default,
so that multiple physical keyboards drive the same wl_seat
(wl_keyboard). Udev configuration is needed to create multiple
wl_seats, and it's not a common configuration.

> >> +	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;
> >> +	const struct piglit_gl_test_config *test_config =
> >> +		winsys_fw->wfl_fw.gl_fw.test_config;
> >> +	union waffle_native_window *n_window =
> >> +		waffle_window_get_native(winsys_fw->wfl_fw.window);
> >> +	struct wl_display *dpy = n_window->wayland->display.wl_display;
> >> +	struct wl_registry *registry = wl_display_get_registry(dpy);  
> >
> > How do you intend to destroy the registry object created here, or
> > do you just willfully leak it?  
> 
> We leak a lot of things with Piglit but I can fix this. Can I make a 
> simple callback to free it with some Wayland API?

No callbacks, it is app's decision to destroy it - just store the
pointer in some struct, and call wl_registry_destroy() when you no
longer need it. But do it *before* destroying the wl_display.

wl_display_get_registry() returns a new instance every time it is
called, even if all instances semantically refer to "the same
wl_registry".

> Thanks for taking a look at this, this is my first time with Wayland so 
> please bare with me!

It's a fine start! :-)

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20161028/cbc7bc32/attachment-0001.sig>