[PATCH xf86-input-libinput] Handle capability events after adding a device

[Peter Hutterer]

On 3/02/2015 18:29 , Hans de Goede wrote:
> Hi,
> 
> On 30-01-15 06:31, Peter Hutterer wrote:
>> Needs a temporary libinput context to get all capability events without
>> events from other devices interfering.
>>
>> This doesn't yet handle true capability changes, only the initial
>> burst of
>> events after the DEVICE_ADDED event.
>>
>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> 
> Hmm, this one does not give me a warm/fuzzy feeling. Are we sure that
> having
> the capabilities of libinput events on the fly is the best way to go ?
> 
> It is the logical thing to do from a libinput pov, but it seems to put an
> unnecessary burden on libinput users, we're likely going to see similar
> problems in compositors. I would prefer to just solve this in libinput,
> e.g. :
> 
> 1) Add a heuristic to see if a device may have multiple evdev nodes
> representing
> a single device.
> 2) If 1) is true, then wait for evdev nodes to show up for 0.5 seconds
> (or maybe
> we can even know which evdev nodes to wait for) and do not give the
> device to
> the libinput user until it is complete.
> 
> This is more work in libinput, but it seems much easier on libinput
> users, and
> thus the right thing to do.

we (Benjamin and I) did consider this at first but the big drawback is
that we then have to keep a model database of devices in libinput.
Heuristics are hard and even harder when you have to guess whether a
tablet is a Intuos 5 or an Intuos 5 Touch - without a model DB you don't
know. And the database is a big drawback: yet another layer in the stack
that needs updating for new devices.

The timeout approach could work, but was pretty much dismissed
immediately, mainly for the usual reasons: not sure how long to wait,
you can't know if/when the second device will show up, etc.

Pushing this to the caller where the device is handled semantically
seemed like the most reliable solution, even if it is more complex to
deal with.

Cheers,
  Peter


> 
>> ---
>> That's based on a patchset scheduled for libinput 0.10
>>
>>   src/libinput.c | 68
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 61 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libinput.c b/src/libinput.c
>> index a24cbff..29a624c 100644
>> --- a/src/libinput.c
>> +++ b/src/libinput.c
>> @@ -51,6 +51,8 @@
>>   #define TOUCH_MAX_SLOTS 15
>>   #define XORG_KEYCODE_OFFSET 8
>>
>> +#define AS_MASK(v) (1 << v)
>> +
>>   /*
>>      libinput does not provide axis information for absolute devices,
>> instead
>>      it scales into the screen dimensions provided. So we set up the
>> axes with
>> @@ -70,6 +72,7 @@ static struct xf86libinput_driver driver_context;
>>   struct xf86libinput {
>>       char *path;
>>       struct libinput_device *device;
>> +    unsigned int capabilities;
>>
>>       int scroll_vdist;
>>       int scroll_hdist;
>> @@ -600,15 +603,15 @@ xf86libinput_init(DeviceIntPtr dev)
>>
>>       dev->public.on = FALSE;
>>
>> -    if (libinput_device_has_capability(device,
>> LIBINPUT_DEVICE_CAP_KEYBOARD))
>> +    if (driver_data->capabilities &
>> AS_MASK(LIBINPUT_DEVICE_CAP_KEYBOARD))
>>           xf86libinput_init_keyboard(pInfo);
>> -    if (libinput_device_has_capability(device,
>> LIBINPUT_DEVICE_CAP_POINTER)) {
>> +    if (driver_data->capabilities &
>> AS_MASK(LIBINPUT_DEVICE_CAP_POINTER)) {
>>           if (libinput_device_config_calibration_has_matrix(device))
>>               xf86libinput_init_pointer_absolute(pInfo);
>>           else
>>               xf86libinput_init_pointer(pInfo);
>>       }
>> -    if (libinput_device_has_capability(device,
>> LIBINPUT_DEVICE_CAP_TOUCH))
>> +    if (driver_data->capabilities & AS_MASK(LIBINPUT_DEVICE_CAP_TOUCH))
>>           xf86libinput_init_touch(pInfo);
>>
>>       /* unref the device now, because we'll get a new ref during
>> @@ -815,6 +818,8 @@ xf86libinput_handle_event(struct libinput_event
>> *event)
>>           case LIBINPUT_EVENT_NONE:
>>           case LIBINPUT_EVENT_DEVICE_ADDED:
>>           case LIBINPUT_EVENT_DEVICE_REMOVED:
>> +        case LIBINPUT_EVENT_DEVICE_CAPABILITY_ADDED:
>> +        case LIBINPUT_EVENT_DEVICE_CAPABILITY_REMOVED:
>>               break;
>>           case LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE:
>>               xf86libinput_handle_absmotion(pInfo,
>> @@ -1117,6 +1122,49 @@ xf86libinput_parse_options(InputInfoPtr pInfo,
>>
>>   }
>>
>> +/* libinput sends capabilities as events after opening the device. In
>> X we
>> +   need the capabilities to be done when we get to DEVICE_INIT.
>> +   The capability events may be intermixed with real events from other
>> +   devices, making handling device init a lot more complicated than
>> it needs
>> +   to be. To avoid this, we create a temporary libinput context, pull
>> the
>> +   capability events off the context, then ignore them in the main
>> context.
>> + */
>> +static int
>> +get_capabilities(const char *path)
>> +{
>> +    int rc = -1;
>> +    struct libinput *libinput = NULL;
>> +    struct libinput_device *device;
>> +    struct libinput_event *event;
>> +    struct libinput_event_device_capability *cap;
>> +
>> +    libinput = libinput_path_create_context(&interface,
>> +                        &driver_context);
>> +    if (!libinput)
>> +        goto out;
>> +
>> +
>> +    device = libinput_path_add_device(libinput, path);
>> +    if (!device)
>> +        goto out;
>> +
>> +    libinput_dispatch(libinput);
>> +
>> +    rc = 0;
>> +    while ((event = libinput_get_event(libinput))) {
>> +        if (libinput_event_get_type(event) ==
>> LIBINPUT_EVENT_DEVICE_CAPABILITY_ADDED) {
>> +            cap = libinput_event_get_device_capability_event(event);
>> +            rc |=
>> AS_MASK(libinput_event_device_capability_get_capability(cap));
>> +        }
>> +        libinput_event_destroy(event);
>> +    }
>> +
>> +out:
>> +    libinput_unref(libinput);
>> +
>> +    return rc;
>> +}
>> +
>>   static int
>>   xf86libinput_pre_init(InputDriverPtr drv,
>>                 InputInfoPtr pInfo,
>> @@ -1125,6 +1173,7 @@ xf86libinput_pre_init(InputDriverPtr drv,
>>       struct xf86libinput *driver_data = NULL;
>>           struct libinput *libinput = NULL;
>>       struct libinput_device *device;
>> +    int capabilities;
>>       char *path;
>>
>>       pInfo->type_name = 0;
>> @@ -1170,6 +1219,12 @@ xf86libinput_pre_init(InputDriverPtr drv,
>>       if (use_server_fd(pInfo))
>>           fd_push(&driver_context, pInfo->fd, path);
>>
>> +    capabilities = get_capabilities(path);
>> +    if (capabilities == -1) {
>> +        xf86IDrvMsg(pInfo, X_ERROR, "Failed to create a device for
>> %s\n", path);
>> +        goto fail;
>> +    }
>> +
>>       device = libinput_path_add_device(libinput, path);
>>       if (!device) {
>>           xf86IDrvMsg(pInfo, X_ERROR, "Failed to create a device for
>> %s\n", path);
>> @@ -1187,6 +1242,7 @@ xf86libinput_pre_init(InputDriverPtr drv,
>>       pInfo->private = driver_data;
>>       driver_data->path = path;
>>       driver_data->device = device;
>> +    driver_data->capabilities = (unsigned int)capabilities;
>>
>>       /* Disable acceleration in the server, libinput does it for us */
>>       pInfo->options = xf86ReplaceIntOption(pInfo->options,
>> "AccelerationProfile", -1);
>> @@ -1197,11 +1253,9 @@ xf86libinput_pre_init(InputDriverPtr drv,
>>       /* now pick an actual type */
>>       if (libinput_device_config_tap_get_finger_count(device) > 0)
>>           pInfo->type_name = XI_TOUCHPAD;
>> -    else if (libinput_device_has_capability(device,
>> -                        LIBINPUT_DEVICE_CAP_TOUCH))
>> +    else if (capabilities & AS_MASK(LIBINPUT_DEVICE_CAP_TOUCH))
>>           pInfo->type_name = XI_TOUCHSCREEN;
>> -    else if (libinput_device_has_capability(device,
>> -                        LIBINPUT_DEVICE_CAP_POINTER))
>> +    else if (capabilities & AS_MASK(LIBINPUT_DEVICE_CAP_POINTER))
>>           pInfo->type_name = XI_MOUSE;
>>       else
>>           pInfo->type_name = XI_KEYBOARD;
>>