[PATCH 0/3] MR6878: dinput: Fix a couple of regressions.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/dinput/tests/driver_bus.c | 2 +- dlls/dinput/tests/driver_hid.c | 2 - dlls/dinput/tests/hid.c | 3 +- dlls/dinput/tests/joystick8.c | 154 ++++++++++++++++++++++++++++++++- 4 files changed, 156 insertions(+), 5 deletions(-) diff --git a/dlls/dinput/tests/driver_bus.c b/dlls/dinput/tests/driver_bus.c index cf42a1bcf6e..320b47057a5 100644 --- a/dlls/dinput/tests/driver_bus.c +++ b/dlls/dinput/tests/driver_bus.c @@ -563,7 +563,7 @@ static NTSTATUS remove_child_device( struct func_device *impl, DEVICE_OBJECT *de for (i = 0; i < impl->devices->Count; ++i) if (impl->devices->Objects[i] == device) break; if (i == impl->devices->Count) status = STATUS_NOT_FOUND; - else impl->devices->Objects[i] = impl->devices->Objects[impl->devices->Count--]; + else impl->devices->Objects[i] = impl->devices->Objects[--impl->devices->Count]; KeReleaseSpinLock( &impl->base.lock, irql ); return status; diff --git a/dlls/dinput/tests/driver_hid.c b/dlls/dinput/tests/driver_hid.c index 7b20af07493..5d92c8b230a 100644 --- a/dlls/dinput/tests/driver_hid.c +++ b/dlls/dinput/tests/driver_hid.c @@ -57,8 +57,6 @@ static void check_device( DEVICE_OBJECT *device ) ok( device == impl->expect_hid_fdo, "got device %p\n", device ); ok( device->DriverObject == expect_driver, "got DriverObject %p\n", device->DriverObject ); - if (!device->NextDevice) ok( device == impl->expect_hid_fdo, "got device %p\n", device ); - else ok( device->NextDevice == impl->expect_hid_fdo, "got NextDevice %p\n", device->NextDevice ); ok( !device->AttachedDevice, "got AttachedDevice %p\n", device->AttachedDevice ); ok( ext->MiniDeviceExtension == impl->expect_hid_ext, "got MiniDeviceExtension %p\n", ext->MiniDeviceExtension ); diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 5e48050738b..06f1e4ee7e5 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -719,7 +719,8 @@ void hid_device_stop( struct hid_device_desc *desc, UINT count ) NULL, OPEN_EXISTING, 0, NULL ); ok( control != INVALID_HANDLE_VALUE, "CreateFile failed, error %lu\n", GetLastError() ); ret = sync_ioctl( control, IOCTL_WINETEST_REMOVE_DEVICE, desc, sizeof(*desc), NULL, 0, 5000 ); - ok( ret || GetLastError() == ERROR_FILE_NOT_FOUND, "IOCTL_WINETEST_REMOVE_DEVICE failed, last error %lu\n", GetLastError() ); + ok( ret || (GetLastError() == ERROR_FILE_NOT_FOUND || GetLastError() == ERROR_NO_SUCH_DEVICE), + "IOCTL_WINETEST_REMOVE_DEVICE failed, last error %lu\n", GetLastError() ); CloseHandle( control ); if (!ret) return; diff --git a/dlls/dinput/tests/joystick8.c b/dlls/dinput/tests/joystick8.c index b18fd17c857..1b2f008702d 100644 --- a/dlls/dinput/tests/joystick8.c +++ b/dlls/dinput/tests/joystick8.c @@ -5900,6 +5900,158 @@ static void test_rawinput_desktop( const char *path, BOOL input ) DestroyWindow( hwnd ); } +struct select_default_instance_data +{ + IDirectInput8W *di8; + DIDEVICEINSTANCEW default_instance; + BOOL default_instance_found; +}; + +static BOOL CALLBACK select_default_instance( const DIDEVICEINSTANCEW *devinst, void *context ) +{ + DIPROPGUIDANDPATH prop_guid_path = + { + .diph = + { + .dwSize = sizeof(DIPROPGUIDANDPATH), + .dwHeaderSize = sizeof(DIPROPHEADER), + .dwHow = DIPH_DEVICE, + }, + }; + + DIPROPDWORD prop_dword = + { + .diph = + { + .dwSize = sizeof(DIPROPDWORD), + .dwHeaderSize = sizeof(DIPROPHEADER), + .dwHow = DIPH_DEVICE, + }, + }; + struct select_default_instance_data *d = context; + IDirectInputDevice8W *device; + HRESULT hr; + + hr = IDirectInput8_CreateDevice( d->di8, &devinst->guidInstance, &device, NULL ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + todo_wine ok( prop_dword.dwData < 100, "got %lu.\n", prop_dword.dwData ); + + hr = IDirectInputDevice8_GetProperty( device, DIPROP_GUIDANDPATH, &prop_guid_path.diph ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + trace( "%s, id %lu, inst %s, path %s.\n", debugstr_w(devinst->tszInstanceName), prop_dword.dwData, + debugstr_guid(&devinst->guidInstance), debugstr_w(prop_guid_path.wszPath) ); + if (!prop_dword.dwData) + { + ok( !d->default_instance_found, "duplicate joystick with id 0.\n" ); + d->default_instance = *devinst; + d->default_instance_found = TRUE; + } + IDirectInputDevice8_Release( device ); + return DIENUM_CONTINUE; +} + +static void test_joystick_id(void) +{ +#include "psh_hid_macros.h" + const unsigned char report_desc[] = + { + USAGE_PAGE(1, HID_USAGE_PAGE_GENERIC), + USAGE(1, HID_USAGE_GENERIC_JOYSTICK), + COLLECTION(1, Application), + USAGE(1, HID_USAGE_GENERIC_JOYSTICK), + COLLECTION(1, Physical), + USAGE_PAGE(1, HID_USAGE_PAGE_BUTTON), + USAGE_MINIMUM(1, 1), + USAGE_MAXIMUM(1, 6), + LOGICAL_MINIMUM(1, 0), + LOGICAL_MAXIMUM(1, 1), + PHYSICAL_MINIMUM(1, 0), + PHYSICAL_MAXIMUM(1, 1), + REPORT_SIZE(1, 1), + REPORT_COUNT(1, 8), + INPUT(1, Data|Var|Abs), + END_COLLECTION, + END_COLLECTION, + }; + C_ASSERT(sizeof(report_desc) < MAX_HID_DESCRIPTOR_LEN); +#include "pop_hid_macros.h" + struct hid_device_desc desc = + { + .use_report_id = TRUE, + .caps = { .InputReportByteLength = 1 }, + }; + struct hid_device_desc desc2; + + DIPROPDWORD prop_dword = + { + .diph = + { + .dwSize = sizeof(DIPROPDWORD), + .dwHeaderSize = sizeof(DIPROPHEADER), + .dwHow = DIPH_DEVICE, + }, + }; + struct select_default_instance_data d = { NULL }; + IDirectInputDevice8W *device; + IDirectInput8W *di8; + HRESULT hr; + + cleanup_registry_keys(); + + hr = DirectInput8Create( instance, DIRECTINPUT_VERSION, &IID_IDirectInput8W, (void **)&di8, NULL ); + if (FAILED(hr)) + { + win_skip( "DirectInput8Create returned %#lx.\n", hr ); + return; + } + + desc.report_descriptor_len = sizeof(report_desc); + memcpy( desc.report_descriptor_buf, report_desc, sizeof(report_desc) ); + fill_context( desc.context, ARRAY_SIZE(desc.context) ); + + desc.attributes = default_attributes; + desc2 = desc; + desc2.attributes.ProductID++; + if (!hid_device_start( &desc, 1 )) goto done; + if (!hid_device_start( &desc2, 1 )) goto done; + + d.di8 = di8; + hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + + hr = IDirectInput8_CreateDevice( di8, &GUID_Joystick, &device, NULL ); + if (d.default_instance_found) + { + ok( hr == DI_OK, "got %#lx.\n", hr ); + hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + ok( !prop_dword.dwData, "got %lu.\n", prop_dword.dwData ); + IDirectInputDevice8_Release( device ); + } + else + { + ok( hr == DIERR_DEVICENOTREG, "got %#lx.\n", hr ); + } + + hid_device_stop( &desc, 1 ); + + memset( &d, 0, sizeof(d) ); + d.di8 = di8; + hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + ok( !d.default_instance_found, "found joystick id 0.\n" ); + hr = IDirectInput8_CreateDevice( di8, &GUID_Joystick, &device, NULL ); + ok( hr == DIERR_DEVICENOTREG, "got %#lx.\n", hr ); + +done: + IDirectInput8_Release( di8 ); + hid_device_stop( &desc, 1 ); + hid_device_stop( &desc2, 1 ); + cleanup_registry_keys(); +} + START_TEST( joystick8 ) { char **argv; @@ -5911,11 +6063,11 @@ START_TEST( joystick8 ) dinput_test_init(); if (!bus_device_start()) goto done; - winetest_mute_threshold = 3; if (test_device_types( 0x800 )) { + test_joystick_id(); /* This needs to be done before doing anything involving dinput.dll * on Windows, or the tests will fail, dinput8.dll is fine though. */ test_winmm_joystick(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6878
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/dinput/joystick_hid.c | 40 ++++++++++++++++++++++++++++++++++- dlls/dinput/tests/joystick8.c | 16 ++++++++------ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 367d6356195..17067539a66 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -239,6 +239,42 @@ struct hid_joystick_effect char *set_envelope_buf; }; +struct joystick_device +{ + WCHAR device_path[MAX_PATH]; +}; + +static CRITICAL_SECTION joystick_devices_crit; +static CRITICAL_SECTION_DEBUG joystick_devices_crit_debug = +{ + 0, 0, &joystick_devices_crit, + { &joystick_devices_crit_debug.ProcessLocksList, &joystick_devices_crit_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": joystick_devices_crit") } +}; +static CRITICAL_SECTION joystick_devices_crit = { &joystick_devices_crit_debug, -1, 0, 0, 0, 0 }; + +static struct joystick_device *joystick_devices; +static unsigned int joystick_device_count; + +static unsigned int get_joystick_index( const WCHAR *device_path ) +{ + unsigned int i; + + EnterCriticalSection( &joystick_devices_crit ); + for (i = 0; i < joystick_device_count; ++i) + if (!wcsicmp( joystick_devices[i].device_path, device_path )) break; + + if (i == joystick_device_count) + { + ++joystick_device_count; + joystick_devices = realloc( joystick_devices, sizeof(*joystick_devices) * joystick_device_count ); + wcscpy( joystick_devices[i].device_path, device_path ); + } + LeaveCriticalSection( &joystick_devices_crit ); + return i; +} + + static inline struct hid_joystick_effect *impl_from_IDirectInputEffect( IDirectInputEffect *iface ) { return CONTAINING_RECORD( iface, struct hid_joystick_effect, IDirectInputEffect_iface ); @@ -814,7 +850,7 @@ static HRESULT hid_joystick_get_property( IDirectInputDevice8W *iface, DWORD pro case (DWORD_PTR)DIPROP_JOYSTICKID: { DIPROPDWORD *value = (DIPROPDWORD *)header; - value->dwData = impl->base.instance.guidInstance.Data3; + value->dwData = get_joystick_index( impl->device_path ); return DI_OK; } case (DWORD_PTR)DIPROP_GUIDANDPATH: @@ -1602,6 +1638,8 @@ static HRESULT hid_joystick_device_open( int index, const GUID *guid, DIDEVICEIN attrs, caps, instance, version ))) continue; } + /* Assign joystick index if the device path is first seen. */ + get_joystick_index( detail->DevicePath ); /* enumerate device by GUID */ if (IsEqualGUID( guid, &instance->guidProduct ) || IsEqualGUID( guid, &instance->guidInstance )) break; diff --git a/dlls/dinput/tests/joystick8.c b/dlls/dinput/tests/joystick8.c index 1b2f008702d..2e3a1ffcf56 100644 --- a/dlls/dinput/tests/joystick8.c +++ b/dlls/dinput/tests/joystick8.c @@ -2077,7 +2077,6 @@ static void test_simple_joystick( DWORD version ) prop_dword.dwData = 0xdeadbeef; hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); ok( hr == DI_OK, "GetProperty DIPROP_JOYSTICKID returned %#lx\n", hr ); - todo_wine ok( prop_dword.dwData == 0, "got %#lx expected 0\n", prop_dword.dwData ); prop_dword.dwData = 0xdeadbeef; @@ -5936,7 +5935,7 @@ static BOOL CALLBACK select_default_instance( const DIDEVICEINSTANCEW *devinst, ok( hr == DI_OK, "got hr %#lx.\n", hr ); hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); - todo_wine ok( prop_dword.dwData < 100, "got %lu.\n", prop_dword.dwData ); + ok( prop_dword.dwData < 100, "got %lu.\n", prop_dword.dwData ); hr = IDirectInputDevice8_GetProperty( device, DIPROP_GUIDANDPATH, &prop_guid_path.diph ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); @@ -6024,11 +6023,14 @@ static void test_joystick_id(void) hr = IDirectInput8_CreateDevice( di8, &GUID_Joystick, &device, NULL ); if (d.default_instance_found) { - ok( hr == DI_OK, "got %#lx.\n", hr ); - hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); - ok( hr == DI_OK, "got hr %#lx.\n", hr ); - ok( !prop_dword.dwData, "got %lu.\n", prop_dword.dwData ); - IDirectInputDevice8_Release( device ); + todo_wine ok( hr == DI_OK, "got %#lx.\n", hr ); + if (hr == DI_OK) + { + hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + ok( !prop_dword.dwData, "got %lu.\n", prop_dword.dwData ); + IDirectInputDevice8_Release( device ); + } } else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6878
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/dinput/joystick_hid.c | 16 ++++++++++++++++ dlls/dinput/tests/joystick8.c | 13 +++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 17067539a66..18d77af032d 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -274,6 +274,15 @@ static unsigned int get_joystick_index( const WCHAR *device_path ) return i; } +static BOOL get_default_joystick_device_path( WCHAR *device_path ) +{ + BOOL ret; + + EnterCriticalSection( &joystick_devices_crit ); + if ((ret = !!joystick_device_count)) wcscpy( device_path, joystick_devices[0].device_path ); + LeaveCriticalSection( &joystick_devices_crit ); + return ret; +} static inline struct hid_joystick_effect *impl_from_IDirectInputEffect( IDirectInputEffect *iface ) { @@ -2056,8 +2065,15 @@ HRESULT hid_joystick_create_device( struct dinput *dinput, const GUID *guid, IDi impl->base.read_event = CreateEventW( NULL, TRUE, FALSE, NULL ); if (memcmp( device_path_guid.Data4, guid->Data4, sizeof(device_path_guid.Data4) )) + { + /* Let hid_joystick_device_open() populate joystick devices before checking for default joystick GUID. */ hr = hid_joystick_device_open( -1, guid, &impl->base.instance, impl->device_path, &impl->device, &impl->preparsed, &attrs, &impl->caps, dinput->dwVersion ); + if (hr == DIERR_DEVICENOTREG && IsEqualGUID( guid, &GUID_Joystick ) + && get_default_joystick_device_path( impl->device_path )) + hr = hid_joystick_device_try_open( impl->device_path, &impl->device, &impl->preparsed, &attrs, + &impl->caps, &impl->base.instance, dinput->dwVersion ); + } else { wcscpy( impl->device_path, *(const WCHAR **)guid ); diff --git a/dlls/dinput/tests/joystick8.c b/dlls/dinput/tests/joystick8.c index 2e3a1ffcf56..b5ec94f4876 100644 --- a/dlls/dinput/tests/joystick8.c +++ b/dlls/dinput/tests/joystick8.c @@ -6023,14 +6023,11 @@ static void test_joystick_id(void) hr = IDirectInput8_CreateDevice( di8, &GUID_Joystick, &device, NULL ); if (d.default_instance_found) { - todo_wine ok( hr == DI_OK, "got %#lx.\n", hr ); - if (hr == DI_OK) - { - hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); - ok( hr == DI_OK, "got hr %#lx.\n", hr ); - ok( !prop_dword.dwData, "got %lu.\n", prop_dword.dwData ); - IDirectInputDevice8_Release( device ); - } + ok( hr == DI_OK, "got %#lx.\n", hr ); + hr = IDirectInputDevice8_GetProperty( device, DIPROP_JOYSTICKID, &prop_dword.diph ); + ok( hr == DI_OK, "got hr %#lx.\n", hr ); + ok( !prop_dword.dwData, "got %lu.\n", prop_dword.dwData ); + IDirectInputDevice8_Release( device ); } else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6878
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149867 Your paranoid android. === w7u_2qxl (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w7u_adm (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w7u_el (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w8 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w8adm (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w864 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064v1507 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064v1809 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064_tsign (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64_en_AE_u8 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w11pro64 (32 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w7pro64 (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w864 (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064v1507 (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064v1809 (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064_2qxl (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064_adm (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w1064_tsign (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64 (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64_ar (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64_ja (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w10pro64_zh_CN (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w11pro64_amd (64 bit report) === dinput: Fatal: test 'driver_bus' does not exist. === w7u_2qxl (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w7u_adm (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w7u_el (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w8 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w8adm (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w864 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064v1507 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064v1809 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064_tsign (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64_en_AE_u8 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w11pro64 (32 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w7pro64 (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w864 (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064v1507 (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064v1809 (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064_2qxl (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064_adm (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w1064_tsign (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64 (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64_ar (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64_ja (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w10pro64_zh_CN (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === w11pro64_amd (64 bit report) === dinput: Fatal: test 'driver_hid' does not exist. === debian11b (64 bit WoW report) === user32: win.c:4070: Test failed: Expected active window 0000000004580182, got 0000000000000000. win.c:4071: Test failed: Expected focus window 0000000004580182, got 0000000000000000.
Fixes Space Engineers where triggers do not work currently. That is a regression from before dinput redesign. The game assumes that DIPROP_JOYSTICKID can be used as index for XInputGetState. While that is not the case on Windows for a generic case (and the issue is reproducible on Windows with multiple controllers or controllers not supporting xinput) it is the case at least for simple single controller configurations for controller supporting xinput. Before dinput redesign DIPROP_JOYSTICKID was (in the most of cases) assigned sequentially for enumerated controllers. That is sort of similar to what happens on Windows (although it is a bit more complicated on Windows). When disconnecting a controller (or even removing it from bluetooth devices, unsinstalling device from Device Manager) Windows may still remember the controller somewhere and adding another one will use the next index; readding previously removed will reuse previously allocated index. However, as seen with dinput tests with Wine test hid driver, removing the driver "frees" the index and it can be reused. Patch 2 implements behaviour similar in important aspect (having sequential indices which will even match xinput in simple cases). Then, before the redesign dinput supported creating default joystick with GUID_Joystick. This game doesn't depend on that but it looks somewhat connected to indexing and probably worth fixing too. This is a bit not straightforward in a way that even if some controller exists it won't necessarily be created for GUID_Joystick: only the one with DIPROP_JOYSTICKID 0 (which may be absent while higher indices present) will be treated as the default, as test shows. WRT removal of the test in driver_hid.c, my added test was leading to the same test failure on Win11 and Wine. I also checked that without my added test that failing condition in 'else' is not hit anywhere in pre-existing tests; the condition in 'if' already checked a couple of lines above unconditionally. So I think it was best to just remove that check. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_88444
Rémi Bernon (@rbernon) commented about dlls/dinput/joystick_hid.c:
+ +static struct joystick_device *joystick_devices; +static unsigned int joystick_device_count; + +static unsigned int get_joystick_index( const WCHAR *device_path ) +{ + unsigned int i; + + EnterCriticalSection( &joystick_devices_crit ); + for (i = 0; i < joystick_device_count; ++i) + if (!wcsicmp( joystick_devices[i].device_path, device_path )) break; + + if (i == joystick_device_count) + { + ++joystick_device_count; + joystick_devices = realloc( joystick_devices, sizeof(*joystick_devices) * joystick_device_count ); Allocation errors need to be handled here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89139
Rémi Bernon (@rbernon) commented about dlls/dinput/joystick_hid.c:
case (DWORD_PTR)DIPROP_JOYSTICKID: { DIPROPDWORD *value = (DIPROPDWORD *)header; - value->dwData = impl->base.instance.guidInstance.Data3; + value->dwData = get_joystick_index( impl->device_path );
It is unappealing to have an ever-growing buffer and critical section only to support joystick indexes, which IIUC are simply unreliable on Windows. We already support opening joystick by indexes, although these indexes will on hotplug, it seems more interesting to use these instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89140
Rémi Bernon (@rbernon) commented about dlls/dinput/joystick_hid.c:
impl->base.read_event = CreateEventW( NULL, TRUE, FALSE, NULL );
if (memcmp( device_path_guid.Data4, guid->Data4, sizeof(device_path_guid.Data4) )) + { + /* Let hid_joystick_device_open() populate joystick devices before checking for default joystick GUID. */ hr = hid_joystick_device_open( -1, guid, &impl->base.instance, impl->device_path, &impl->device, &impl->preparsed, &attrs, &impl->caps, dinput->dwVersion ); + if (hr == DIERR_DEVICENOTREG && IsEqualGUID( guid, &GUID_Joystick ) + && get_default_joystick_device_path( impl->device_path )) + hr = hid_joystick_device_try_open( impl->device_path, &impl->device, &impl->preparsed, &attrs, + &impl->caps, &impl->base.instance, dinput->dwVersion );
Support for GUID_Joystick could simply be about opening the first device at any point in time. Keeping a cached path as the default device to use, with no way to invalidate it after it's been set, doesn't seem very appealing either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89141
On Thu Nov 28 17:09:50 2024 +0000, Rémi Bernon wrote:
Allocation errors need to be handled here. I guess now the general approach in Wine is that we don't add code to somehow deal with small size not dependent on app data allocation failures, especially when we don't have a very good way out of that except for printing some error, and let it crash instead? Thing is, if such an allocation can't be performed the process is done anyway, adding some formal handling without actually being able to recover only pollutes the code (when it is not something related to working with application data when allocation block can be large or other memory may be freed on allocation error path).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89142
On Thu Nov 28 17:10:31 2024 +0000, Rémi Bernon wrote:
It is unappealing to have an ever-growing buffer and critical section only to support joystick indexes, which IIUC are simply unreliable on Windows. We already support opening joystick by indexes, although these indexes will change on hotplug, still, it would seem more interesting to use these instead if we can. Could you please point me to which opening joystick by index you mean? The only one I can immediately think of is in xinput and there it is based on setupapi device enumeration order and not some intrinsic index.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89143
On Thu Nov 28 17:23:13 2024 +0000, Paul Gofman wrote:
Could you please point me to which opening joystick by index you mean? The only one I can immediately think of is in xinput and there it is based on setupapi device enumeration order and not some intrinsic index. Ah, if you mean the index passed to hid_joystick_device_open(), I think it is no go. It is based on setupapi current device enumeration and may change during lifetime of an open device, yielding opended _JOYSTICKID of open device changing after something else is plugged or unplugged. Or, if we store that id for joystick on creation, opening the same device once again without replugging it may lead a different index.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89144
On Thu Nov 28 17:09:50 2024 +0000, Rémi Bernon wrote:
Support for GUID_Joystick could simply be about opening the first device at any point in time. Keeping a cached path as the default device to use, with no way to invalidate it after it's been set, doesn't seem very appealing either. Yes, I also think for GUID_Joystick these indexing shenanigans are not important, I only made it this way because I already had to implement indexes one or another way and having disconnected default joystick easily achievable on Windows.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89145
On Thu Nov 28 17:27:47 2024 +0000, Paul Gofman wrote:
Ah, if you mean the index passed to hid_joystick_device_open(), I think it is no go. It is based on setupapi current device enumeration and may change during lifetime of an open device, yielding opended _JOYSTICKID of open device changing after something else is plugged or unplugged. Or, if we store that id for joystick on creation, opening the same device once again without replugging it may lead a different index. The only alternative (maybe still better?) that I see so far is to have that index somewhere in hidclass.sys queryable by IOCTL_HID_GET_WINE_JOYSTICK_INDEX (similar to IOCTL_HID_GET_WINE_RAWINPUT_HANDLE), but then this index has to be persisted in some (somewhat similar?) way there too so it is not subject to spurious change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89147
On Thu Nov 28 17:49:00 2024 +0000, Paul Gofman wrote:
The only alternative (maybe still better?) that I see so far is to have that index somewhere in hidclass.sys queryable by IOCTL_HID_GET_WINE_JOYSTICK_INDEX (similar to IOCTL_HID_GET_WINE_RAWINPUT_HANDLE), but then this index has to be persisted in some (somewhat similar?) way there too so it is not subject to spurious change. It doesn't seem to be a problem for joystick ids to change, as far as I can tell Windows does it as well (although it keeps some sort of a cache but given the details below, it also seems quite futile).
With this test: ```c desc.attributes.ProductID = 1; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); hid_device_stop( &desc, 1 ); desc.attributes.ProductID = 2; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); desc.attributes.ProductID = 1; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); hid_device_stop( &desc, 1 ); desc.attributes.ProductID = 2; hid_device_stop( &desc, 1 ); ``` First enumeration shows one device 1 with ID 0. Second enumeration shows one device 2 also with ID 0. Third enumeration, which is done after plugging device 1 back and without unplugging device 2, shows that device 2 ID changes to 1 to avoid collision. It seems to me that using the device enumeration index as joystick ID is just good enough. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89149
On Thu Nov 28 18:29:35 2024 +0000, Rémi Bernon wrote:
It doesn't seem to be a problem for joystick ids to change, as far as I can tell Windows does it as well (although it keeps some sort of a cache but given the details below, it also seems quite futile). With this test: ```c desc.attributes.ProductID = 1; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); hid_device_stop( &desc, 1 ); desc.attributes.ProductID = 2; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); desc.attributes.ProductID = 1; if (!hid_device_start( &desc, 1 )) goto done; hr = IDirectInput8_EnumDevices( di8, DI8DEVCLASS_GAMECTRL, select_default_instance, &d, DIEDFL_ALLDEVICES ); ok( hr == DI_OK, "got hr %#lx.\n", hr ); hid_device_stop( &desc, 1 ); desc.attributes.ProductID = 2; hid_device_stop( &desc, 1 ); ``` First enumeration shows one device 1 with ID 0. Second enumeration shows one device 2 also with ID 0. Third enumeration, which is done after plugging device 1 back and without unplugging device 2, shows that device 2 ID changes to 1 to avoid collision. It seems to me that using the device enumeration index as joystick ID is just good enough. Fwiw, device that were created before the enumerated ID change still keep the ID they had at the time of creation, so if you create a IDirectInputDevice from device 1 after first enumeration, it gets ID 0 whenever you query it (including after it's been unplugged). Similarly, if you create a IDirectInputDevice from device 2 after second enumeration, it returns ID 0 whenever you query it, including after the third enumeration when the enumerated device 2 has changed to ID 1.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89154
On Thu Nov 28 18:45:54 2024 +0000, Rémi Bernon wrote:
Fwiw, device that were created before the enumerated ID change still keep the ID they had at the time of creation, so if you create a IDirectInputDevice from device 1 after first enumeration, it gets ID 0 whenever you query it (including after it's been unplugged). Similarly, if you create a IDirectInputDevice from device 2 after second enumeration, it returns ID 0 whenever you query it, even after the third enumeration when the enumerated device 2 has changed to ID 1, and device 1 gets its ID 0 back (so all the created devices are with ID 0, even though they are targeting different hardware devices). That may work this way (I am still trying to run this test so it doesn't crash) with synthetic driver which just get removed, but if you plug / unplug real controllers they keep indexes, and that what happens in real life with controller replugging outside of synthetic test driver which gets removed completely during the test. Anyway, if you think this doesn't matter I am not up for arguing, that probably doesn't matter for this specific game (mostly because it is utterly broken with multiple controllers anyway, at least if those are not Xbox controllers). The only thing is, hid_joystick_create_device also supports opening device by path, determining the index in this case will need some ugly enumeration but I will figure something out once reproduce these test results locally.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89156
On Thu Nov 28 18:58:25 2024 +0000, Paul Gofman wrote:
That may work this way (I am still trying to run this test so it doesn't crash) with synthetic driver which just get removed, but if you plug / unplug real controllers they keep indexes, and that what happens in real life with controller replugging outside of synthetic test driver which gets removed completely during the test. Anyway, if you think this doesn't matter I am not up for arguing, that probably doesn't matter for this specific game (mostly because it is utterly broken with multiple controllers anyway, at least if those are not Xbox controllers). The only thing is, hid_joystick_create_device also supports opening device by path, determining the index in this case will need some ugly enumeration but I will figure something out once reproduce these test results locally. I am not reproducing the results you describe here. Attach the full test patch (on top of this MR) so it can be compiled and executed (to be sure the same actual code is tested).
I am running it on real Win11 machine with driver signing enforcement disabled in boot options (choosable with reboot with Shift + Restart option in Start menu). What it outputs here is: ``` joystick8.c:6097: Subtest hid joystick8.c:6097: Subtest driver joystick8.c:6097: Subtest driver_bus joystick8.c:6097: Subtest driver_hid joystick8.c:6097: Subtest driver_hid_poll joystick8.c:6022: ----1. joystick8.c:5942: L"Wine Test", id 0, inst {6b2c8080-ad8c-11ef-8001-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0001#2&2c33ed5d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:6030: ----2. joystick8.c:5942: L"Wine Test", id 2, inst {d69a7790-a842-11ef-8004-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0002#2&86033a3&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:6036: ----3. joystick8.c:5942: L"Wine Test", id 0, inst {6b2c8080-ad8c-11ef-8001-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0001#2&2c33ed5d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:5942: L"Wine Test", id 2, inst {d69a7790-a842-11ef-8004-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0002#2&86033a3&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". 0004:driver_hid: 47 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0004:driver_hid: 81 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0004:driver_bus: 59 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 3504:joystick8: 341 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. ``` So the second device is created with id 2 at once, even though device 1 is destroyed (probably index 1 is busy with something else). Maybe your actual test is diffent in some ways, or something in machine setup which makes it always reset indexes, or you are using older Windows and that behaviour changed? FWIW I rebooted the machine before repeating this test to exclude some possible non-persistent leftover devices from previous test run. [test.patch](/uploads/5c1644a9d0527df7659caf5b07ce1d0f/test.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89157
On Thu Nov 28 19:33:21 2024 +0000, Paul Gofman wrote:
I am not reproducing the results you describe here. Attach the full test patch (on top of this MR) so it can be compiled and executed (to be sure the same actual code is tested). I am running it on real Win11 machine with driver signing enforcement disabled in boot options (choosable with reboot with Shift + Restart option in Start menu). What it outputs here is: ``` joystick8.c:6097: Subtest hid joystick8.c:6097: Subtest driver joystick8.c:6097: Subtest driver_bus joystick8.c:6097: Subtest driver_hid joystick8.c:6097: Subtest driver_hid_poll joystick8.c:6022: ----1. joystick8.c:5942: L"Wine Test", id 0, inst {6b2c8080-ad8c-11ef-8001-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0001#2&2c33ed5d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:6030: ----2. joystick8.c:5942: L"Wine Test", id 2, inst {d69a7790-a842-11ef-8004-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0002#2&86033a3&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:6036: ----3. joystick8.c:5942: L"Wine Test", id 0, inst {6b2c8080-ad8c-11ef-8001-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0001#2&2c33ed5d&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". joystick8.c:5942: L"Wine Test", id 2, inst {d69a7790-a842-11ef-8004-444553540000}, path L"\\\\?\\hid#vid_1209&pid_0002#2&86033a3&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}". 0004:driver_hid: 47 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0004:driver_hid: 81 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0004:driver_bus: 59 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 3504:joystick8: 341 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. ``` So the second device is created with id 2 at once, even though device 1 is destroyed (probably index 1 is busy with something else). Maybe your actual test is diffent in some ways, or something in machine setup which makes it always reset indexes, or you are using older Windows and that behaviour changed? FWIW I rebooted the machine before repeating this test to exclude some possible non-persistent leftover devices from previous test run. [test.patch](/uploads/5c1644a9d0527df7659caf5b07ce1d0f/test.patch) On Testbot Win8 and Win10 machines that works like you describe. I am guessing that this was maybe changed in Win11 (as the behaviour with jumping joystick ID without any sort of unplugging of specific device involved, with it keeping the same instance id, looks like a bug), or it doesn't work very well on first device attach / remove on Windows but then it persists something and avoids jumping IDs even on earlier versions.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89158
On Thu Nov 28 20:02:23 2024 +0000, Paul Gofman wrote:
On Testbot Win8 and Win10 machines that works like you describe. I am guessing that this was maybe changed in Win11 (as the behaviour with jumping joystick ID without any sort of unplugging of specific device involved, with it keeping the same instance id, looks like a bug), or it doesn't work very well on first device attach / remove on Windows but then it persists something and avoids jumping IDs even on earlier versions. The ID get cached quickly, they are also cached by VID:PID, not by device path. So you need to change the VID/PID of the virtual device in the tests to trigger the behavior I described again (just incrementing the VID for instance seems enough). I suspect it's probably cached somewhere in the registry, although I have not looked for it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89173
On Thu Nov 28 21:36:31 2024 +0000, Rémi Bernon wrote:
The ID get cached quickly, they are also cached by VID:PID, not by device path. So you need to change the VID/PID of the virtual device in the tests to trigger the behavior I described again (just incrementing the VID for instance seems enough). I suspect it's probably cached somewhere in the registry, although I have not looked for it. To avoid any confusion, could you please attach exactly the test which works differently for you?
Then, if I understand correctly, the behaviour you describe only happens with ever-chaning vid / pid? If that is the case, it is not the most interesting case in practice probably, users don't have the unlimited amount of different controllers, they probably unplug / replug the ones with the same vid / pid. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89174
On Thu Nov 28 21:46:18 2024 +0000, Paul Gofman wrote:
To avoid any confusion, could you please attach exactly the test which works differently for you? Then, if I understand correctly, the behaviour you describe only happens with ever-chaning vid / pid? If that is the case, it is not the most interesting case in practice probably, users don't have the unlimited amount of different controllers, they probably unplug / replug the ones with the same vid / pid. That looks like exactly what I tried to describe as what I think is essential Windows behaviour: yes, on first device plug ever it may take any unused index, but then the id sticks to the index stored somewhere for this device.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89175
On Thu Nov 28 21:54:29 2024 +0000, Paul Gofman wrote:
That looks like exactly what I tried to describe as what I think is essential Windows behaviour: yes, on first device plug ever it may take any unused index, but then the id sticks to the index stored somewhere for this device. I pushed some reworked tests there: https://gitlab.winehq.org/rbernon/wine/-/commits/mr/dinput-test-ids, the ID cache is in the registry. Removing the key resets the cache and makes it work every time. There was some registry cleanup already, but it was only removing the VID/PID used in every test, and assumed there was only going to be one.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89281
On Fri Nov 29 22:19:38 2024 +0000, Rémi Bernon wrote:
I pushed some reworked tests there: https://gitlab.winehq.org/rbernon/wine/-/commits/mr/dinput-test-ids, the ID cache is in the registry. Removing the key resets the cache and makes it work every time. There was some registry cleanup already, but it was only removing the VID/PID used in every test, and assumed there was only going to be one. If we want to test what happens with multiple joysticks with the same VID/PID the test driver would need to be fixed first, I believe it uses VID/PID to uniquely identify devices and I've had VM crashes when trying duplicate devices.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89282
On Fri Nov 29 22:30:56 2024 +0000, Rémi Bernon wrote:
If we want to test what happens with multiple joysticks with the same VID/PID the test driver would need to be fixed first, I believe it uses VID/PID to uniquely identify devices and I've had VM crashes when trying duplicate devices. Oh, interesting, somehow I missed Joistick Id here while found those DirectInput properties keys.
But in a practical sense in terms of implementation, it looks like the case with artificially cleaned up registry is least interesting, in practice that works without cleaning registry keys before any controller replug? We could probably persist that in registry as well, although I am not sure whether it worth the complication, I still suggest to track it locally / in memory. Yes, device path may be not the best choice, I could do it using vid / pid and sequential index accounting for multiple controllers with duplicated vid / pid (and in that case it doesn't matter which index each of those identical controllers get on replug I guess). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89283
On Fri Nov 29 22:33:30 2024 +0000, Paul Gofman wrote:
Oh, interesting, somehow I missed Joistick Id here while found those DirectInput properties keys. But in a practical sense in terms of implementation, it looks like the case with artificially cleaned up registry is least interesting, in practice that works without cleaning registry keys before any controller replug? We could probably persist that in registry as well, although I am not sure whether it worth the complication, I still suggest to track it locally / in memory. Yes, device path may be not the best choice, I could do it using vid / pid and sequential index accounting for multiple controllers with duplicated vid / pid (and in that case it doesn't matter which index each of those identical controllers get on replug I guess). If we need to fix test driver to support identical vid / pid I can look into that (previously I managed to run that with multiple devices of the same vid / pid using `hid_device_start(, 2)` (with some fixes I've included in patches), not yet exactly sure if that is the same as starting separate devices with the same vid pid. I only feel bad about "volatile" ids changing on the fly, that's only happens in specific synthetic test conditions.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6878#note_89284
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon