[PATCH v3] dinput: Introduce new HID joystick backend.

[Rémi Bernon]

On 6/30/21 1:08 PM, Arkadiusz Hiler wrote:
> On Wed, Jun 30, 2021 at 10:54:19AM +0200, Rémi Bernon wrote:
>> This adds a new joystick backend, implemented on top of HID and without
>> any host dependencies. This will be progressively implementated, and
>> it's not going to be usable until at least a few more patches.
>>
>> Because of that, and because it may also introduce regressions compared
>> to the existing backends, it is disabled by default and is optionally
>> enabled using the following global registry key:
>>
>>    [HKCU\\Software\\Wine\\DirectInput\\Joysticks]
>>    "HID"="enabled"
>>
>> Or using the corresponding AppDefaults registry key:
>>
>>    [HKCU\\Software\\Wine\\AppDefaults\\<app.exe>\\DirectInput\\Joysticks]
>>    "HID"="enabled"
>>
>> This setting will be removed later, when it becomes usable enough, to
>> use the individual device disable mechanism available in joy.cpl.
>>
>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>> ---
>>
>> v3: Use setupapi directly, as user32 api usage was confusing. This also
>>      now matches device on either their index, HID handle or vid_pid,
>>      depending on what's been provided.
>>
>>      The instance GUID now includes the HID handle, which will be more
>>      stable than the device index in setupapi device list.
>>
>>      Matching the vid_pid assumes that setupapi device path is upper
>>      case, which I think is the case. We could make sure otherwise but
>>      I don't think we have to and it's not very convenient without wcsupr.
> 
> Yes, setupapi seems to be always uppercasing thing.
> 
>> +static HRESULT hid_joystick_device_open( DWORD index, UINT32 handle, UINT32 vid_pid, WCHAR *device_path,
>> +                                         HANDLE *device, DWORD version, PHIDP_PREPARSED_DATA *preparsed,
>> +                                         HIDD_ATTRIBUTES *attrs, HIDP_CAPS *caps, DIDEVICEINSTANCEW *instance )
>> +{
>> +    static const WCHAR vid_pid_fmt_w[] = {'V','I','D','_','%','0','4','X','&','P','I','D','_','%','0','4','X',0};
>> +    char buffer[sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W) + MAX_PATH * sizeof(WCHAR)];
>> +    SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail = (void *)buffer;
>> +    SP_DEVICE_INTERFACE_DATA iface = {sizeof(iface)};
>> +    SP_DEVINFO_DATA devinfo = {sizeof(devinfo)};
>> +    WCHAR vid_pid_w[18] = {0};
>> +    UINT32 i = 0, hid_handle;
>> +    HDEVINFO set;
>> +    DWORD type;
>> +    GUID hid;
>> +
>> +    TRACE( "index %u, handle %u, vid_pid %d\n", index, handle, vid_pid );
>> +
>> +    HidD_GetHidGuid( &hid );
>> +    snprintfW( vid_pid_w, ARRAY_SIZE(vid_pid_w), vid_pid_fmt_w, vid_pid & 0xffff, vid_pid >> 16 );
>> +
>> +    set = SetupDiGetClassDevsW( &hid, NULL, NULL, DIGCF_DEVICEINTERFACE | DIGCF_PRESENT );
>> +    if (set == INVALID_HANDLE_VALUE) return DIERR_DEVICENOTREG;
>> +
>> +    *device = NULL;
>> +    *preparsed = NULL;
>> +    while (SetupDiEnumDeviceInterfaces( set, NULL, &hid, i++, &iface ))
>> +    {
>> +        detail->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
>> +        if (!SetupDiGetDeviceInterfaceDetailW( set, &iface, detail, sizeof(buffer), NULL, &devinfo ))
>> +            continue;
> 
> This should never fail because MAX_PATH, so how about a FIXME / WARN in
> addition to that continue?
> 

Well I'm not sure if it's even worth it if it's not even supposed to 
fail. I also considered adding TRACEs to the enumeration, to be able to 
more easily see which devices were skipped, but this quickly becomes 
verbose.

>> +        if (!SetupDiGetDevicePropertyW( set, &devinfo, &DEVPROPKEY_HID_HANDLE, &type,
>> +                                        (BYTE *)&hid_handle, sizeof(hid_handle), NULL, 0 ) ||
>> +            type != DEVPROP_TYPE_UINT32)
>> +            continue;
> 
> I was thinking about sticking to RawInput enumeration and using the
> handles that you get from there instead of the indexes. This way you
> would need to do GetRawInputDeviceInfo() only in
> hid_joystick_enum_device() or maybe another helper that would try to map
> vidpid from product GUID to a handle.
> 
> With that you would be able to make hid_joystick_device_open() take the
> RawInput handle as the only parameter identifying the device.
> 
> This would also work on Windows (after the PE conversion), but I don't
> think it's our goal, so I believe that setupapi may be a better choice
> here.
> 

Yeah it's actually inconvenient that it doesn't work anymore on Windows 
(compared to the previous version), I was trying the PE-converted dinput 
on Windows too and only realized late that it doesn't work anymore.

It's probably not too much of an issue, but if we're interested in 
making it work there we could perhaps use "i" as a hid_handle fallback? 
Assuming that all HID devices will have a handle on Wine it should not 
have any impact there.

I don't have any better idea right now, and using setupapi is actually 
an improvement IMHO. In any case it can probably be done as a separate 
later change, after the conversion, as there's the critical section 
debug field usage which is also incompatible.

>> +        if (!hid_joystick_device_try_open( detail->DevicePath, device, preparsed, attrs, caps, instance ))
>> +            continue;
>> +
>> +        if (!handle && !vid_pid && !index--) break;
>> +        if (handle && handle == hid_handle) break;
>> +        if (vid_pid && strstrW( detail->DevicePath, vid_pid_w )) break;
> 
> You have the device handle here, so why not just use HidD_GetAttibutes()?
> 
> Another option would be to use GetRawInputDeviceInfo() with the
> hid_handle.
> 

Indeed, much better idea to use the attrs, I moved the code around a few 
times after adding the vid_pid match and didn't reconsider that logic.

>> +        CloseHandle( *device );
>> +        HidD_FreePreparsedData( *preparsed );
>> +        *device = NULL;
>> +        *preparsed = NULL;
>> +    }
>> +
>> +    SetupDiDestroyDeviceInfoList( set );
>> +    if (!*device || !*preparsed) return DIERR_DEVICENOTREG;
>> +
>> +    lstrcpynW( device_path, detail->DevicePath, MAX_PATH );
>> +    instance->guidInstance = hid_joystick_guid;
>> +    instance->guidInstance.Data3 = hid_handle;
>> +    instance->guidProduct = DInput_PIDVID_Product_GUID;
>> +    instance->guidProduct.Data1 = MAKELONG( attrs->VendorID, attrs->ProductID );
>> +    instance->dwDevType = get_device_type( version, caps->Usage != HID_USAGE_GENERIC_GAMEPAD ) | DIDEVTYPE_HID;
>> +    instance->guidFFDriver = GUID_NULL;
>> +    instance->wUsagePage = caps->UsagePage;
>> +    instance->wUsage = caps->Usage;
>> +    return DI_OK;
>> +}
>> +
>> +static HRESULT hid_joystick_enum_device( DWORD type, DWORD flags, DIDEVICEINSTANCEW *instance,
>> +                                         DWORD version, int index )
>> +{
>> +    HIDD_ATTRIBUTES attrs = {sizeof(attrs)};
>> +    PHIDP_PREPARSED_DATA preparsed;
>> +    WCHAR device_path[MAX_PATH];
>> +    HIDP_CAPS caps;
>> +    HANDLE device;
>> +    HRESULT hr;
>> +
>> +    TRACE( "type %x, flags %#x, instance %p, version %04x, index %d\n", type, flags, instance, version, index );
>> +
>> +    hr = hid_joystick_device_open( index, 0, 0, device_path, &device, version,
>> +                                   &preparsed, &attrs, &caps, instance );
> 
> hid_joystick_device_open() is getting a bit unwieldy with all the
> possible parameter combinations. I don't think it needs the enumeration
> / vidpid lookup logic.
> 
> IMO it would read better if hid_joystick_device_open() would take the
> path or RI handle and enum logic would be moved either here or to
> a helper.
> 

I don't think we need yet another device list cache, and I would rather 
avoid it if possible. It's really just two call sites.

Instead of the two additional params we could use the DIDEVICEINSTANCEW 
parameter as an in/out filter, providing the desired GUIDs to match (or 
zeroed when enumerating all devices).

>> +    if (hr != DI_OK) return hr;
>> +
>> +    HidD_FreePreparsedData( preparsed );
>> +    CloseHandle( device );
>> +
>> +    if (instance->dwSize != sizeof(DIDEVICEINSTANCEW))
>> +        return S_FALSE;
>> +    if (version < 0x0800 && type != DIDEVTYPE_JOYSTICK)
>> +        return S_FALSE;
>> +    if (version >= 0x0800 && type != DI8DEVCLASS_ALL && type != DI8DEVCLASS_GAMECTRL)
>> +        return S_FALSE;
>> +
>> +    if (device_disabled_registry( "HID", TRUE ))
>> +        return DIERR_DEVICENOTREG;
>> +
>> +    TRACE( "Found device %s, usage %04x:%04x, product %s, instance %s, name %s\n", debugstr_w(device_path),
>> +           instance->wUsagePage, instance->wUsage, debugstr_guid( &instance->guidProduct ),
>> +           debugstr_guid( &instance->guidInstance ), debugstr_w(instance->tszInstanceName) );
>> +
>> +    return DI_OK;
>> +}
>> +
>> +static HRESULT hid_joystick_create_device( IDirectInputImpl *dinput, REFGUID guid, IDirectInputDevice8W **out )
>> +{
>> +    DIDEVICEINSTANCEW instance = {sizeof(instance)};
>> +    GUID inst_guid = *guid, prod_guid = *guid;
>> +    DWORD size = sizeof(struct hid_joystick);
>> +    HIDD_ATTRIBUTES attrs = {sizeof(attrs)};
>> +    struct hid_joystick *impl = NULL;
>> +    PHIDP_PREPARSED_DATA preparsed;
>> +    UINT32 handle = 0, vid_pid = 0;
>> +    DIDATAFORMAT *format = NULL;
>> +    WCHAR device_path[MAX_PATH];
>> +    HIDP_CAPS caps;
>> +    HANDLE device;
>> +    HRESULT hr;
>> +
>> +    TRACE( "dinput %p, guid %s, out %p\n", dinput, debugstr_guid( guid ), out );
>> +
>> +    *out = NULL;
>> +
>> +    inst_guid.Data3 = hid_joystick_guid.Data3;
>> +    prod_guid.Data1 = DInput_PIDVID_Product_GUID.Data1;
>> +    if (IsEqualGUID( &hid_joystick_guid, &inst_guid )) handle = guid->Data3;
> 
> Data3 is only a short. I think we should use 64 bits for our magic and
> another 64 bits for the RawInput handle.
> 
> Or 32 if we go with our internal knowledge about how those handles are
> implemented in Wine.
> 

Should we really worry about overflowing the short capacity for the HID 
handle? If we do we could combine Data2 and Data3, or XOR it in Data1 
(just to keep a bit of randomness), but as the property is also checked 
to be 32bit I don't think we need to use more than that.
-- 
Rémi Bernon <rbernon at codeweavers.com>