[RFC PATCH libinput] udev: add libinput_udev_rescan_devices()
Peter Hutterer
peter.hutterer at who-t.net
Thu Mar 20 22:14:53 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?
fwiw, this is effectively what happens internally anyway, you get removed
events for every device on suspend, and added events on resume for those
still there.
> 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?
libinput_suspend/resume only tear down the devices, but not anything
else. there isn't much global state that's kept across suspend/resume yet
(seats are one example though) but the biggest difference is that that you
can't use _any_ object around after libinput_destroy(). suspend/resume
keeps them alive until you unref them.
Cheers,
Peter
>
>
> 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