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

[Arkadiusz Hiler]

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
>
> 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?
> -
>    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.
> -
>    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.
> -
>    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.
> - 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.