Hi Jason,
[I've seen v2 and v3 but the discussion is mainly going there, so
pulling this one out of the archives]
On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> >> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> >> solution to the problem of the driver having few good heuristics to use
> >> to determine if two devices should be considered siblings or not. This
> >> commit replaces them with heuristics that leverage the information we
> >> have available to determine if two devices are likely siblings or not.
> >
> > I agree it's nicer to have a better heuristic for sibling matching,
> > but I wonder if this heuristic is reliable enough when talking about
> > future devices. It might be, but I know from experience that the
> > firmware team can be very original and find a way that screws up us all
> > :/
> >
> > Keeping the safety net of reducing the checks with ovid/opid might come
> > handy in the future.
> >
>
> The biggest problem with oVid/oPid is that they're kinda antithetical to
> the HID_GENERIC codepath. If a device is split between two PIDs then
> arbitration is broken for any kernel that doesn't include specific
> support for the device.
Well, we currently already have different paths for HID_GENERIC and
other various protocols. Why is that a problem to have heuristics for
HID_GENERIC and ovid/opid for those we need to have special handling?
>
> I agree that heuristics aren't as confidence-inspiring as explicit
> notation, but if they can be made to work well enough then I'd prefer
> using them. Either way, I suppose as userspace starts taking over
> arbitration duties it will become a more and more moot issue.
Yep, but currently the patch might link 2 devices that were not bound
together before, so I still think ovid/opid is the best way for the non
generic devices. For generic devices (future I think) we can always ask
people to use userspace touch arbitration.
>
> >>
> >> Written out, the new heuristics are basically:
> >>
> >> * If a device with the same VID/PID as that being probed already exists,
> >> it should be preferentially checked for siblingship.
> >
> > That's assuming you don't have 2 27QHD connected as 2 monitors (if you
> > really have a lot of money to spend) :)
> >
> > I think the code is OK, just not the comment above (mostly the
> > "preferentially" word itches me)
> >
>
> I'll try to come up with better / more clear wording (see my later
> comments on the "Try to find an already-probed interface from the same
> device" hunk for more detail).
Thanks for the changes in v2/3
>
> >>
> >> * Two HID devices which have the same VID/PID are not siblings if they
> >> are not part of the same logical hardware device.
> >>
> >> * Two HID devices which have different VID/PIDs are not siblings if
> >> they have different parent (e.g. USB hub) devices.
> >>
> >> * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
> >> siblings of devies with the same VID/PID (Wacom often splits their
> >> direct input tablets into two devices to make it easier to meet
> >> Microsoft's touchscreen requirements).
> >
> > That's a strong assumption on the future. If you are forced to use the
> > Precision Touchpad protocol for Intuos, this will just blow up.
> >
>
> I originally didn't include this condition in my checks since I was
> similarly wary. Leaving it out does open two small corner cases though.
>
> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
> Touch-only Wacom tablets aren't particularly common and it would
> be a little strange to have one paired with a pen-only tablet, but it
> could conceivably happen. Although pairing the devices shouldn't
> normally be an issue since a user isn't likely to use both
> simultaneously, it might cause problems for multi-seat setups.
Yes, multi-seats is one big issue here.
>
> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
> which has the *touch* interface probed first. As far as I'm aware, there
> aren't any Wacom devices that have the USB interfaces ordered
> touch-then-pen, but as you point out, who knows what those tricksy
> firmware engineers will do in the future to ruin your day.
And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
is a direct device which I assume doesn't have an internal hub, you end
up binding the 21UX pen with the 13HD touch, and things gets nasty for
the 13HD pen interface.
>
> I'm open to leaving the check in or out depending on your feelings. If
> you've got thoughts on how to close these corner cases as well, I'm all
> ears :)
I really think the ovid/opid approach solves most of the issues with the
current hardware. You are concerned about future hardware that will be
handled by HID_GENERIC. Why not just adding a special case for
HID_GENERIC that uses your heuristics?
In userspace I think we will still have the ovid/opid approach in
libwacom because it restricts the amount of combinations by a very good
factor.
If the kernel with the new heuristics (HID_GENERIC only) fails, we will
be able to tell users to switch to userspace touch arbitration.
/me turns in circle a little, but how does that sounds?
Cheers,
Benjamin
>
> >>
> >> * Two devices which have different "directness" are not siblings.
> >>
> >> * Two devices which do not serve complementary roles (i.e. pen/touch)
> >> are not siblings.
> >
> > I think it would be useful to have these write outs as comments in the
> > code. It's quite tricky to understand the actual code without these
> > explanations.
> >
>
> Ack.
>
> >>
> >> Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx>
> >> ---
> >> drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
> >> drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
> >> drivers/hid/wacom_wac.h | 2 --
> >> 3 files changed, 62 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> >> index 4a0bb6f..a5bc038 100644
> >> --- a/drivers/hid/wacom_sys.c
> >> +++ b/drivers/hid/wacom_sys.c
> >> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
> >> {
> >> struct wacom *wacom = hid_get_drvdata(hdev);
> >> struct wacom_features *features = &wacom->wacom_wac.features;
> >> - int vid = features->oVid;
> >> - int pid = features->oPid;
> >> - int n1,n2;
> >> + struct wacom *sibling_wacom = hid_get_drvdata(sibling);
> >> + struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
> >> + int n1, n2;
> >>
> >> - if (vid == 0 && pid == 0) {
> >> - vid = hdev->vendor;
> >> - pid = hdev->product;
> >> + /* Compare the physical path. Require devices with the same
> >> + * PID to share the same device, and devices with different
> >> + * PIDs to share parent devices.
> >> + */
> >
> > I stumbled this morning upon:
> > http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/
> >
> > Please make sure to follow the comments style guidelines :)
> >
>
> Whatever makes the style gods happy :)
>
> >> + if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
> >> + n1 = strrchr(hdev->phys, '/') - hdev->phys;
> >> + n2 = strrchr(sibling->phys, '/') - sibling->phys;
> >> + }
> >> + else {
> >> + n1 = strrchr(hdev->phys, '.') - hdev->phys;
> >> + n2 = strrchr(sibling->phys, '.') - sibling->phys;
> >> }
> >>
> >> - if (vid != sibling->vendor || pid != sibling->product)
> >> + if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >> return false;
> >>
> >> - /* Compare the physical path. */
> >> - n1 = strrchr(hdev->phys, '.') - hdev->phys;
> >> - n2 = strrchr(sibling->phys, '.') - sibling->phys;
> >> - if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >> + if (strncmp(hdev->phys, sibling->phys, n1))
> >> + return false;
> >> +
> >> + if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
> >> + if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
> >> + return false;
> >> + }
> >
> > As mentioned in the commit log, I am not sure it's such a good idea to
> > have this check. Does it really remove a false positive or can this just
> > be considered as an extra check that can be safely removed?
> >
>
> See comment above.
>
> >> +
> >> + if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
> >> + (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
> >> return false;
> >
> > unless I am mistaken, you might as well need {if indirect and sibling
> > is not indirect, than false }.
> >
>
> That case should be covered since we're comparing the actual values of
> each device's "direct" flag for equality (direct/indirect and
> indirect/direct will return false).
>
> If its not immediately clear though, I could decompose it into
> independent checks for each case. E.g.:
>
> if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
> !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
> return false;
>
> if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
> (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
> return false;
>
> >>
> >> - return !strncmp(hdev->phys, sibling->phys, n1);
> >> + if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
> >> + !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
> >> + return false;
> >> +
> >> + if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
> >> + !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
> >> + return false;
> >
> > I think it would be better to have those last 3 (plus mine) tests at the
> > beginning, before doing string lookouts. They seem to be the most
> > reliable ones and able to exclude a lot of false positive.
> >
>
> Reasonable enough.
>
> >> +
> >> + return true;
> >> }
> >>
> >> static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
> >> {
> >> struct wacom_hdev_data *data;
> >>
> >> + /* Try to find an already-probed interface from the same device */
> >> + list_for_each_entry(data, &wacom_udev_list, list) {
> >> + int n1, n2;
> >> + n1 = strrchr(hdev->phys, '/') - hdev->phys;
> >> + n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
> >> + if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >> + continue;
> >> + if (!strncmp(hdev->phys, data->dev->phys, n1))
> >> + return data;
> >> + }
> >
> > I can't see the benefit of having this here, while it seems to be
> > already tested in wacom_are_sibling().
> >
>
> There's a subtle issue with not performing this search before proceeding
> to trying to pair arbitrary devices in wacom_are_sibling. Imagine that
> you've already got a pen-only device connected to your system. Now,
> connect a (non-split) pen+touch device to the same hub. When the touch
> interface is probed, the first device checked its checked against in
> wacom_are_sibling is that pen-only device, and provided it passes all
> the checks then it will be incorrectly paired with it.
>
> This is actually probably a leftover from before I added the requirement
> that only direct devices can have different PIDs and might not be
> strictly necessary in the patch's current state, but I'd feel better
> keeping it in either way.
>
> > Also, if there is a need for it, there is a common pattern used 3 times
> > here:
> > int n1, n2;
> > n1 = strrchr(hdev->phys, separator) - hdev->phys;
> > n2 = strrchr(other->dev->phys, separator) - other->dev->phys;
> > if (n1 != n2 || n1 <= 0 || n2 <= 0)
> > continue;
> > return !strncmp(hdev->phys, other->dev->phys, n1);
> >
> > It would make sense to put a name on it and have a separate function.
> >
> > Cheers,
> > Benjamin
> >
>
> Good call, if the block remains necessary.
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
> >> +
> >> + /* Fallback to finding devices that appear to be "siblings" */
> >> list_for_each_entry(data, &wacom_udev_list, list) {
> >> if (wacom_are_sibling(hdev, data->dev)) {
> >> kref_get(&data->kref);
> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> >> index 2523a29..cb6fc63 100644
> >> --- a/drivers/hid/wacom_wac.c
> >> +++ b/drivers/hid/wacom_wac.c
> >> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
> >> static const struct wacom_features wacom_features_0xF8 =
> >> { "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
> >> WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0xF6 =
> >> { "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0x32A =
> >> { "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
> >> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
> >> static const struct wacom_features wacom_features_0x32B =
> >> { "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
> >> WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x32C =
> >> { "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
> >> + .touch_max = 10 };
> >> static const struct wacom_features wacom_features_0x3F =
> >> { "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
> >> CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
> >> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
> >> static const struct wacom_features wacom_features_0x333 =
> >> { "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
> >> WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x335 =
> >> { "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0xC7 =
> >> { "Wacom DTU1931", 37832, 30305, 511, 0,
> >> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
> >> static const struct wacom_features wacom_features_0x59 = /* Pen */
> >> { "Wacom DTH2242", 95640, 54060, 2047, 63,
> >> DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x5D = /* Touch */
> >> { "Wacom DTH2242", .type = WACOM_24HDT,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0xCC =
> >> { "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
> >> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
> >> static const struct wacom_features wacom_features_0x5B =
> >> { "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
> >> WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x5E =
> >> { "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0x90 =
> >> { "Wacom ISDv4 90", 26202, 16325, 255, 0,
> >> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
> >> static const struct wacom_features wacom_features_0x307 =
> >> { "Wacom ISDv5 307", 59152, 33448, 2047, 63,
> >> CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x309 =
> >> { "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0x30A =
> >> { "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
> >> CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x30C =
> >> { "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
> >> + .touch_max = 10,
> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >> static const struct wacom_features wacom_features_0x318 =
> >> { "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
> >> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
> >> static const struct wacom_features wacom_features_0x325 =
> >> { "Wacom ISDv5 325", 59552, 33848, 2047, 63,
> >> CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >> static const struct wacom_features wacom_features_0x326 = /* Touch */
> >> - { "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
> >> - .oPid = 0x325 };
> >> + { "Wacom ISDv5 326", .type = HID_GENERIC };
> >> static const struct wacom_features wacom_features_0x323 =
> >> { "Wacom Intuos P M", 21600, 13500, 1023, 31,
> >> INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
> >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> >> index 7ad6273..a5bd05a 100644
> >> --- a/drivers/hid/wacom_wac.h
> >> +++ b/drivers/hid/wacom_wac.h
> >> @@ -181,8 +181,6 @@ struct wacom_features {
> >> int tilt_fuzz;
> >> unsigned quirks;
> >> unsigned touch_max;
> >> - int oVid;
> >> - int oPid;
> >> int pktlen;
> >> bool check_for_hid_type;
> >> int hid_type;
> >> --
> >> 2.9.0
> >>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
