[systemd-devel] [PATCH] udev: input_id: tag accelerometers as ID_INPUT_ACCELEROMETER

[Benjamin Tissoires]

On Apr 03 2015 or thereabouts, David Herrmann wrote:
> Hi
> 
> On Fri, Apr 3, 2015 at 5:30 PM, Benjamin Tissoires <btissoir at redhat.com> wrote:
> > On Apr 03 2015 or thereabouts, David Herrmann wrote:
> >> Hi
> >>
> >> On Fri, Apr 3, 2015 at 4:38 PM, Benjamin Tissoires <btissoir at redhat.com> wrote:
> >> > On Apr 03 2015 or thereabouts, Hans de Goede wrote:
> >> >> Hi,
> >> >>
> >> >> On 03-04-15 15:51, David Herrmann wrote:
> >> >> >Hi
> >> >> >
> >> >> >On Fri, Apr 3, 2015 at 12:07 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> >> >> >>input_id already (tries to) tag accelerometers as such, but this only works
> >> >> >>for absolute accelerometers. Recent kernels mark accelerometers through an
> >> >> >>input prop. Trust that prop and always tag devices with it with
> >> >> >>ID_INPUT_ACCELEROMETER.
> >> >> >>
> >> >> >>Note that detection by the prop bit works the same as the existing detection
> >> >> >>and will ensure that no other tags get set on the device.
> >> >> >>
> >> >> >>Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> >> >>---
> >> >> >>  src/shared/missing.h             | 4 ++++
> >> >> >>  src/udev/udev-builtin-input_id.c | 5 +++++
> >> >> >>  2 files changed, 9 insertions(+)
> >> >> >>
> >> >> >>diff --git a/src/shared/missing.h b/src/shared/missing.h
> >> >> >>index 3bdfd8f..4464e35 100644
> >> >> >>--- a/src/shared/missing.h
> >> >> >>+++ b/src/shared/missing.h
> >> >> >>@@ -944,3 +944,7 @@ static inline int kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1, uns
> >> >> >>  #ifndef INPUT_PROP_POINTING_STICK
> >> >> >>  #define INPUT_PROP_POINTING_STICK 0x05
> >> >> >>  #endif
> >> >> >>+
> >> >> >>+#ifndef INPUT_PROP_ACCELEROMETER
> >> >> >>+#define INPUT_PROP_ACCELEROMETER  0x06
> >> >> >>+#endif
> >> >> >>diff --git a/src/udev/udev-builtin-input_id.c b/src/udev/udev-builtin-input_id.c
> >> >> >>index d4c38ca..ecfc447 100644
> >> >> >>--- a/src/udev/udev-builtin-input_id.c
> >> >> >>+++ b/src/udev/udev-builtin-input_id.c
> >> >> >>@@ -136,6 +136,11 @@ static void test_pointers (struct udev_device *dev,
> >> >> >>          int is_mouse = 0;
> >> >> >>          int is_touchpad = 0;
> >> >> >>
> >> >> >>+        if (test_bit (INPUT_PROP_ACCELEROMETER, bitmask_props)) {
> >> >> >>+                udev_builtin_add_property(dev, test, "ID_INPUT_ACCELEROMETER", "1");
> >> >> >>+                return;
> >> >> >>+        }
> >> >> >>+
> >> >> >
> >> >> >So this property is only set for accelerometer-only devices?
> >> >>
> >> >> Hmm, good question. Currently I see only one user of it in the kernel:
> >> >>
> >> >> drivers/hid/wacom_wac.c
> >> >
> >> > Yep, this is the first and only for now.
> >> >
> >> >>
> >> >> Which most likely does not count as an accelerometer only device, so maybe
> >> >> my idea to follow the existing accelerometer detection code and short-circuit
> >> >> the other tests was not such a good idea.
> >> >>
> >> >> Benjamin, do you have any input on this ?
> >> >
> >> > See Peter's documentation patch here:
> >> > https://patchwork.kernel.org/patch/6102851/
> >> >
> >> > If the property is set, "Directional axes on this device (absolute
> >> > and/or relative x, y, z) represent accelerometer data. All other axes
> >> > retain their meaning. A device must not mix regular directional axes and
> >> > accelerometer axes on the same event node."
> >> >
> >> > So we can have multiple axis, multiple buttons but X,Y,Z will be accel
> >> > values.
> >> >
> >> > The Cintiq 27 QHD has a builtin accelerometer and it is reported by this
> >> > property to explicitly say that the X,Y,Z are not stylus data but
> >> > accelerometers values.
> >> >
> >> > For this device, there is no problem because libwacom will set
> >> > ID_INPUT_TABLET through a custom udev rule, but for others, the "return"
> >> > will be a problem IMO (note that Wacom already breaks the ID_INPUT_* tag
> >> > should be set only once per device).
> >>
> >> Yeah, thanks!
> >>
> >> Hans, I'm not sure your patch is correct. I mean, just because the
> >> input-device exports an accelerometer, doesn't necessarily mean that
> >> we should tag it as such, right? Or does the Cintiq 27 QHD export
> >> _multiple_ input devices, and the accelerometer has its own evdev
> >> node? In that case, I'm fine with it. But otherwise, we should still
> >> tag it as tablet, right?
> >
> > It's a little bit messier than that actually :)
> >
> > I think (I never had it in my hands, so this is from reading the driver)
> > that the 27 QHD (Touch) will show up as:
> > - "Wacom Cintiq 27QHD touch Pen" -> regular tablet input (for the
> >   stylus) which will be seen by udev as a tablet
> > - "Wacom Cintiq 27QHD touch Touch" -> regular multitouch screen,
> >   libwacom will set the ID_INPUT_TABLET tag (no way for udev to detect
> >   it except lookking at the siblings and this might not be reliable)
> > - "Wacom Cintiq 27QHD touch Pad" -> this will have
> >   INPUT_PROP_ACCELEROMETER, ABS_X|Y|Z, KEY_PROG1|2|3 -> so setting only
> >   ID_INPUT_ACCELEROMETER for it is fine, there will be no way to
> >   reliably detect the tablet capability (unless the kernel provides it)
> >
> > So I think you are both right (and wrong). Hans' patch works with this
> > current device, but it won't allow to have other ID_INPUT_* tags set.
> > And David is right, but we should not tag as ID_INPUT_TABLET :)
> 
> Ok, so what's the expected behavior? The "touch Touch" device should
> be marked as TABLET, the others should not be marked at all? libinput
> and friends can thus detect it as master device?
> What happens if all are marked as TABLET?
> What tagging do you guys expect/want?

What we want is simple:
- (multi)touch devices tagged as such (here the "touch Touch")
- stylus devices marked as tablet (here the "touch Pen")
- accelerometers tagged as such (here the "touch Pad"), in addition to
  every other ID_INPUT (keyboard, mouse, etc...)

Don't bother with marking the other nodes as tablet, libwacom adds a
rule which does that based on the VID:PID:name.

> 
> I'm not really sure tagging those devices makes any sense at all. You
> need to know how they work to make any use of them. No idea.. maybe

I am not requesting to implement a specific udev builtin for such
devices. We mostly treat them out of udev through custom rules. As long
as udev tags devices on a reliable manner, we are fine and can do
whatever needs to be done later.

For udev to properly tag tablets, we would need to add a kernel property
INPUT_PROP_TABLET and then, yes, the built-ins could do something about
that. Until then, the heuristic is fine.

> Hans' patch should be applied unchanged. At least we now know the
> background story.
> 

I don't think we should return when we see INPUT_PROP_ACCEL. If a
keyboard embeds an accelerometer, we will miss it. I think I went too
deep in detail for Wacoms/tablets devices and it confused you. Those
don't need much processing in udev, we can mostly control everything
out the tree.

Cheers,
Benjamin