Hi,

On 03-04-15 20:21, Benjamin Tissoires wrote:
On Apr 03 2015 or thereabouts, David Herrmann wrote:
Hi

On Fri, Apr 3, 2015 at 5:30 PM, Benjamin Tissoires <btiss...@redhat.com> wrote:
On Apr 03 2015 or thereabouts, David Herrmann wrote:
Hi

On Fri, Apr 3, 2015 at 4:38 PM, Benjamin Tissoires <btiss...@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 <hdego...@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 <hdego...@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.

Ok, I'll respin my patch to not have the return then.

Regards,

Hans


_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Reply via email to