winebus.sys: Only apply hidraw disable hacks to hidraw subsystem

[twhitehead]

Due to the location of an if check, PROTON_ENABLE_HIDRAW, SDL_GAMECONTROLLER_ALLOW_STEAM_VIRTUAL_GAMEPAD, SDL_GAMECONTROLLER_IGNORE_DEVICES_EXCEPT, and SDL_GAMECONTROLLER_IGNORE_DEVICES apply to the input subsystem too.

The logged messages are "hidraw %s: deferring %s to a different backend" and "hidraw %s: ignoring %s, in SDL blacklist", so this was probably not intended. It was likely not noticed though as the input subsystem is disabled by default due to commit e69c9b1.

Because the input subsystem is disabled by default, this change only affects those who deliberately enabling it by explicitly setting "DisableInput" to 0 in their registry. With this change it then works. Without it, they need to also whitelist the devices with PROTON_ENABLE_HIDRAW and remove any /dev/hidrawXX permissions to block the raw subsytem from gabbing the devices first.

[kakra]

Before applying this, we need to ensure that HOTAS are still correctly identified and not labelled as a gamepad, and that hidraw mode is automatically enabled and the default game input drivers are skipped.

[kakra]

This seems problematic because this will now define devices as gamepad that have 6 axis and more than 14 buttons - like common HOTAS do have. This has been explicitly in the else case so is_hidraw_enabled can override this behavior.

[kakra]

Looking at this again, the else probably didn't make sense in the first place because both if return from the function.

[twhitehead]

Yes. That is why I took it out.

I see what you mean about HOTAS. This seems to be a standard heuristics used in all the input types though (iohid and sdl). Probably the only way to really get it right is to not rely on a heuristics at all if hwdb info is available.

The hwdb info can already be extracted via the what_am_I function. It just isn't being used here. Maybe it should be?

[kakra]

Yeah, SDL has a similar problem of identifying some joysticks as gamepads... But because axis centers differ, this creates phantom movement. So we need to ensure such devices do not become mapped to the xinput driver.

[twhitehead]

I think I can mark this as resolved?

[kakra]

These ifs should probably revert is_gamepad then in case it has been unconditionally enabled above. Or the above (previous) else should be moved lower. desc.is_gamepad must not be enabled if a HOTAS was detected, otherwise it enables the xinput driver for such devices which confuses games.

[kakra]

Also looking at this again, is_gamepad probably still needs proper handling and wasn't working correctly in the first place (just by accident).

[twhitehead]

I figured it was okay to move the !is_hidraw_enabled and is_sdl_blacklisted checks past the setting of desc.is_gamepad because their only possible effect is aborting the addition of the device.

That is, my reading of the code was, in the event these checks trigger, whether if desc.is_gamepad got set or not doesn't matter as desc is just thrown away (i.e., there is no final call to to bus_event_queue_device_created(..., ..., &desc)). Maybe I missed something though?

[kakra]

If desc is thrown away after returning from the function, then everything should be fine. I didn't check the call stack, tho. I just was involved here to improve heuristics for HOTAS detection (this is why is_hidraw_enabled got additional parameters for axes and buttons). A hwdb check could probably placed there.

is_hidraw_enabled currently looks at known VID/PID, and additionally checks some well-known VID for button count (many HOTAS have either 31 or 128 buttons, some may have 8 axes but that's currently not used, so if it matches a known VID, it seems good enough as a heuristic).

[twhitehead]

Yup. My reading of the code is desc is just a local variable that is being filled in for a call at the end to bus_event_queue_device_created(..., ..., &desc), so shouldn't have any effects if there is a return before this call.

[twhitehead]

And this on also as resolved?

[kakra]

I gave this a second thought, still needs testing for the is_gamepad flag.

[kakra]

Also looking at this again, is_gamepad probably still needs proper handling and wasn't working correctly in the first place (just by accident).

[twhitehead]

@kakra I am pretty ignorant on Microsoft API and input devices. My limited exposure so far has been that CH Pro Pedals have a tendency to get classified as accelerators (here and here) without hwdb help

id-input:modalias:input:b0003v068Ep00F2e0100*
    ID_INPUT_ACCELEROMETER="0"
    ID_INPUT_JOYSTICK="1"

From googling a bit, it looks like XInput is intended for XBox controllers? So, is the objective to only enable XInput (set is_gamepad) for true gamepad controllers?

I take it a HOTAS is not something that you can plug into your XBox? Or is it just that wine cannot emulate it correctly as an XInput device due to things like the axis centers difference you mentioned creating phantom movement issue?

[twhitehead]

Also, does it matter at all on the Windows side whether a device comes in on the Unix side via the via wine's input layer or hidraw layer? Are they two equally good options, or is there stuff that requires one or the other?

[rbernon]

Also, does it matter at all on the Windows side whether a device comes in on the Unix side via the via wine's input layer or hidraw layer? Are they two equally good options, or is there stuff that requires one or the other?

Nothing in Wine itself requires it. Some devices works best with hidraw because the applications on the Windows side expect to talk to them through some vendor specific HID reports (such as Sony controllers). Other than that, in Wine, only the is_gamepad flag matters and is used to report a device as a XInput compatible gamepad or not.

Fwiw, I haven't had a look at the code yet, and it's okay to try fixing any issue Proton has with non-default options, however I think we always have SDL in Proton, so there should be no need to enable the evdev subsystem. SDL has its own environment variables to allow/disallow devices, and I think they should be used.

Then, this Proton-specific backend interaction is not meant to stay in Proton, and it would be nice if upstream Wine was also able to prefer hidraw over SDL for some devices. Instead of having a SDL vs (evdev + hidraw) backend selection where SDL excludes the two other, we should have (SDL vs evdev) + hidraw.

[kakra]

Or is it just that wine cannot emulate it correctly as an XInput device due to things like the axis centers difference you mentioned creating phantom movement issue?

From the MS docs, last time I looked, it looked like the XInput API really only cares about devices having a gamepad-like layout. A quick search reveals that it can actually distinguish multiple device types, even exotic ones:

• unknown (type 0)
• gamepad (type 1)
• wheel (type 2...)
• arcade stick (3)
• flight stick (which is more or less Xbox controller axis and buttons mapped to a joystick layout)
• dance pad
• guitar
• ...
  (https://learn.microsoft.com/en-us/windows/win32/xinput/xinput-and-controller-subtypes)

All of these devices somehow map inputs to two sticks, two analog triggers, a, b, x, y etc. So these are really just different device types of the same mapping layout with the same expectations (triggers map 0..100%, sticks map -100..100% with idle center). I'm not sure if wine implements any of the other devices types but it probably doesn't matter if we look at the buttons and axes only: A full HOTAS will never map to these expectations - even if the device type would be "flight stick". So games will confuse that with a Xbox controller if it appears as a Xinput device.

Having wine map any gamepad to xinput is probably a special feature of the Proton variant of wine because in Windows, only original Xbox controllers are Xinput devices (and proper clones are). We just need to ensure that really only devices with standard Xbox controller buttons and axes map to this driver (a,b,x,y,lb,rb,... no matter the actual names on the controller face).

My limited exposure so far has been that CH Pro Pedals have a tendency to get classified as accelerators

This is probably because in the HID definitions of the Linux kernel, devices without any buttons but just axes are classified as acceleratometers. The device driver in Linux really should provide a fake button for this device (like BTN_WHEEL, there's nothing like a foot pedal thingy) because the API implies that a device type can be detected by the first button of a button range (see input-event-codes.h in the kernel). Sadly, reality is very different: Usually, devices are running through hid-generic which just presents what the HID descriptor has - and instead of fixing the kernel, user-space tools all do heuristics and detections/fixups on their own (at least we have SDL in wine now which can play a central role here).

Also, udev doesn't seem to have a lot of choices about what type of device we are seeing. It seems to know just joysticks, no other or even some generic gaming input device.

So I think unless your patch has negative impact on HOTAS detection (because it would turn on is_gamepad and thus make it an xinput device), it could probably just go in as fixing this hack (because it works around current limitations of the wine code) - and any other more complicated work should go to wine upstream instead.

@rbernon Maybe is_xinput would have been a better name than is_gamepad (because xinput does support more than just gamepads but all of those devices have the same set of buttons and axes, while a gamepad without this set of buttons and axes would not qualify for xinput).

Maybe we could also change this line:

desc.is_gamepad = (axes == 6 && buttons >= 14);

to

desc.is_gamepad = (axes == 6 && buttons >= 14 && buttons <= 16);

because xinput controllers cannot have more than 16 buttons due to limitations of the HID reports (only 16 bit for buttons). But OTOH, I do not know if there may be controllers with more than 16 buttons but xinput would not map those buttons anyways. It would successfully avoid HOTAS (with more than 16 buttons usually) to be misidentified for xinput, except for e.g. Thrustmaster T16000m which has exactly 16 buttons (and could thus probably work with Xbox via xinput).

Actually, Xbox controllers have exactly 10 gaming buttons plus 1 or 2 extra buttons (Guide, Share). That we see more than 12 buttons is just due to the fact that the HID mode of those gamepads uses sparse keymaps and the generic HID driver in the kernel doesn't unmap the dead bits from the keymap.

The dpad may show as buttons in the keymap (depending on the driver), thus we may have 10+4 or 12+4 buttons where in the HID report the dpad never maps to the 16 bit field for buttons (but in xinput it does). This sums up to 16 buttons at most. See also https://learn.microsoft.com/en-us/windows/win32/api/xinput/ns-xinput-xinput_gamepad which has WORD for buttons and which is 16 bit: Thus a xinput device can never have more than 16 buttons.

[twhitehead]

Thanks for the info to both of you. I feel I understand what is happening better now. The

    desc.is_gamepad = (axes == 6 && buttons >= 14);

also make more sense. Offer an XInput interface to it if it has the correct number of axis and a sufficient number of buttons to give gamepad like output. I guess this has no impact on it also showing up as a DirectInput device. The only thing is you don't want to create a bunch of XInput devices from non-gamepads the user isn't going to be able to/want to see. Not to mention you only seem to be allowed 4 devices according to the XInput API docs.

Your comment about the maximum number of buttons seems entirely reasonable to me. I can also see that you would want to use SDL in Proton and not input or hidraw (unless required for the specific device as mentioned) as that is what steam uses too, so you get a more unified input handling between the two of them.

Was reading some through more of the bug reports and issues I mentioned. Maybe the thing would be to improve SDL's figuring of what devices are. @smcv mentioned issues with enabling udev in SDL (which explains my device not recognized issues) to do with the container interface and event notification. To quote

The reason why we don't use udev in versions of Proton that use the container runtime (5.13+) is that it doesn't work properly across a container boundary. The events that tell Proton/SDL about game controller plug/unplug events don't get delivered, so hotplug doesn't work (which is why it caused ValveSoftware/steam-for-linux#9150 when we accidentally used the udev code path). Also, the information that tells SDL/Proton more details about a device is not designed to be compatible between different versions of udev, but the version of libudev inside the container doesn't necessarily match the version in the host operating system. Sometimes it accidentally works (as it did for @kevenwyld recently) - but when it does, that's only through luck, and a udev upgrade could easily make it permanently stop working. So we can't rely on this

Would be really nice though if Steam/Proton, that is, SDL could still tap into the host systems device type database even if it wasn't using a full udev backend due to the container issues. This would give a more unified with the system experience and be updateable through something other than just new steam runtime/container/pressure vessel releases that have new hardware ids and heuristics baked into the contained SDL libraries.

As I mentioned in one of those bug reports, I feel the device classification part of udev (actually sd-device now) could still be used to try and classify devices even if a full udev isn't working. That is, try something like the what_am_I code used in wine before falling back to the baked in hardware ids and heuristics. I expect this would still work in a container as these parts of udev are all basically string based key-value files. Basically add a limited device-type-only udev option to SDL for compatibility with containers.

[twhitehead]

I put together a SDL2 patch to still use udev for device classification even if in a container. If anyone want to try it, I rebuilt libSDL2 for all the current steam runtimes. You can get them here (see the README.txt file for how to install them).

Note that this may not effect all programs. You can check what libSDL2 a program is using by running

lsof -p <PID> | fgrep libSDL2

where <PID> is the process id obtained from looking the program up with ps -HU $USER. On my system, for example, while Proton uses the steam runtime libSDL2.so, steam itself uses my OS libSDL2.so.

[twhitehead]

Maybe we could also change this line:

desc.is_gamepad = (axes == 6 && buttons >= 14);

to

desc.is_gamepad = (axes == 6 && buttons >= 14 && buttons <= 16);

@kakra your reasoning sounds pretty solid there. Especially the bits bit. Perhaps this is something we should see if upstream wine would take? I can put together a pull request for them if you want.

[twhitehead]

On the actual pull request (sorry for my SDL side trip polluting this thread), I think we are in agreement that, with the if branches the way there are, this should not cause any new devices to be classified as xinput devices. That is, I took this code

    ...
    if (!is_hidraw_enabled(desc.vid, desc.pid, axes, buttons))
    {
        TRACE("hidraw %s: deferring %s to a different backend\n", debugstr_a(devnode), debugstr_device_desc(&desc));
        close(fd);
        return;
    }
    if (is_sdl_blacklisted(desc.vid, desc.pid))
    {
        /* this device is blacklisted */
        TRACE("hidraw %s: ignoring %s, in SDL blacklist\n", debugstr_a(devnode), debugstr_device_desc(&desc));
        close(fd);
        return;
    }
#ifdef HAS_PROPER_INPUT_HEADER
    else
        desc.is_gamepad = (axes == 6 && buttons >= 14);
#endif

    TRACE("dev %p, node %s, desc %s.\n", dev, debugstr_a(devnode), debugstr_device_desc(&desc));

    if (strcmp(subsystem, "hidraw") == 0)
    {
        ...

and floated the two hidraw if statements down to the the if (strcmp(subsystem, "hidraw") == 0) block. This causes them to only apply to the hidraw subsytem. It leaves the desc.is_gamepad = (axes == 6 && buttons >= 14) without the else. This else did nothing anyways in the original code as the corresponding if branch always returned.

This leaves the question of it now running before the is_hidraw_enabled and the is_sdl_blacklisted if checks. This is also okay as all it does is set desc.is_gamepad. This also has no effect if either of these ifs succeed, because they both return early, so the bus_event_queue_device_created(&event_queue, &impl->unix_device, &desc) call at the end of the function doesn't run that would have made a device out of the local desc structure we are building.

I also compared with upstream wine to see how it looks. Under this pull, we would have

#ifdef HAS_PROPER_INPUT_HEADER
    desc.is_gamepad = (axes == 6 && buttons >= 14);
#endif

where upstream wine has

    if (is_xbox_gamepad(desc.vid, desc.pid))
        desc.is_gamepad = TRUE;
#ifdef HAS_PROPER_INPUT_HEADER
    else
    {
        int axes=0, buttons=0;
        axes = count_abs_axis(fd);
        buttons = count_buttons(fd, NULL);
        desc.is_gamepad = (axes == 6 && buttons >= 14);
    }
#endif

Besides the scoping of axes and buttons, the primary difference is the wine code also set desc.is_gamepad if is_xbox_gamepad(desc.vid, desc.pid) says so. In upstream wine this just checks if the vid and pid match the known xbox gamepad ids. This function isn't in valve wine, but it certainly doesn't seem like there should be any harm in adding it as it will only trigger on exact vid and pid match for xbox gamepads. I can do that if people like.

Either way though, I think the pull should be okay. I believe we have established it won't effect the hidraw subsystem or which devices are flagged for xinput. It should only disable the hidraw disabling hacks for the non-hidraw subsystems. Which is to say, the input subsystem (the final bus_event_queue_device_created call is only made inside subsystem hidraw and input checks), and valve wine has disabled the input subsystem be default, meaning it should only effect those that have edited their registry to re-enable it.

It also cleans up the code by make it obvious that desc.is_gamepad = (axes == 6 && buttons >= 14) always runs, and it bring the code back in closer alignment with upstream wine.

[kakra]

Replying to #207 (comment)

If this is an upstream issue, yes, please do. Thanks.