Re: [PATCH 2/2] hid: Don't sign-extend 16-bit values.

[Kai Krakow]

Hi!

Am Mo., 11. Feb. 2019 um 03:27 Uhr schrieb Zebediah Figura
:
> Wine can pick up controllers from udev, either via hidraw or evdev, as
> well as from SDL. As of bd9e130ee, we shouldn't be using signed offsets
> anymore when reporting SDL controllers, so I assumed that your
> controller was coming through hidraw directly.

Not sure, does the log show this? I'm pretty sure it's going through
SDL because I can apply SDL controller mappings to it through
environment.
> The symptoms you describe imply that anything above 0x6000 (?) is
> somehow wrapping around to negative, though that doesn't necessarily
> make very much sense.

Indeed, I'm confused too. ;-)
> Can you provide a log with
> +hid,+hidp,+hid_report,+rawinput?

Sure, I'll do. But please wait, I really need some sleep now, this was
bothering me for a few hours now. ;-)

Just let me know: Two logs? One with patches reverted and one without?

Thanks,
Kai
> On 2/10/19 7:16 PM, Kai Krakow wrote:
> > Hey Zeb!
> >
> > Am So., 10. Feb. 2019 um 22:57 Uhr schrieb Zebediah Figura
> > :
> > > Is the controller being reported through udev, then?
> >
> > Not exactly sure, what you mean... In Steam, I disabled controller
> > configuration support for this controller because it messes with the
> > axis, converting them to mouse movement in some games. So it probably
> > doesn't go through the Steam uinput implementation.
> >
> > "udevadm monitor" shows the controller when turning it on:
> >
> > KERNEL[20453.657006] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71
> > (bluetooth)
> > UDEV  [20453.679748] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71
> > (bluetooth)
> > KERNEL[20453.756728] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> > (hid)
> > KERNEL[20453.757194] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19
> > (input)
> > KERNEL[20453.757408] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/js0
> > (input)
> > KERNEL[20453.757459] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/event17
> > (input)
> > KERNEL[20453.757491] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/hidraw/hidraw10
> > (hidraw)
> > KERNEL[20454.753078] bind
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> > (hid)
> > KERNEL[20454.770809] change
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> > (power_supply)
> > UDEV  [20454.778571] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> > (hid)
> > UDEV  [20454.782705] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19
> > (input)
> > UDEV  [20454.783105] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/hidraw/hidraw10
> > (hidraw)
> > UDEV  [20454.792791] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/js0
> > (input)
> > UDEV  [20455.001865] add
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/input/input19/event17
> > (input)
> > UDEV  [20455.004553] bind
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013
> > (hid)
> > UDEV  [20455.005809] change
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> > (power_supply)
> > KERNEL[20455.739024] change
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> > (power_supply)
> > UDEV  [20455.740310] change
> > /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.6/2-1.6.3/2-1.6.3:1.0/bluetooth/hci0/hci0:71/0005:045E:02FD.0013/power_supply/xpadneo_batt_3a:66:33:3a:38:63_0
> > (power_supply)
> >
> > "xpadneo" uses a trick to work around libSDL wrongly remapping its
> > buttons and controllers by faking a different interface serial/version
> > number. This is needed because "xpadneo" actually exposes compatible
> > button and axis mapping while the current firmware of the controller
> > puts the controller into "Android" mode when paired via BT in Linux.
> > While both Steam and SDL distribute a SDL_Mapping which works around
> > this, it still doesn't work for games which access /dev/input/js0
> > directly. But the fix implemented by "xpadneo" does work correctly
> > here but now has to sidestep "broken" SDL and Steam behavior. But
> > that's all it does, it shouldn't break the axis values itself.
> > Actually, this fake interface serial has been an idea developed by the
> > author of xpadneo and me. OTOH, this means libSDL or Steam do not do
> > any remapping with this driver.
> > > Does the attached patch improve anything?
> >
> > Thanks, I'll try...
> >
> > [some minutes later]
> >
> > No, it's still the same.
> >
> > Native Linux games running through Steam: Works just right, axis work
> > as usual. "evtest" shows no strange jumps in values.
> >
> > Windows games running through SteamPlay: Axis are broken.
> >
> > The following can be observed:
> >
> > Vertical axis still work normally.
> >
> > Horizontal axis work normally to the left but will flip over to left
> > when pushed further than maybe half-way to the right. According to
> > "evtest" that would be somewhere when we cross 24000-26000 (on a scale
> > -32768..32767)...
> >
> > This is really strange, it tells me that there's an underlying problem
> > somewhere else. Shouldn't all axis be affected? Maybe there's a buffer
> > overflow or overwrite somewhere?
> >
> > Reverting "hid: Don't sign-extend 16-bit values" does not fix the
> > problem as far as I could find out (I don't know why it did for Aric).
> > This is another clue that the problem was introduced by another
> > commit. Sorry for the confusion at this point.
> >
> > I'm using Proton rebased to wine-4.1, so it includes all the Proton patches.
> >
> > Thanks,
> > Kai
> > > On 2/10/19 1:10 PM, Kai Krakow wrote:
> > > > Hello again!
> > > >
> > > > I'm not sure if this commit is the problem, with it reverted I still
> > > > see strange effects: Moving the left stick only half-way right still
> > > > works correctly, moving it further turns it mirrored into left. Have
> > > > there been any other changes related to this? It worked fine in 4.0,
> > > > and the only other related change I could find here in my system was
> > > > updating from libSDL2-2.0.8 to 2.0.9 but I AFAIR this version still
> > > > worked with 4.0 correctly.
> > > >
> > > > Thanks,
> > > > Kai
> > > >
> > > > Am So., 10. Feb. 2019 um 19:02 Uhr schrieb Kai Krakow :
> > > > > Hello!
> > > > >
> > > > > This change seems to introduce odd behavior in at least some games:
> > > > > Gamepad axis are mirrored to one side only now. This happens in Shadow
> > > > > of War (found by me) and Rocket League (reported by a user). I expect
> > > > > other games to fail in a similar way but didn't test those yet.
> > > > >
> > > > > I guess the clue is in "_may_ report a logical range"...
> > > > >
> > > > > I'm using the xpadneo driver with an Xbox One S wireless controller,
> > > > > and it reports -32768..32767 according to the driver (which follows
> > > > > the Linux spec on joysticks for this). This used to work perfectly
> > > > > before this change.
> > > > >
> > > > > Regards,
> > > > > Kai
> > > > >
> > > > > Am Mi., 6. Feb. 2019 um 19:42 Uhr schrieb Aric Stewart :
> > > > > > Signed-off-by: Aric Stewart
> > > > > >
> > > > > > On 2/5/19 1:09 PM, Zebediah Figura wrote:
> > > > > > > From: Zebediah Figura 
> > > > > > >
> > > > > > > Some controllers (including, with the previous patch, any reported
> > > > > > > through SDL) may report a logical range of [0,65535], which takes up
> > > > > > > 16 bits but should not be sign-extended.
> > > > > > >
> > > > > > > Signed-off-by: Zebediah Figura 
> > > > > > > ---
> > > > > > >     dlls/hid/hidp.c | 4 ----
> > > > > > >     1 file changed, 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/dlls/hid/hidp.c b/dlls/hid/hidp.c
> > > > > > > index f9978038e3..15d827edf1 100644
> > > > > > > --- a/dlls/hid/hidp.c
> > > > > > > +++ b/dlls/hid/hidp.c
> > > > > > > @@ -277,8 +277,6 @@ NTSTATUS WINAPI HidP_GetScaledUsageValue(HIDP_REPORT_TYPE ReportType, USAGE Usag
> > > > > > >                                  element->valueStartBit, element->bitCount, &rawValue);
> > > > > > >             if (rc != HIDP_STATUS_SUCCESS)
> > > > > > >                 return rc;
> > > > > > > -        if (element->caps.value.BitSize == 16)
> > > > > > > -            rawValue = (short)rawValue;
> > > > > > >             *UsageValue = rawValue;
> > > > > > >         }
> > > > > > >
> > > > > > > @@ -925,8 +923,6 @@ NTSTATUS WINAPI HidP_GetData(HIDP_REPORT_TYPE ReportType, HIDP_DATA *DataList, U
> > > > > > >                                          element->valueStartBit, element->bitCount, &v);
> > > > > > >                     if (rc != HIDP_STATUS_SUCCESS)
> > > > > > >                         return rc;
> > > > > > > -                if (element->caps.value.BitSize == 16)
> > > > > > > -                    v = (short)v;
> > > > > > >                     DataList[uCount].DataIndex = element->caps.value.u.NotRange.DataIndex;
> > > > > > >                     DataList[uCount].u.RawValue = v;
> > > > > > >                 }
> > > > > > >