[RFC PATCH libinput] udev: add libinput_udev_rescan_devices()
Jonas Ådahl
jadahl at gmail.com
Fri Mar 21 01:04:55 PDT 2014
On Fri, Mar 21, 2014 at 12:27:45AM -0400, Jasper St. Pierre wrote:
> So you're saying that every time we're suspended, we simply throw out the
> entire context and drop all the devices on the floor, as if you just
> unplugged all of them?
No, what I meant was possible was:
1. start
2. "resume" devices
3. create libinput context
...
4. suspend libinput context
5. "pause" devices
...
6. "resume" devices
7. resume libinput context
And I mean this instead of doing
1. start
2. create libinput context
3. "resume" devices
4. rescan for devices
...
5. suspend libinput context
6. "pause" devices
...
7. "resume" devices
8. resume libinput context
9. rescan for devices
Jonas
>
> I suppose I just never thought of that. On first though, I don't see
> anything wrong with that.
>
> If that's what we should do, should we remove libinput_suspend /
> libinput_resume then?
>
>
> On Mon, Mar 17, 2014 at 11:21 PM, Peter Hutterer
> <peter.hutterer at who-t.net>wrote:
>
> > On Sat, Mar 15, 2014 at 07:59:29PM +0100, Jonas Ådahl wrote:
> > > On Thu, Mar 13, 2014 at 04:18:20PM +1000, Peter Hutterer wrote:
> > > > When a libinput context for a given seat is initialized, not all
> > devices may
> > > > be available. Some or all devices may be paused by systemd-logind.
> > Waiting for
> > > > a unplug event is not appropriate here as the devices are physically
> > > > available, just prevented from getting access.
> > > >
> > > > Add a libinput_udev_rescan_devices() function that triggers a scan of
> > all
> > > > devices on the current udev seat. Devices that do not exist on the
> > seat are
> > > > added, devices that already exist on the seat but have been revoked are
> > > > removed.
> > > >
> > > > Note that devices that have been physically removed are not removed,
> > instead
> > > > we wait for the udev event to handle this for us.
> > > >
> > > > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > > > ---
> > > > The idea here is basically to start a udev context as usual. If the
> > > > compositor doesn't have the session, open_restricted will fail. Once
> > the
> > > > ResumeDevice signals are handled by the compositor it can ask libinput
> > to
> > > > rescan the device list to add the ones that failed before (or remove
> > revoked
> > > > ones).
> > > >
> > > > Notes on why this approach:
> > > > * libinput_device_suspend()/resume() seems nicer at first but if a
> > device
> > > > fails to open, we don't have a libinput_device context. handling that
> > > > would make the API complicated since we cannot guarantee that all
> > > > libinput_device_* functions (specificall has_capability) work on all
> > > > devices anymore.
> > > > * I suspect in the 90% case the the PauseDevice/ResumeDevice signals
> > come in
> > > > in a batch anyway, so the compositor should be able to optimise this
> > to
> > > > one single call
> > > > * this is a udev specific call, for the path backend the compositor
> > can and
> > > > should maintain the devices manually anyway
> > > > * EVIOCGVERSION was picked because it always succeeds, except after
> > revoke
> > > >
> > > > This is an RFC at this point, let me know if that approach works.
> > Still need
> > > > to write tests and better evdev duplicate detection - right now there
> > is a
> > > > race condition that could remove the wrong device.
> > >
> > > Hi,
> > >
> > > So what this patch is trying to solve is handling the following flow:
> > >
> > > * create libinput udev context
> > > - some or all devices will fail to open due to being paused
> > > * devices are resumed
> > >
> > > What stops us from simply doing
> > >
> > > * devices are resumed
> > > * create libinput udev context
> >
> > Jasper? you can answer that better than me
> >
> > > As you say, a compositor should be able to know when it should rescan,
> > > and in most cases (?) before this, we won't get a single device anyway
> > > so whats the point of creating earlier than that? For resuming after
> > > session switch I suppose we'd have the same problem, but this would then
> > > just work the same:
> > >
> > > * devices are resumed
> > > * resume libinput context
> >
> > the question here is: is there a use-case for a single device to be
> > paused/resumed outside of the usual process? David?
> >
> > We're struggling with this in X but that's caused by a completely different
> > problem and is rather orthogonal to this.
> >
> > Cheers,
> > Peter
> >
> >
> > > > src/evdev.c | 15 +++++++++++++++
> > > > src/evdev.h | 2 ++
> > > > src/libinput.h | 21 +++++++++++++++++++++
> > > > src/udev-seat.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
> > > > 4 files changed, 79 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/evdev.c b/src/evdev.c
> > > > index ba7c8b3..018fbb1 100644
> > > > --- a/src/evdev.c
> > > > +++ b/src/evdev.c
> > > > @@ -790,3 +790,18 @@ evdev_device_destroy(struct evdev_device *device)
> > > > free(device->sysname);
> > > > free(device);
> > > > }
> > > > +
> > > > +int
> > > > +evdev_device_is_alive(struct evdev_device *device)
> > > > +{
> > > > + int rc;
> > > > + int version;
> > > > +
> > > > + rc = ioctl(device->fd, EVIOCGVERSION, &version);
> > > > +
> > > > + if (rc < 0 && errno != ENODEV)
> > > > + log_info("evdev: %s failed with errno %d (%s)\n",
> > > > + __func__, errno, strerror(errno));
> > > > +
> > > > + return rc != -1;
> > > > +}
> > > > diff --git a/src/evdev.h b/src/evdev.h
> > > > index b83a2f9..82a3873 100644
> > > > --- a/src/evdev.h
> > > > +++ b/src/evdev.h
> > > > @@ -156,4 +156,6 @@ evdev_device_remove(struct evdev_device *device);
> > > > void
> > > > evdev_device_destroy(struct evdev_device *device);
> > > >
> > > > +int
> > > > +evdev_device_is_alive(struct evdev_device *device);
> > > > #endif /* EVDEV_H */
> > > > diff --git a/src/libinput.h b/src/libinput.h
> > > > index 3e09871..dadcac2 100644
> > > > --- a/src/libinput.h
> > > > +++ b/src/libinput.h
> > > > @@ -715,6 +715,27 @@ libinput_udev_create_for_seat(const struct
> > libinput_interface *interface,
> > > > /**
> > > > * @ingroup base
> > > > *
> > > > + * Re-scan the list of devices available to this context. Devices in
> > the
> > > > + * seat specified in libinput_udev_create_for_seat() that previously
> > have
> > > > + * failed to initialize are re-initialized. Devices that have
> > successfully
> > > > + * re-initialized but are now revoked are removed.
> > > > + *
> > > > + * Calling libinput_udev_rescan_devices() on a context suspended with
> > > > + * libinput_suspend() does nothing.
> > > > + *
> > > > + * @note This function should not be used for detection of physically
> > added
> > > > + * or removed devices, libinput_dispatch() detects those. This
> > function
> > > > + * should only be used to re-open or close existing devices, e.g. if
> > > > + * systemd-logind prevented access to a device before.
> > > > + *
> > > > + * @param libinput The previously initialized libinput context
> > > > + */
> > > > +void
> > > > +libinput_udev_rescan_devices(struct libinput *libinput);
> > > > +
> > > > +/**
> > > > + * @ingroup base
> > > > + *
> > > > * Create a new libinput context that requires the caller to manually
> > add or
> > > > * remove devices with libinput_path_add_device() and
> > > > * libinput_path_remove_device().
> > > > diff --git a/src/udev-seat.c b/src/udev-seat.c
> > > > index 366c92b..5b09e4b 100644
> > > > --- a/src/udev-seat.c
> > > > +++ b/src/udev-seat.c
> > > > @@ -136,12 +136,28 @@ device_removed(struct udev_device *udev_device,
> > struct udev_input *input)
> > > > }
> > > > }
> > > >
> > > > +static struct evdev_device*
> > > > +udev_input_find_device_by_sysname(struct udev_input *input, const
> > char *sysname)
> > > > +{
> > > > + struct udev_seat *seat;
> > > > + struct evdev_device *device;
> > > > +
> > > > + list_for_each(seat, &input->base.seat_list, base.link) {
> > > > + list_for_each(device, &seat->base.devices_list, base.link)
> > > > + if (!strcmp(device->sysname, sysname)) {
> > > > + return device;
> > > > + }
> > > > + }
> > > > + return NULL;
> > > > +}
> > > > +
> > > > static int
> > > > udev_input_add_devices(struct udev_input *input, struct udev *udev)
> > > > {
> > > > struct udev_enumerate *e;
> > > > struct udev_list_entry *entry;
> > > > struct udev_device *device;
> > > > + struct evdev_device *evdev;
> > > > const char *path, *sysname;
> > > >
> > > > e = udev_enumerate_new(udev);
> > > > @@ -157,11 +173,15 @@ udev_input_add_devices(struct udev_input *input,
> > struct udev *udev)
> > > > continue;
> > > > }
> > > >
> > > > - if (device_added(device, input) < 0) {
> > > > - udev_device_unref(device);
> > > > - udev_enumerate_unref(e);
> > > > - return -1;
> > > > - }
> > > > + evdev = udev_input_find_device_by_sysname(input, sysname);
> > > > + if (!evdev) {
> > > > + if (device_added(device, input) < 0) {
> > > > + udev_device_unref(device);
> > > > + udev_enumerate_unref(e);
> > > > + return -1;
> > > > + }
> > > > + } else if (!evdev_device_is_alive(evdev))
> > > > + device_removed(device, input);
> > > >
> > > > udev_device_unref(device);
> > > > }
> > > > @@ -364,3 +384,19 @@ libinput_udev_create_for_seat(const struct
> > libinput_interface *interface,
> > > >
> > > > return &input->base;
> > > > }
> > > > +
> > > > +LIBINPUT_EXPORT void
> > > > +libinput_udev_rescan_devices(struct libinput *libinput)
> > > > +{
> > > > + struct udev_input *udev_input = (struct udev_input*)libinput;
> > > > +
> > > > + if (libinput->interface_backend != &interface_backend) {
> > > > + log_error("Mismatching backends. This is an application
> > bug.\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + if (udev_input->udev_monitor == NULL)
> > > > + return;
> > > > +
> > > > + udev_input_add_devices(udev_input, udev_input->udev);
> > > > +}
> > > > --
> > > > 1.8.5.3
> > > >
> > > > _______________________________________________
> > > > wayland-devel mailing list
> > > > wayland-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
>
>
>
> --
> Jasper
More information about the wayland-devel
mailing list