Re: [PATCH xf86-input-libinput] Always delay hotplugging subdevices

2016-08-18 Thread Keith Packard
Peter Hutterer  writes:

> Avoid creating new devices from within the input thread which was the case for
> tablet tools. It requires a lot more care about locking and has a potential to
> mess up things.
>
> Instead, schedule a WorkProc and buffer all events until we have the device
> created. Once that's done, replay the event sequence so far. If the device
> comes into proximity and out again before we manage to create the new device
> we just ditch the whole sequence and wait for the next proximity in.
>
> Signed-off-by: Peter Hutterer 
> ---
>  src/xf86libinput.c | 259 
> +
>  1 file changed, 202 insertions(+), 57 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 1ecbc41..9b9ba12 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -102,6 +102,12 @@ struct xf86libinput_device {
>   struct xorg_list unclaimed_tablet_tool_list;
>  };
>  
> +struct xf86libinput_tablet_tool_event_queue {
> + bool need_to_queue;
> + struct libinput_event_tablet_tool *events[128];
> + size_t nevents;

Any particular reason to use an array of fixed size here, instead of a
list which wouldn't have any limit?

> -static void
> +static inline void

Yeah, just drop the 'inline' for the huge functions in this file; the
compiler will probably ignore it anyways...


> +xf86libinput_tool_replay_events(struct xf86libinput_tablet_tool_event_queue 
> *queue)
> +{
> + size_t i;
> +
> + for (i = 0; i < queue->nevents; i++) {
> + struct libinput_event *event;
> + event = 
> libinput_event_tablet_tool_get_base_event(queue->events[i]);
> + xf86libinput_handle_event(event);
> + queue->events[i] = NULL;
> + /* we're not queueing anymore so we expect handle_event to
> +libinput_event_destroy() */

looks like handle_event just returns whether to destroy the event, so
you'd need a call here, or make handle_event handle the event
destruction internally.

The rest of this looks reasonable to me.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xf86-input-libinput] Always delay hotplugging subdevices

2016-08-17 Thread Peter Hutterer
Avoid creating new devices from within the input thread which was the case for
tablet tools. It requires a lot more care about locking and has a potential to
mess up things.

Instead, schedule a WorkProc and buffer all events until we have the device
created. Once that's done, replay the event sequence so far. If the device
comes into proximity and out again before we manage to create the new device
we just ditch the whole sequence and wait for the next proximity in.

Signed-off-by: Peter Hutterer 
---
 src/xf86libinput.c | 259 +
 1 file changed, 202 insertions(+), 57 deletions(-)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 1ecbc41..9b9ba12 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -102,6 +102,12 @@ struct xf86libinput_device {
struct xorg_list unclaimed_tablet_tool_list;
 };
 
+struct xf86libinput_tablet_tool_event_queue {
+   bool need_to_queue;
+   struct libinput_event_tablet_tool *events[128];
+   size_t nevents;
+};
+
 struct xf86libinput_tablet_tool {
struct xorg_list node;
struct libinput_tablet_tool *tool;
@@ -162,20 +168,22 @@ struct xf86libinput {
bool allow_mode_group_updates;
 };
 
-enum hotplug_when {
-   HOTPLUG_LATER,
-   HOTPLUG_NOW,
+enum event_cleanup {
+   EVENT_KEEP,
+   EVENT_DESTROY,
 };
 
-static DeviceIntPtr
+static void
 xf86libinput_create_subdevice(InputInfoPtr pInfo,
  uint32_t capabilities,
- enum hotplug_when,
  XF86OptionPtr extra_opts);
 static inline void
 update_mode_prop(InputInfoPtr pInfo,
 struct libinput_event_tablet_pad *event);
 
+static enum event_cleanup
+xf86libinput_handle_event(struct libinput_event *event);
+
 static inline int
 use_server_fd(const InputInfoPtr pInfo) {
return pInfo->fd > -1 && (pInfo->flags & XI86_SERVER_FD);
@@ -1386,28 +1394,110 @@ xf86libinput_pick_device(struct xf86libinput_device 
*shared_device,
return NULL;
 }
 
-static void
+static inline void
+xf86libinput_tool_replay_events(struct xf86libinput_tablet_tool_event_queue 
*queue)
+{
+   size_t i;
+
+   for (i = 0; i < queue->nevents; i++) {
+   struct libinput_event *event;
+   event = 
libinput_event_tablet_tool_get_base_event(queue->events[i]);
+   xf86libinput_handle_event(event);
+   queue->events[i] = NULL;
+   /* we're not queueing anymore so we expect handle_event to
+  libinput_event_destroy() */
+   }
+   queue->nevents = 0;
+}
+
+/* Return true if we queued, false otherwise */
+static inline bool
+xf86libinput_tool_queue_event(struct libinput_event_tablet_tool *event)
+{
+   struct libinput_event *e;
+   struct libinput_tablet_tool *tool;
+   struct xf86libinput_tablet_tool_event_queue *queue;
+
+   tool = libinput_event_tablet_tool_get_tool(event);
+   if (!tool)
+   return true;
+
+   queue = libinput_tablet_tool_get_user_data(tool);
+   if (!queue)
+   return false;
+
+   if (!queue->need_to_queue) {
+   if (queue->nevents > 0) {
+   libinput_tablet_tool_set_user_data(tool, NULL);
+   xf86libinput_tool_replay_events(queue);
+   free(queue);
+   }
+
+   return false;
+   }
+
+   /* We got the prox out while still queuing, just ditch the whole
+* series of events and the event queue with it. */
+   if (libinput_event_tablet_tool_get_proximity_state(event) ==
+   LIBINPUT_TABLET_TOOL_PROXIMITY_STATE_OUT) {
+   int i;
+
+   for (i = 0; i < queue->nevents; i++) {
+   e = 
libinput_event_tablet_tool_get_base_event(queue->events[i]);
+   libinput_event_destroy(e);
+   }
+   libinput_tablet_tool_set_user_data(tool, NULL);
+   free(queue);
+
+   /* we destroy the event here but return true
+* to make sure the event looks like it got queued and the
+* caller doesn't destroy it for us
+*/
+   e = libinput_event_tablet_tool_get_base_event(event);
+   libinput_event_destroy(e);
+   return true;
+   }
+
+   if (queue->nevents == ARRAY_SIZE(queue->events)) {
+   e = libinput_event_tablet_tool_get_base_event(event);
+   libinput_event_destroy(e);
+   return true;
+   }
+
+   queue->events[queue->nevents++] = event;
+   return true;
+}
+
+static enum event_cleanup
 xf86libinput_handle_tablet_tip(InputInfoPtr pInfo,
   struct libinput_event_tablet_tool *event)
 {
enum libinput_tablet_tool_tip_state state;
const BOOL is_absolute = TRUE;
 
+   if (xf86libinput_tool_queue_even