[PATCH weston 2/3] input: Add support for making libxkbcommon optional

[Kristian Høgsberg]

On Sun, Jun 16, 2013 at 01:29:36PM +0300, Pekka Paalanen wrote:
> On Fri, 14 Jun 2013 17:42:08 +0100
> Rob Bradford <robert.bradford at intel.com> wrote:
> 
> > From: Matt Roper <matthew.d.roper at intel.com>
> > 
> > In embedded environments, devices that appear as evdev "keyboards" often
> > have no resemblence to PC-style keyboards.  It is not uncommon for such
> > environments to have no concept of modifier keys and no need for XKB key
> > mapping; in these cases libxkbcommon initialization becomes unnecessary
> > startup overhead.  On some SOC platforms, xkb keymap compilation can
> > account for as much as 1/3 - 1/2 of the total compositor startup time.
> > 
> > This patch introduces a 'use_xkbcommon' flag in the core compositor
> > structure that indicates whether the compositor is running in "raw
> > keyboard" mode.  In raw keyboard mode, the compositor bypasses all
> > libxkbcommon initialization and processing.  'key' events containing the
> > integer keycode will continue to be delivered via the wl_keyboard
> > interface, but no 'keymap' event will be sent to clients.  No modifier
> > handling or keysym mapping is performed in this mode.
> > 
> > Note that upstream sample apps (e.g., weston-terminal or the
> > desktop-shell client) will not recognize raw keycodes and will not react
> > to keypresses when the compositor is operating in raw keyboard mode.
> > This is expected behavior; key events are still being sent to the
> > client, the client (and/or its toolkit) just isn't written to handle
> > keypresses without doing xkb keysym mapping.  Applications written
> > specifically for such embedded environments would be handling keypresses
> > via the raw keycode delivered as part of the 'key' event rather than
> > using xkb keysym mapping.
> > 
> > Whether to use xkbcommon is a global option that applies to all
> > compositor keyboard devices on the system; it is an all-or-nothing flag.
> > This patch simply adds conditional checks on whether xkbcommon is to be
> > used or not.
> > 
> > v2 by Rob Bradford <rob at linux.intel.com>: the original version of the
> > patch used a "raw_keycodes" flag instead of the "use_xkbcommon" used in
> > this patch.
> > 
> > v1: Reviewed-by: Singh, Satyeshwar <satyeshwar.singh at intel.com>
> > v1: Reviewed-by: Bob Paauwe <bob.j.paauwe at intel.com>
> 
> Hi,
> 
> sending raw keycodes sounds crazy, given e.g. the problems we will
> have with gamepads elsewhere, but I'll leave that for others to
> criticise, since I'm not too familiar with keyboard input.

No, it's not crazy.  Compiling a xkb keymap when you never intend to
lookup a keysym for a keycode is crazy.  There are plenty of use cases
where you have a device with a specific, custom "keyboard", (buttons
on a remote or a car dash board), and can look at just the keycode.

> Did you investigate writing a small, custom xkb keymap for your
> device?
> 
> > ---
> >  src/compositor.c |  1 +
> >  src/compositor.h |  3 +++
> >  src/input.c      | 71 +++++++++++++++++++++++++++++++++++++++-----------------
> >  3 files changed, 54 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 42011f5..fc407dd 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -2811,6 +2811,7 @@ weston_compositor_init(struct weston_compositor *ec,
> >  	weston_plane_init(&ec->primary_plane, 0, 0);
> >  	weston_compositor_stack_plane(ec, &ec->primary_plane, NULL);
> >  
> > +	ec->use_xkbcommon = 1;
> >  	s = weston_config_get_section(ec->config, "keyboard", NULL, NULL);
> >  	weston_config_section_get_string(s, "keymap_rules",
> >  					 (char **) &xkb_names.rules, NULL);
> > diff --git a/src/compositor.h b/src/compositor.h
> > index c87d9f1..b80388e 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -558,6 +558,9 @@ struct weston_compositor {
> >  	struct xkb_rule_names xkb_names;
> >  	struct xkb_context *xkb_context;
> >  	struct weston_xkb_info xkb_info;
> > +
> > +	/* Raw keyboard processing (no libxkbcommon initialization or handling) */
> > +	int use_xkbcommon;
> >  };
> >  
> >  struct weston_buffer_reference {
> > diff --git a/src/input.c b/src/input.c
> > index 5463d78..8ffbd10 100644
> > --- a/src/input.c
> > +++ b/src/input.c
> > @@ -815,6 +815,10 @@ update_modifier_state(struct weston_seat *seat, uint32_t serial, uint32_t key,
> >  {
> >  	enum xkb_key_direction direction;
> >  
> > +	/* Keyboard modifiers don't exist in raw keyboard mode */
> > +	if (!seat->compositor->use_xkbcommon)
> > +		return;
> > +
> >  	if (state == WL_KEYBOARD_KEY_STATE_PRESSED)
> >  		direction = XKB_KEY_DOWN;
> >  	else
> > @@ -1199,9 +1203,15 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
> >  	wl_list_insert(&seat->keyboard->resource_list, &cr->link);
> >  	cr->destroy = unbind_resource;
> >  
> > -	wl_keyboard_send_keymap(cr, WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1,
> > -				seat->xkb_info.keymap_fd,
> > -				seat->xkb_info.keymap_size);
> > +	if (seat->compositor->use_xkbcommon) {
> > +		wl_keyboard_send_keymap(cr, WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1,
> > +					seat->xkb_info.keymap_fd,
> > +					seat->xkb_info.keymap_size);
> > +	} else {
> > +		wl_keyboard_send_keymap(cr, WL_KEYBOARD_KEYMAP_FORMAT_NO_KEYMAP,
> > +					0,
> > +					0);
> 
> 0 is a legal fd, and probably even an open one. -1 should be
> guaranteed an invalid fd, and is usually silently ignored. Can you
> send -1 instead?
> 
> If you send 0, you will probably clone the stdin file descriptor of
> the server, and send it to client. Did you check that clients will
> close the fd regardless of the format or length?

We can't send 0, of course, but we can't send -1 either.  sendmsg
won't send -1 as an fd and we can't receive it either.  I think the
best approach is to open /dev/null and send that fd.

Kristian