[PATCH 1/4] winebus.sys: Add a function to check if the device is a Sony DualSense gamepad.

[Arkadiusz Hiler]

Signed-off-by: Arkadiusz Hiler 
---
 dlls/winebus.sys/unix_private.h | 1 +
 dlls/winebus.sys/unixlib.c      | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/dlls/winebus.sys/unix_private.h b/dlls/winebus.sys/unix_private.h
index efecf6cdbe3..e897251dea8 100644
--- a/dlls/winebus.sys/unix_private.h
+++ b/dlls/winebus.sys/unix_private.h
@@ -266,5 +266,6 @@ extern void hid_device_set_effect_state(struct unix_device *iface, BYTE index, B

BOOL is_xbox_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;
 BOOL is_dualshock4_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;
+BOOL is_dualsense_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;

#endif /* __WINEBUS_UNIX_PRIVATE_H */
diff --git a/dlls/winebus.sys/unixlib.c b/dlls/winebus.sys/unixlib.c
index 1269ae05c2b..3a7c3a16428 100644
--- a/dlls/winebus.sys/unixlib.c
+++ b/dlls/winebus.sys/unixlib.c
@@ -70,6 +70,12 @@ BOOL is_dualshock4_gamepad(WORD vid, WORD pid)
     return FALSE;
 }

+BOOL is_dualsense_gamepad(WORD vid, WORD pid)
+{
+    if (vid == 0x054c && pid == 0x0ce6) return TRUE;
+    return FALSE;
+}
+
 struct mouse_device
 {
     struct unix_device unix_device;

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Arkadiusz Hiler]

Signed-off-by: Arkadiusz Hiler 
---
 dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 2269f5b904e..c290cea6f4f 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
 }

#define QUIRK_DS4_BT 0x1
+#define QUIRK_DUALSENSE_BT 0x2

struct hidraw_device
 {
@@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
             buff[0] = 1;
         }

+        /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
+         *
+         * The report number #41 is used for the extended bluetooth input report. The report comes
+         * with only one extra byte in front and the format is not exactly the same and we need to
+         * shuffle a few bytes around.
+         *
+         * Small report:
+         *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
+         *
+         * Extended report:
+         *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
+         */
+        if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
+        {
+            BYTE tmp;
+            size = 10;
+            buff += 1;
+
+            buff[0] = 1; /* fake report #1 */
+
+            tmp = buff[6];      /* Trigger Right */
+            buff[5] = buff[8];  /* Buttons[0] */
+            buff[6] = buff[9];  /* Buttons[1] */
+            buff[7] = buff[10]; /* Buttons[2] */
+            buff[8] = buff[5];  /* TriggerLeft */
+            buff[9] = tmp;      /* TirggerRight */
+        }
+
         bus_event_queue_input_report(&event_queue, iface, buff, size);
     }
 }
@@ -1201,6 +1230,9 @@ static void hidraw_set_quirks(struct hidraw_device *impl, DWORD bus_type, WORD v
 {
     if (bus_type == BUS_BLUETOOTH && is_dualshock4_gamepad(vid, pid))
         impl->quirks |= QUIRK_DS4_BT;
+
+    if (bus_type == BUS_BLUETOOTH && is_dualsense_gamepad(vid, pid))
+        impl->quirks |= QUIRK_DUALSENSE_BT;
 }

static void udev_add_device(struct udev_device *dev, int fd)

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Rémi Bernon]

Hi Arek,

On 1/21/22 17:10, Arkadiusz Hiler wrote:
> Signed-off-by: Arkadiusz Hiler
>
> dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 2269f5b904e..c290cea6f4f 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
>   }
>
> #define QUIRK_DS4_BT 0x1
> +#define QUIRK_DUALSENSE_BT 0x2
>
> struct hidraw_device
>   {
> @@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
>               buff[0] = 1;
>           }
> -
>    /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
> -
>     *
> -
>     * The report number #41 is used for the extended bluetooth input report. The report comes
> -
>     * with only one extra byte in front and the format is not exactly the same and we need to
> -
>     * shuffle a few bytes around.
> -
>     *
> -
>     * Small report:
> -
>     *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
> -
>     *
> -
>     * Extended report:
> -
>     *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
> -
>     */
> -
>    if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
> -
>    {
> -
>        BYTE tmp;
> -
>        size = 10;
> -
>        buff += 1;
> -
> -
>        buff[0] = 1; /* fake report #1 */
> -
> -
>        tmp = buff[6];      /* Trigger Right */
> -
>        buff[5] = buff[8];  /* Buttons[0] */
> -
>        buff[6] = buff[9];  /* Buttons[1] */
> -
>        buff[7] = buff[10]; /* Buttons[2] */
> -
>        buff[8] = buff[5];  /* TriggerLeft */
> -
>        buff[9] = tmp;      /* TirggerRight */
> -
>    }

You're writing to buff[5] and then reading it, probably you need to save 
both triggers first.

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Roderick Colenbrander]

On Fri, Jan 21, 2022 at 2:53 PM Rémi Bernon  wrote:
> Hi Arek,
>
> On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > Signed-off-by: Arkadiusz Hiler
> >
> > dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> >
> > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> > index 2269f5b904e..c290cea6f4f 100644
> > --- a/dlls/winebus.sys/bus_udev.c
> > +++ b/dlls/winebus.sys/bus_udev.c
> > @@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
> >   }
> >
> > #define QUIRK_DS4_BT 0x1
> > +#define QUIRK_DUALSENSE_BT 0x2
> >
> > struct hidraw_device
> >   {
> > @@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
> >               buff[0] = 1;
> >           }
> > -
> >    /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
> > -
> >     *
> > -
> >     * The report number #41 is used for the extended bluetooth input report. The report comes
> > -
> >     * with only one extra byte in front and the format is not exactly the same and we need to
> > -
> >     * shuffle a few bytes around.
> > -
> >     *
> > -
> >     * Small report:
> > -
> >     *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
> > -
> >     *
> > -
> >     * Extended report:
> > -
> >     *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
> > -
> >     */
> > -
> >    if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
> > -
> >    {
> > -
> >        BYTE tmp;
> > -
> >        size = 10;
> > -
> >        buff += 1;
> > -
> > -
> >        buff[0] = 1; /* fake report #1 */
> > -
> > -
> >        tmp = buff[6];      /* Trigger Right */
> > -
> >        buff[5] = buff[8];  /* Buttons[0] */
> > -
> >        buff[6] = buff[9];  /* Buttons[1] */
> > -
> >        buff[7] = buff[10]; /* Buttons[2] */
> > -
> >        buff[8] = buff[5];  /* TriggerLeft */
> > -
> >        buff[9] = tmp;      /* TirggerRight */
> > -
> >    }
>
> You're writing to buff[5] and then reading it, probably you need to save
> both triggers first.

Hi Arkadiusz,

I'm not a big fan of this type of fixup here (and the same for DS4 to
be honest). The DS4/DualSense have unusual HID reports, but this type
of fixup causes issues for applications which properly parse the HID
data. For example various applications for example do use our
libScePad.dll for games on Windows. They go through the Windows HID
APIs. This breaks such applications.

Thanks,

Roderick Colenbrander
Sony Interactive Entertainment, LLC

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Rémi Bernon]

On 1/22/22 21:01, Roderick Colenbrander wrote:
> On Fri, Jan 21, 2022 at 2:53 PM Rémi Bernon  wrote:
> > Hi Arek,
> >
> > On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > > Signed-off-by: Arkadiusz Hiler
> > >
> > > dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
> > >    1 file changed, 32 insertions(+)
> > >
> > > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> > > index 2269f5b904e..c290cea6f4f 100644
> > > --- a/dlls/winebus.sys/bus_udev.c
> > > +++ b/dlls/winebus.sys/bus_udev.c
> > > @@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
> > >    }
> > >
> > > #define QUIRK_DS4_BT 0x1
> > > +#define QUIRK_DUALSENSE_BT 0x2
> > >
> > > struct hidraw_device
> > >    {
> > > @@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
> > >                buff[0] = 1;
> > >            }
> > > -
> > >    /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
> > > -
> > >     *
> > > -
> > >     * The report number #41 is used for the extended bluetooth input report. The report comes
> > > -
> > >     * with only one extra byte in front and the format is not exactly the same and we need to
> > > -
> > >     * shuffle a few bytes around.
> > > -
> > >     *
> > > -
> > >     * Small report:
> > > -
> > >     *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
> > > -
> > >     *
> > > -
> > >     * Extended report:
> > > -
> > >     *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
> > > -
> > >     */
> > > -
> > >    if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
> > > -
> > >    {
> > > -
> > >        BYTE tmp;
> > > -
> > >        size = 10;
> > > -
> > >        buff += 1;
> > > -
> > > -
> > >        buff[0] = 1; /* fake report #1 */
> > > -
> > > -
> > >        tmp = buff[6];      /* Trigger Right */
> > > -
> > >        buff[5] = buff[8];  /* Buttons[0] */
> > > -
> > >        buff[6] = buff[9];  /* Buttons[1] */
> > > -
> > >        buff[7] = buff[10]; /* Buttons[2] */
> > > -
> > >        buff[8] = buff[5];  /* TriggerLeft */
> > > -
> > >        buff[9] = tmp;      /* TirggerRight */
> > > -
> > >    }
> >
> > You're writing to buff[5] and then reading it, probably you need to save
> > both triggers first.
>
> Hi Arkadiusz,
>
> I'm not a big fan of this type of fixup here (and the same for DS4 to
> be honest). The DS4/DualSense have unusual HID reports, but this type
> of fixup causes issues for applications which properly parse the HID
> data. For example various applications for example do use our
> libScePad.dll for games on Windows. They go through the Windows HID
> APIs. This breaks such applications.
>
> Thanks,
>
> Roderick Colenbrander
> Sony Interactive Entertainment, LLC

Hi Roderick,

Thanks for giving some feedback. The fixups have been added for games 
and applications which do not expect to receive the extended reports, 
and only support the reports that are openly described in the HID 
descriptors.

As Arek has found already, and as you say, some other applications or 
middlewares are explicitly requesting them, so we were considering 
adding some more logic to remove the fixups and send the extended 
reports in the same way as on Windows, when they are requested.

I understand that these extended reports are triggered by some feature 
report requests, and this is automatically done on Linux by the kernel 
and evdev driver, to the contrary to what happens on Windows, or maybe 
there's something we've missed?

Cheers,

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Arkadiusz Hiler]

On Mon, Jan 24, 2022 at 12:48:21AM +0100, Rémi Bernon wrote:
> On 1/22/22 21:01, Roderick Colenbrander wrote:
> > Hi Arkadiusz,
> >
> > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > be honest). The DS4/DualSense have unusual HID reports, but this type
> > of fixup causes issues for applications which properly parse the HID
> > data. For example various applications for example do use our
> > libScePad.dll for games on Windows. They go through the Windows HID
> > APIs. This breaks such applications.
> >
> > Thanks,
> >
> > Roderick Colenbrander
> > Sony Interactive Entertainment, LLC
>
> Hi Roderick,
>
> Thanks for giving some feedback. The fixups have been added for games and
> applications which do not expect to receive the extended reports, and only
> support the reports that are openly described in the HID descriptors.
>
> As Arek has found already, and as you say, some other applications or
> middlewares are explicitly requesting them, so we were considering adding
> some more logic to remove the fixups and send the extended reports in the
> same way as on Windows, when they are requested.
>
> I understand that these extended reports are triggered by some feature
> report requests, and this is automatically done on Linux by the kernel and
> evdev driver, to the contrary to what happens on Windows, or maybe there's
> something we've missed?

Hi folks,

As Rémi has mentioned this is only to "undo" the mode switch that
hid_playstation/hid_sony does on Linux for Sony controllers when they
are used over Bluetooth.

On Windows those use the basic reports (#1) by default and require
getting calibration feature report (#5) to switch modes.

The next patch in this series handles the mode switch:
https://source.winehq.org/patches/data/224003

That's at least how SDL does that. If there are more ways to trigger
this please let us know so we can support it.

Also after switching the modes the controller no longer works through
DirectInput and requires to be reconnected, as it seems to be a one way
transition.

I've found a fairly new game that comes with libScePad.dll - God of War.
The game does handle DualSense and DualShock 4 over USB only. When those
contollers are connected over Bluetooth the game does not detect them.

If you have any suggestions for a game that I could use for testing the
Bluetooth extended mode switching please let me know.

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Roderick Colenbrander]

On Mon, Jan 24, 2022 at 2:25 AM Arkadiusz Hiler  wrote:
> On Mon, Jan 24, 2022 at 12:48:21AM +0100, Rémi Bernon wrote:
> > On 1/22/22 21:01, Roderick Colenbrander wrote:
> > > Hi Arkadiusz,
> > >
> > > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > > be honest). The DS4/DualSense have unusual HID reports, but this type
> > > of fixup causes issues for applications which properly parse the HID
> > > data. For example various applications for example do use our
> > > libScePad.dll for games on Windows. They go through the Windows HID
> > > APIs. This breaks such applications.
> > >
> > > Thanks,
> > >
> > > Roderick Colenbrander
> > > Sony Interactive Entertainment, LLC
> >
> > Hi Roderick,
> >
> > Thanks for giving some feedback. The fixups have been added for games and
> > applications which do not expect to receive the extended reports, and only
> > support the reports that are openly described in the HID descriptors.
> >
> > As Arek has found already, and as you say, some other applications or
> > middlewares are explicitly requesting them, so we were considering adding
> > some more logic to remove the fixups and send the extended reports in the
> > same way as on Windows, when they are requested.
> >
> > I understand that these extended reports are triggered by some feature
> > report requests, and this is automatically done on Linux by the kernel and
> > evdev driver, to the contrary to what happens on Windows, or maybe there's
> > something we've missed?
>
> Hi folks,
>
> As Rémi has mentioned this is only to "undo" the mode switch that
> hid_playstation/hid_sony does on Linux for Sony controllers when they
> are used over Bluetooth.
>
> On Windows those use the basic reports (#1) by default and require
> getting calibration feature report (#5) to switch modes.
>
> The next patch in this series handles the mode switch:
> https://source.winehq.org/patches/data/224003
>
> That's at least how SDL does that. If there are more ways to trigger
> this please let us know so we can support it.

I believe feature report 5 is the main way.
> Also after switching the modes the controller no longer works through
> DirectInput and requires to be reconnected, as it seems to be a one way
> transition.
>
> I've found a fairly new game that comes with libScePad.dll - God of War.
> The game does handle DualSense and DualShock 4 over USB only. When those
> contollers are connected over Bluetooth the game does not detect them.

Correct, libScePad.dll doesn't handle Bluetooth devices and I'm not
sure why that decision was made (on older Windows version there
inconsistent Bluetooth stacks, so may have been related).
> If you have any suggestions for a game that I could use for testing the
> Bluetooth extended mode switching please let me know.

Unfortunately I don't know other games by head as I'm not part of the
Windows / game support teams. A good starting point would probably be
Sony games e.g. Horizon. There have been some other published as well.
> --
> Cheers,
> Arek

Overall I'm not sure what the best thing to do is. As I shared in the
other email various other applications (e.g. Chrome) do handle the
extended report data properly and would thus break.

One method, but also not ideal at all. Would be to override the device
descriptors and report it as a USB device. However that would mean
spoofing various other reports as well and would make more like
hid-sony/hid-playstation. It would be the most compatible way and not
break other applications.

I really hate HID so bad it is such a poor abstraction and since every
application accesses the device directly any state is lost (e.g. for
our devices you can change power management, reporting speed, light
colors and more).

Thanks,
Roderick

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Arkadiusz Hiler]

On Mon, Jan 24, 2022 at 08:37:14AM -0800, Roderick Colenbrander wrote:
> On Mon, Jan 24, 2022 at 2:25 AM Arkadiusz Hiler  wrote:
> > On Mon, Jan 24, 2022 at 12:48:21AM +0100, Rémi Bernon wrote:
> > > On 1/22/22 21:01, Roderick Colenbrander wrote:
> > > > Hi Arkadiusz,
> > > >
> > > > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > > > be honest). The DS4/DualSense have unusual HID reports, but this type
> > > > of fixup causes issues for applications which properly parse the HID
> > > > data. For example various applications for example do use our
> > > > libScePad.dll for games on Windows. They go through the Windows HID
> > > > APIs. This breaks such applications.
> > > >
> > > > Thanks,
> > > >
> > > > Roderick Colenbrander
> > > > Sony Interactive Entertainment, LLC
> > >
> > > Hi Roderick,
> > >
> > > Thanks for giving some feedback. The fixups have been added for games and
> > > applications which do not expect to receive the extended reports, and only
> > > support the reports that are openly described in the HID descriptors.
> > >
> > > As Arek has found already, and as you say, some other applications or
> > > middlewares are explicitly requesting them, so we were considering adding
> > > some more logic to remove the fixups and send the extended reports in the
> > > same way as on Windows, when they are requested.
> > >
> > > I understand that these extended reports are triggered by some feature
> > > report requests, and this is automatically done on Linux by the kernel and
> > > evdev driver, to the contrary to what happens on Windows, or maybe there's
> > > something we've missed?
> >
> > Hi folks,
> >
> > As Rémi has mentioned this is only to "undo" the mode switch that
> > hid_playstation/hid_sony does on Linux for Sony controllers when they
> > are used over Bluetooth.
> >
> > On Windows those use the basic reports (#1) by default and require
> > getting calibration feature report (#5) to switch modes.
> >
> > The next patch in this series handles the mode switch:
> > https://source.winehq.org/patches/data/224003
> >
> > That's at least how SDL does that. If there are more ways to trigger
> > this please let us know so we can support it.
>
> I believe feature report 5 is the main way.

Good to know.
> > Also after switching the modes the controller no longer works through
> > DirectInput and requires to be reconnected, as it seems to be a one way
> > transition.
> >
> > I've found a fairly new game that comes with libScePad.dll - God of War.
> > The game does handle DualSense and DualShock 4 over USB only. When those
> > contollers are connected over Bluetooth the game does not detect them.
>
> Correct, libScePad.dll doesn't handle Bluetooth devices and I'm not
> sure why that decision was made (on older Windows version there
> inconsistent Bluetooth stacks, so may have been related).

Then it won't support the controllers over Bluetooth whether we apply
the quirk or not.
> > If you have any suggestions for a game that I could use for testing the
> > Bluetooth extended mode switching please let me know.
>
> Unfortunately I don't know other games by head as I'm not part of the
> Windows / game support teams. A good starting point would probably be
> Sony games e.g. Horizon. There have been some other published as well.

I can check HZD, but I doubt that it will support Bluetooth if the newer
GoW doesn't. It take a while to download though.
> > --
> > Cheers,
> > Arek
>
> Overall I'm not sure what the best thing to do is. As I shared in the
> other email various other applications (e.g. Chrome) do handle the
> extended report data properly and would thus break.

Why would they break? I've just tested a few things on Windows and
Chrome works with DualShock 4 over Bluetooth in both basic and enhanced
modes.

Also Chrome doesn't switch the controller mode by itself, it's just fine
if something else switches the mode for it.

Switching mode to extended on Windows makes the DualShock 4 unusable
through DirectInput though which breaks *a lot* of games. This is also
the case with our dinput HID backend in Wine.
> One method, but also not ideal at all. Would be to override the device
> descriptors and report it as a USB device. However that would mean
> spoofing various other reports as well and would make more like
> hid-sony/hid-playstation. It would be the most compatible way and not
> break other applications.

I still don't understand what's bad about having the quirk and disabling
the it on feture report #5? This will make the controller behave just
like it does on Windows - if application wants the extended reports and
can understand them it can still request them.
> I really hate HID so bad it is such a poor abstraction and since every
> application accesses the device directly any state is lost (e.g. for
> our devices you can change power management, reporting speed, light
> colors and more).

I share the sentiment but at least it somewhat works across various
platoforms with no need for dedicated drivers.

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Rémi Bernon]

On 1/24/22 17:37, Roderick Colenbrander wrote:
> On Mon, Jan 24, 2022 at 2:25 AM Arkadiusz Hiler  wrote:
> > On Mon, Jan 24, 2022 at 12:48:21AM +0100, Rémi Bernon wrote:
> > > On 1/22/22 21:01, Roderick Colenbrander wrote:
> > > > Hi Arkadiusz,
> > > >
> > > > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > > > be honest). The DS4/DualSense have unusual HID reports, but this type
> > > > of fixup causes issues for applications which properly parse the HID
> > > > data. For example various applications for example do use our
> > > > libScePad.dll for games on Windows. They go through the Windows HID
> > > > APIs. This breaks such applications.
> > > >
> > > > Thanks,
> > > >
> > > > Roderick Colenbrander
> > > > Sony Interactive Entertainment, LLC
> > >
> > > Hi Roderick,
> > >
> > > Thanks for giving some feedback. The fixups have been added for games and
> > > applications which do not expect to receive the extended reports, and only
> > > support the reports that are openly described in the HID descriptors.
> > >
> > > As Arek has found already, and as you say, some other applications or
> > > middlewares are explicitly requesting them, so we were considering adding
> > > some more logic to remove the fixups and send the extended reports in the
> > > same way as on Windows, when they are requested.
> > >
> > > I understand that these extended reports are triggered by some feature
> > > report requests, and this is automatically done on Linux by the kernel and
> > > evdev driver, to the contrary to what happens on Windows, or maybe there's
> > > something we've missed?
> >
> > Hi folks,
> >
> > As Rémi has mentioned this is only to "undo" the mode switch that
> > hid_playstation/hid_sony does on Linux for Sony controllers when they
> > are used over Bluetooth.
> >
> > On Windows those use the basic reports (#1) by default and require
> > getting calibration feature report (#5) to switch modes.
> >
> > The next patch in this series handles the mode switch:
> > https://source.winehq.org/patches/data/224003
> >
> > That's at least how SDL does that. If there are more ways to trigger
> > this please let us know so we can support it.
>
> I believe feature report 5 is the main way.
> > Also after switching the modes the controller no longer works through
> > DirectInput and requires to be reconnected, as it seems to be a one way
> > transition.
> >
> > I've found a fairly new game that comes with libScePad.dll - God of War.
> > The game does handle DualSense and DualShock 4 over USB only. When those
> > contollers are connected over Bluetooth the game does not detect them.
>
> Correct, libScePad.dll doesn't handle Bluetooth devices and I'm not
> sure why that decision was made (on older Windows version there
> inconsistent Bluetooth stacks, so may have been related).
> > If you have any suggestions for a game that I could use for testing the
> > Bluetooth extended mode switching please let me know.
>
> Unfortunately I don't know other games by head as I'm not part of the
> Windows / game support teams. A good starting point would probably be
> Sony games e.g. Horizon. There have been some other published as well.
> > --
> > Cheers,
> > Arek
>
> Overall I'm not sure what the best thing to do is. As I shared in the
> other email various other applications (e.g. Chrome) do handle the
> extended report data properly and would thus break.
>
> One method, but also not ideal at all. Would be to override the device
> descriptors and report it as a USB device. However that would mean
> spoofing various other reports as well and would make more like
> hid-sony/hid-playstation. It would be the most compatible way and not
> break other applications.

Well that's mostly what we are doing here, although we don't change the 
descriptor, and we fixup the minimum amount of data by using and filling 
the default input report which is already described, dropping all the 
extended data.

We have other backends (SDL / evdev) which are actually doing what you 
suggest, and which expose their own HID descriptor with the data they 
get from the backend, so there too only basic gamepad data. That's no 
different imho, and it would not match the extended reports either, and 
so not provide the full feature to Chrome or other middleware which 
would be expecting it.

Even worse, some games or applications are hardcoding the HID reports 
from the DS4 and other controllers and expect them, when they detect the 
controller VID/PID, to exactly match what the device is usually sending 
on Windows, whether it is the basic or the extended reports that depends 
on what they support.

Crafting our own reports here, like we do with SDL or evdev backends, 
completely break these games. That's why Proton is using hidraw as a 
default instead, and then why we need to do these fixups.

Faking VID/PID otoh isn't great either, as we won't get the expected 
controller glyphs in game anymore, and they would display some generic 
ones, or even worse again, not support the controller at all if they 
hardcode a few VID/PID.

Last, not doing any kind of fixup will just make the DS4 completely 
unusable in Wine with the hidraw backend, as Steam (I suspect it comes 
from it most of the time) is already requesting calibration and thus 
enabling the extended input reports by default. We certainly don't want 
to have vendor-specific parsing logic in DInput.
> I really hate HID so bad it is such a poor abstraction and since every
> application accesses the device directly any state is lost (e.g. for
> our devices you can change power management, reporting speed, light
> colors and more).

Sure, it's maybe hard to describe advanced data, but imho for the basic 
things it just works fine, and it's actually supposed to do so in order 
for application not to have to hardcode the packet structures. Not 
having anything described just makes third-party and application 
hardcode these structures again and lose all the flexibility HID could 
have provided.

Overall, and trying to move forward on this I'd like to better 
understand what you are worried about exactly. Our intention here is to 
mimic Windows behavior as close as possible, regardless of what the 
Linux environment is doing around, so we intended to:

1) Have the basic input reports sent by default, translated from the 
extended ones if needed, and until they are supposed to be sent, from 
the Windows world point of view.

2) Watch and support whatever mechanism is supposed to trigger the 
extended input reports and pass them through after that point.

Are you worried that 2) isn't well defined and that it would be hard to 
decide whether the extended input reports we receive are actually 
supposed to be passed through or not?

In that case instead of trying to figure which report numbers matter 
here, we could just assume that any kind of output or feature reports 
seen for these devices are potentially supposed to trigger the extended 
reports, would that be better?

Cheers,

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Roderick Colenbrander]

On Mon, Jan 24, 2022 at 10:49 AM Rémi Bernon  wrote:
> On 1/24/22 17:37, Roderick Colenbrander wrote:
> > On Mon, Jan 24, 2022 at 2:25 AM Arkadiusz Hiler  wrote:
> > > On Mon, Jan 24, 2022 at 12:48:21AM +0100, Rémi Bernon wrote:
> > > > On 1/22/22 21:01, Roderick Colenbrander wrote:
> > > > > Hi Arkadiusz,
> > > > >
> > > > > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > > > > be honest). The DS4/DualSense have unusual HID reports, but this type
> > > > > of fixup causes issues for applications which properly parse the HID
> > > > > data. For example various applications for example do use our
> > > > > libScePad.dll for games on Windows. They go through the Windows HID
> > > > > APIs. This breaks such applications.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Roderick Colenbrander
> > > > > Sony Interactive Entertainment, LLC
> > > >
> > > > Hi Roderick,
> > > >
> > > > Thanks for giving some feedback. The fixups have been added for games and
> > > > applications which do not expect to receive the extended reports, and only
> > > > support the reports that are openly described in the HID descriptors.
> > > >
> > > > As Arek has found already, and as you say, some other applications or
> > > > middlewares are explicitly requesting them, so we were considering adding
> > > > some more logic to remove the fixups and send the extended reports in the
> > > > same way as on Windows, when they are requested.
> > > >
> > > > I understand that these extended reports are triggered by some feature
> > > > report requests, and this is automatically done on Linux by the kernel and
> > > > evdev driver, to the contrary to what happens on Windows, or maybe there's
> > > > something we've missed?
> > >
> > > Hi folks,
> > >
> > > As Rémi has mentioned this is only to "undo" the mode switch that
> > > hid_playstation/hid_sony does on Linux for Sony controllers when they
> > > are used over Bluetooth.
> > >
> > > On Windows those use the basic reports (#1) by default and require
> > > getting calibration feature report (#5) to switch modes.
> > >
> > > The next patch in this series handles the mode switch:
> > > https://source.winehq.org/patches/data/224003
> > >
> > > That's at least how SDL does that. If there are more ways to trigger
> > > this please let us know so we can support it.
> >
> > I believe feature report 5 is the main way.
> > > Also after switching the modes the controller no longer works through
> > > DirectInput and requires to be reconnected, as it seems to be a one way
> > > transition.
> > >
> > > I've found a fairly new game that comes with libScePad.dll - God of War.
> > > The game does handle DualSense and DualShock 4 over USB only. When those
> > > contollers are connected over Bluetooth the game does not detect them.
> >
> > Correct, libScePad.dll doesn't handle Bluetooth devices and I'm not
> > sure why that decision was made (on older Windows version there
> > inconsistent Bluetooth stacks, so may have been related).
> > > If you have any suggestions for a game that I could use for testing the
> > > Bluetooth extended mode switching please let me know.
> >
> > Unfortunately I don't know other games by head as I'm not part of the
> > Windows / game support teams. A good starting point would probably be
> > Sony games e.g. Horizon. There have been some other published as well.
> > > --
> > > Cheers,
> > > Arek
> >
> > Overall I'm not sure what the best thing to do is. As I shared in the
> > other email various other applications (e.g. Chrome) do handle the
> > extended report data properly and would thus break.
> >
> > One method, but also not ideal at all. Would be to override the device
> > descriptors and report it as a USB device. However that would mean
> > spoofing various other reports as well and would make more like
> > hid-sony/hid-playstation. It would be the most compatible way and not
> > break other applications.
>
> Well that's mostly what we are doing here, although we don't change the
> descriptor, and we fixup the minimum amount of data by using and filling
> the default input report which is already described, dropping all the
> extended data.
>
> We have other backends (SDL / evdev) which are actually doing what you
> suggest, and which expose their own HID descriptor with the data they
> get from the backend, so there too only basic gamepad data. That's no
> different imho, and it would not match the extended reports either, and
> so not provide the full feature to Chrome or other middleware which
> would be expecting it.
>
> Even worse, some games or applications are hardcoding the HID reports
> from the DS4 and other controllers and expect them, when they detect the
> controller VID/PID, to exactly match what the device is usually sending
> on Windows, whether it is the basic or the extended reports that depends
> on what they support.
>
> Crafting our own reports here, like we do with SDL or evdev backends,
> completely break these games. That's why Proton is using hidraw as a
> default instead, and then why we need to do these fixups.
>
> Faking VID/PID otoh isn't great either, as we won't get the expected
> controller glyphs in game anymore, and they would display some generic
> ones, or even worse again, not support the controller at all if they
> hardcode a few VID/PID.
>
> Last, not doing any kind of fixup will just make the DS4 completely
> unusable in Wine with the hidraw backend, as Steam (I suspect it comes
> from it most of the time) is already requesting calibration and thus
> enabling the extended input reports by default. We certainly don't want
> to have vendor-specific parsing logic in DInput.
> > I really hate HID so bad it is such a poor abstraction and since every
> > application accesses the device directly any state is lost (e.g. for
> > our devices you can change power management, reporting speed, light
> > colors and more).
>
> Sure, it's maybe hard to describe advanced data, but imho for the basic
> things it just works fine, and it's actually supposed to do so in order
> for application not to have to hardcode the packet structures. Not
> having anything described just makes third-party and application
> hardcode these structures again and lose all the flexibility HID could
> have provided.
>
> Overall, and trying to move forward on this I'd like to better
> understand what you are worried about exactly. Our intention here is to
> mimic Windows behavior as close as possible, regardless of what the
> Linux environment is doing around, so we intended to:
> - Have the basic input reports sent by default, translated from the
>
> extended ones if needed, and until they are supposed to be sent, from
> the Windows world point of view.
> - Watch and support whatever mechanism is supposed to trigger the
>
> extended input reports and pass them through after that point.
>
> Are you worried that 2) isn't well defined and that it would be hard to
> decide whether the extended input reports we receive are actually
> supposed to be passed through or not?
>
> In that case instead of trying to figure which report numbers matter
> here, we could just assume that any kind of output or feature reports
> seen for these devices are potentially supposed to trigger the extended
> reports, would that be better?
> Cheers,
>
> Rémi Bernon

I have been thinking about it a bit more. Maybe the hack if paired
with the feature report 5 filter is not too bad.

Though be aware that this fixup would also need to be done probably
for other platforms e.g. Mac. I do not know what applications all
support DS4/DualSense there. I assume at least SIE's RemotePlay app,
but probably other applications as well. I do not know what level of
support there is in native OSX, but there might be some (as Apple did
implement a lot of code for our controllers in iOS).

Thanks,
Roderick

[PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Roderick Colenbrander]

On Sun, Jan 23, 2022 at 3:48 PM Rémi Bernon  wrote:
> On 1/22/22 21:01, Roderick Colenbrander wrote:
> > On Fri, Jan 21, 2022 at 2:53 PM Rémi Bernon  wrote:
> > > Hi Arek,
> > >
> > > On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > > > Signed-off-by: Arkadiusz Hiler
> > > >
> > > > dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
> > > >    1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> > > > index 2269f5b904e..c290cea6f4f 100644
> > > > --- a/dlls/winebus.sys/bus_udev.c
> > > > +++ b/dlls/winebus.sys/bus_udev.c
> > > > @@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
> > > >    }
> > > >
> > > > #define QUIRK_DS4_BT 0x1
> > > > +#define QUIRK_DUALSENSE_BT 0x2
> > > >
> > > > struct hidraw_device
> > > >    {
> > > > @@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
> > > >                buff[0] = 1;
> > > >            }
> > > > -
> > > >    /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
> > > > -
> > > >     *
> > > > -
> > > >     * The report number #41 is used for the extended bluetooth input report. The report comes
> > > > -
> > > >     * with only one extra byte in front and the format is not exactly the same and we need to
> > > > -
> > > >     * shuffle a few bytes around.
> > > > -
> > > >     *
> > > > -
> > > >     * Small report:
> > > > -
> > > >     *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
> > > > -
> > > >     *
> > > > -
> > > >     * Extended report:
> > > > -
> > > >     *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
> > > > -
> > > >     */
> > > > -
> > > >    if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
> > > > -
> > > >    {
> > > > -
> > > >        BYTE tmp;
> > > > -
> > > >        size = 10;
> > > > -
> > > >        buff += 1;
> > > > -
> > > > -
> > > >        buff[0] = 1; /* fake report #1 */
> > > > -
> > > > -
> > > >        tmp = buff[6];      /* Trigger Right */
> > > > -
> > > >        buff[5] = buff[8];  /* Buttons[0] */
> > > > -
> > > >        buff[6] = buff[9];  /* Buttons[1] */
> > > > -
> > > >        buff[7] = buff[10]; /* Buttons[2] */
> > > > -
> > > >        buff[8] = buff[5];  /* TriggerLeft */
> > > > -
> > > >        buff[9] = tmp;      /* TirggerRight */
> > > > -
> > > >    }
> > >
> > > You're writing to buff[5] and then reading it, probably you need to save
> > > both triggers first.
> >
> > Hi Arkadiusz,
> >
> > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > be honest). The DS4/DualSense have unusual HID reports, but this type
> > of fixup causes issues for applications which properly parse the HID
> > data. For example various applications for example do use our
> > libScePad.dll for games on Windows. They go through the Windows HID
> > APIs. This breaks such applications.
> >
> > Thanks,
> >
> > Roderick Colenbrander
> > Sony Interactive Entertainment, LLC
>
> Hi Roderick,
>
> Thanks for giving some feedback. The fixups have been added for games
> and applications which do not expect to receive the extended reports,
> and only support the reports that are openly described in the HID
> descriptors.
>
> As Arek has found already, and as you say, some other applications or
> middlewares are explicitly requesting them, so we were considering
> adding some more logic to remove the fixups and send the extended
> reports in the same way as on Windows, when they are requested.
>
> I understand that these extended reports are triggered by some feature
> report requests, and this is automatically done on Linux by the kernel
> and evdev driver, to the contrary to what happens on Windows, or maybe
> there's something we've missed?

The DS4 / DualSense in Bluetooth can be switched to a different mode
in which they also report motion sensors, touchpad and other data. In
the default mode (not sure why it exists to be honest) it sends limit
data and in case of DS4 is actually not that responsive.

The calibration operations switches the device to this other mode.
Various applications including Chrome have their own HID parsing for
our devices (https://chromium.googlesource.com/chromium/src/+/refs/heads/main/device/game...).

Doing this type of fixup is not really the right thing for either
controller. It fixes some applications, while breaks others (e.g.
Chrome would break) as apps really need this extended data. Also other
data in the report would need transformation as it would be at the
wrong offsets.

Let me reply more to the email to Arek.
> Cheers,
>
> Rémi Bernon

Thanks,
Roderick

[PATCH 3/4] winebus.sys: Enable extended BT reports for Sony controllers when requested.

[Arkadiusz Hiler]

SDL does that when calling SDL_GameControllerSetSensorEnabled() on
supported controllers.

This is a one-way transition, i.e. cannot be undone without re-plugging
the controller or rebooting.

Signed-off-by: Arkadiusz Hiler 
---
 dlls/winebus.sys/bus_udev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index c290cea6f4f..ea54e19df47 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -442,6 +442,17 @@ static void hidraw_device_get_feature_report(struct unix_device *iface, HID_XFER
     {
         io->Information = count;
         io->Status = STATUS_SUCCESS;
+
+        /* disable DS4 report quirk when we get calibration feature report */
+        if ((impl->quirks & QUIRK_DS4_BT) && packet->reportId == 0x5)
+            impl->quirks &= ~QUIRK_DS4_BT;
+
+        /* disable DualSense report quirk when we get calibration feature report */
+        if ((impl->quirks & QUIRK_DUALSENSE_BT) && packet->reportId == 0x5)
+        {
+            TRACE("unquirking dualsense\n");
+            impl->quirks &= ~QUIRK_DUALSENSE_BT;
+        }
     }
     else
     {

[PATCH 3/4] winebus.sys: Enable extended BT reports for Sony controllers when requested.

[Rémi Bernon]

On 1/21/22 17:10, Arkadiusz Hiler wrote:
> SDL does that when calling SDL_GameControllerSetSensorEnabled() on
> supported controllers.
>
> This is a one-way transition, i.e. cannot be undone without re-plugging
> the controller or rebooting.
> Signed-off-by: Arkadiusz Hiler
>
> dlls/winebus.sys/bus_udev.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index c290cea6f4f..ea54e19df47 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -442,6 +442,17 @@ static void hidraw_device_get_feature_report(struct unix_device *iface, HID_XFER
>       {
>           io->Information = count;
>           io->Status = STATUS_SUCCESS;
> -
> -
>    /* disable DS4 report quirk when we get calibration feature report */
> -
>    if ((impl->quirks & QUIRK_DS4_BT) && packet->reportId == 0x5)
> -
>        impl->quirks &= ~QUIRK_DS4_BT;
> -
> -
>    /* disable DualSense report quirk when we get calibration feature report */
> -
>    if ((impl->quirks & QUIRK_DUALSENSE_BT) && packet->reportId == 0x5)
> -
>    {
> -
>        TRACE("unquirking dualsense\n");
> -
>        impl->quirks &= ~QUIRK_DUALSENSE_BT;
> -
>    }
> }
> else
> {

Looks okay, but there could be a message for the DS4 code too. Something 
more readable like "Disabling DS4 controller quirks" could be nice, 
maybe including the device pointer so we can match it in the log.

[PATCH 4/4] hidclass.sys: Add input.inf that matches all HID devices.

[Arkadiusz Hiler]

This makes it so that all the devices on the HID bus now have Class,
ClassGUID, Driver and DriverDesc register properties populated.

Without having at least Driver and Class set SDL2 refuses to use HID
devices directly.

Signed-off-by: Arkadiusz Hiler 
---
 dlls/hidclass.sys/Makefile.in |  2 ++
 dlls/hidclass.sys/hidclass.rc | 20 ++++++++++++++++++++
 dlls/hidclass.sys/input.inf   | 13 +++++++++++++
 loader/wine.inf.in            |  1 +
 4 files changed, 36 insertions(+)
 create mode 100644 dlls/hidclass.sys/hidclass.rc
 create mode 100644 dlls/hidclass.sys/input.inf

diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in
index 25a0396dabe..788828ad66a 100644
--- a/dlls/hidclass.sys/Makefile.in
+++ b/dlls/hidclass.sys/Makefile.in
@@ -5,3 +5,5 @@ IMPORTS   = hal ntoskrnl user32 hidparse
 C_SRCS = \
    device.c \
    pnp.c
+
+RC_SRCS = hidclass.rc
diff --git a/dlls/hidclass.sys/hidclass.rc b/dlls/hidclass.sys/hidclass.rc
new file mode 100644
index 00000000000..409bdc16b45
--- /dev/null
+++ b/dlls/hidclass.sys/hidclass.rc
@@ -0,0 +1,20 @@
+/*
+ * Copyright 2022 Arkadiusz Hiler
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+
+/* @makedep: input.inf */
+1 WINE_DATA_FILE input.inf
diff --git a/dlls/hidclass.sys/input.inf b/dlls/hidclass.sys/input.inf
new file mode 100644
index 00000000000..d9222b13672
--- /dev/null
+++ b/dlls/hidclass.sys/input.inf
@@ -0,0 +1,13 @@
+[Version]
+Signature="$CHICAGO$"
+ClassGuid={745a17a0-74d3-11d0-b6fe-00a0c90f57da}
+Class=HIDClass
+
+[Manufacturer]
+Wine=mfg_section
+
+[mfg_section]
+Wine HID device=device_section,HID\
+
+[device_section.Services]
+AddService = ,0x2
diff --git a/loader/wine.inf.in b/loader/wine.inf.in
index c0251934dfc..afa344ffa80 100644
--- a/loader/wine.inf.in
+++ b/loader/wine.inf.in
@@ -5717,6 +5717,7 @@ protocol,"@%11%\ws2_32.dll,-3"
 services,"@%11%\ws2_32.dll,-4"

[InfFiles]
+input.inf,"@%12%\hidclass.sys,-1"
 winebus.inf,"@%12%\winebus.sys,-1"
 winehid.inf,"@%12%\winehid.sys,-1"
 wineusb.inf,"@%12%\wineusb.sys,-1"

[PATCH 4/4] hidclass.sys: Add input.inf that matches all HID devices.

[Rémi Bernon]

On 1/21/22 17:10, Arkadiusz Hiler wrote:
> This makes it so that all the devices on the HID bus now have Class,
> ClassGUID, Driver and DriverDesc register properties populated.
>
> Without having at least Driver and Class set SDL2 refuses to use HID
> devices directly.
> Signed-off-by: Arkadiusz Hiler
>
> dlls/hidclass.sys/Makefile.in |  2 ++
>   dlls/hidclass.sys/hidclass.rc | 20 ++++++++++++++++++++
>   dlls/hidclass.sys/input.inf   | 13 +++++++++++++
>   loader/wine.inf.in            |  1 +
>   4 files changed, 36 insertions(+)
>   create mode 100644 dlls/hidclass.sys/hidclass.rc
>   create mode 100644 dlls/hidclass.sys/input.inf
>
> diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in
> index 25a0396dabe..788828ad66a 100644
> --- a/dlls/hidclass.sys/Makefile.in
> +++ b/dlls/hidclass.sys/Makefile.in
> @@ -5,3 +5,5 @@ IMPORTS   = hal ntoskrnl user32 hidparse
>   C_SRCS = \
>    device.c \
>    pnp.c
> -
>
> +RC_SRCS = hidclass.rc
> diff --git a/dlls/hidclass.sys/hidclass.rc b/dlls/hidclass.sys/hidclass.rc
> new file mode 100644
> index 00000000000..409bdc16b45
> --- /dev/null
> +++ b/dlls/hidclass.sys/hidclass.rc
> @@ -0,0 +1,20 @@
> +/*
> -
> - Copyright 2022 Arkadiusz Hiler
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
>
> +/* @makedep: input.inf */
> +1 WINE_DATA_FILE input.inf
> diff --git a/dlls/hidclass.sys/input.inf b/dlls/hidclass.sys/input.inf
> new file mode 100644
> index 00000000000..d9222b13672
> --- /dev/null
> +++ b/dlls/hidclass.sys/input.inf
> @@ -0,0 +1,13 @@
> +[Version]
> +Signature="$CHICAGO$"
> +ClassGuid={745a17a0-74d3-11d0-b6fe-00a0c90f57da}
> +Class=HIDClass
> -
>
> +[Manufacturer]
> +Wine=mfg_section
> -
>
> +[mfg_section]
> +Wine HID device=device_section,HID\
> -
>
> +[device_section.Services]
> +AddService = ,0x2
> diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> index c0251934dfc..afa344ffa80 100644
> --- a/loader/wine.inf.in
> +++ b/loader/wine.inf.in
> @@ -5717,6 +5717,7 @@ protocol,"@%11%\ws2_32.dll,-3"
>   services,"@%11%\ws2_32.dll,-4"
>
> [InfFiles]
> +input.inf,"@%12%\hidclass.sys,-1"
>   winebus.inf,"@%12%\winebus.sys,-1"
>   winehid.inf,"@%12%\winehid.sys,-1"
>   wineusb.inf,"@%12%\wineusb.sys,-1"

Looking at the SDL hidapi code it looks like they have much more HID 
devices than just the DS4 and DS5.

Isn't there a risk, if we satisfy all the requirements, that this would 
make SDL try using the devices through HID where we don't support the 
same reports, when it would currently just fallback to XInput / DInput?

[PATCH 4/4] hidclass.sys: Add input.inf that matches all HID devices.

[Arkadiusz Hiler]

On Sat, Jan 22, 2022 at 12:01:12AM +0100, Rémi Bernon wrote:
> On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > This makes it so that all the devices on the HID bus now have Class,
> > ClassGUID, Driver and DriverDesc register properties populated.
> >
> > Without having at least Driver and Class set SDL2 refuses to use HID
> > devices directly.
> > Signed-off-by: Arkadiusz Hiler
> >
> > dlls/hidclass.sys/Makefile.in |  2 ++
> >   dlls/hidclass.sys/hidclass.rc | 20 ++++++++++++++++++++
> >   dlls/hidclass.sys/input.inf   | 13 +++++++++++++
> >   loader/wine.inf.in            |  1 +
> >   4 files changed, 36 insertions(+)
> >   create mode 100644 dlls/hidclass.sys/hidclass.rc
> >   create mode 100644 dlls/hidclass.sys/input.inf
> >
> > diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in
> > index 25a0396dabe..788828ad66a 100644
> > --- a/dlls/hidclass.sys/Makefile.in
> > +++ b/dlls/hidclass.sys/Makefile.in
> > @@ -5,3 +5,5 @@ IMPORTS   = hal ntoskrnl user32 hidparse
> >   C_SRCS = \
> >    device.c \
> >    pnp.c
> > -
> >
> > +RC_SRCS = hidclass.rc
> > diff --git a/dlls/hidclass.sys/hidclass.rc b/dlls/hidclass.sys/hidclass.rc
> > new file mode 100644
> > index 00000000000..409bdc16b45
> > --- /dev/null
> > +++ b/dlls/hidclass.sys/hidclass.rc
> > @@ -0,0 +1,20 @@
> > +/*
> > -
> > - Copyright 2022 Arkadiusz Hiler
> > -
> > -
> > -
> > - This library is free software; you can redistribute it and/or
> > -
> > - modify it under the terms of the GNU Lesser General Public
> > -
> > - License as published by the Free Software Foundation; either
> > -
> > - version 2.1 of the License, or (at your option) any later version.
> > -
> > -
> > -
> > - This library is distributed in the hope that it will be useful,
> > -
> > - but WITHOUT ANY WARRANTY; without even the implied warranty of
> > -
> > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > -
> > - Lesser General Public License for more details.
> > -
> > -
> > -
> > - You should have received a copy of the GNU Lesser General Public
> > -
> > - License along with this library; if not, write to the Free Software
> > -
> > - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> > - */
> > -
> >
> > +/* @makedep: input.inf */
> > +1 WINE_DATA_FILE input.inf
> > diff --git a/dlls/hidclass.sys/input.inf b/dlls/hidclass.sys/input.inf
> > new file mode 100644
> > index 00000000000..d9222b13672
> > --- /dev/null
> > +++ b/dlls/hidclass.sys/input.inf
> > @@ -0,0 +1,13 @@
> > +[Version]
> > +Signature="$CHICAGO$"
> > +ClassGuid={745a17a0-74d3-11d0-b6fe-00a0c90f57da}
> > +Class=HIDClass
> > -
> >
> > +[Manufacturer]
> > +Wine=mfg_section
> > -
> >
> > +[mfg_section]
> > +Wine HID device=device_section,HID\
> > -
> >
> > +[device_section.Services]
> > +AddService = ,0x2
> > diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> > index c0251934dfc..afa344ffa80 100644
> > --- a/loader/wine.inf.in
> > +++ b/loader/wine.inf.in
> > @@ -5717,6 +5717,7 @@ protocol,"@%11%\ws2_32.dll,-3"
> >   services,"@%11%\ws2_32.dll,-4"
> >   [InfFiles]
> > +input.inf,"@%12%\hidclass.sys,-1"
> >   winebus.inf,"@%12%\winebus.sys,-1"
> >   winehid.inf,"@%12%\winehid.sys,-1"
> >   wineusb.inf,"@%12%\wineusb.sys,-1"
>
> Looking at the SDL hidapi code it looks like they have much more HID devices
> than just the DS4 and DS5.
>
> Isn't there a risk, if we satisfy all the requirements, that this would make
> SDL try using the devices through HID where we don't support the same
> reports, when it would currently just fallback to XInput / DInput?

There's always a risk, but we were doing this for a long time as a hack
in Proton 6.3 and there were no complaints. FWIW SDL always handles IG_
devices through xinput.

Maybe we should not match devices that come with fake HID reports built
on top of SDL / evdev? We could solve that by having a specific
CompatibleID assigned for the real HID devices and then using that as a
matcher in the INF's model section.

Some context on why I want to do this: some games provide custom SDL
mappings or detect the controller type using SDLJoystickGUID. GUID's
last two bytes depend on the API SDL is using. Hades for example won't
display correct button glyphs for Sony controllers that are not handled
through HID. That's why the hacks were originally added to Proton.

[PATCH 4/4] hidclass.sys: Add input.inf that matches all HID devices.

[Rémi Bernon]

On 1/22/22 00:17, Arkadiusz Hiler wrote:
> On Sat, Jan 22, 2022 at 12:01:12AM +0100, Rémi Bernon wrote:
> > On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > > This makes it so that all the devices on the HID bus now have Class,
> > > ClassGUID, Driver and DriverDesc register properties populated.
> > >
> > > Without having at least Driver and Class set SDL2 refuses to use HID
> > > devices directly.
> > > Signed-off-by: Arkadiusz Hiler
> > >
> > > dlls/hidclass.sys/Makefile.in |  2 ++
> > >    dlls/hidclass.sys/hidclass.rc | 20 ++++++++++++++++++++
> > >    dlls/hidclass.sys/input.inf   | 13 +++++++++++++
> > >    loader/wine.inf.in            |  1 +
> > >    4 files changed, 36 insertions(+)
> > >    create mode 100644 dlls/hidclass.sys/hidclass.rc
> > >    create mode 100644 dlls/hidclass.sys/input.inf
> > >
> > > diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in
> > > index 25a0396dabe..788828ad66a 100644
> > > --- a/dlls/hidclass.sys/Makefile.in
> > > +++ b/dlls/hidclass.sys/Makefile.in
> > > @@ -5,3 +5,5 @@ IMPORTS   = hal ntoskrnl user32 hidparse
> > >    C_SRCS = \
> > >    	device.c \
> > >    	pnp.c
> > > -
> > >
> > > +RC_SRCS = hidclass.rc
> > > diff --git a/dlls/hidclass.sys/hidclass.rc b/dlls/hidclass.sys/hidclass.rc
> > > new file mode 100644
> > > index 00000000000..409bdc16b45
> > > --- /dev/null
> > > +++ b/dlls/hidclass.sys/hidclass.rc
> > > @@ -0,0 +1,20 @@
> > > +/*
> > > -
> > > - Copyright 2022 Arkadiusz Hiler
> > > -
> > > -
> > > -
> > > - This library is free software; you can redistribute it and/or
> > > -
> > > - modify it under the terms of the GNU Lesser General Public
> > > -
> > > - License as published by the Free Software Foundation; either
> > > -
> > > - version 2.1 of the License, or (at your option) any later version.
> > > -
> > > -
> > > -
> > > - This library is distributed in the hope that it will be useful,
> > > -
> > > - but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > -
> > > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > -
> > > - Lesser General Public License for more details.
> > > -
> > > -
> > > -
> > > - You should have received a copy of the GNU Lesser General Public
> > > -
> > > - License along with this library; if not, write to the Free Software
> > > -
> > > - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> > > - */
> > > -
> > >
> > > +/* @makedep: input.inf */
> > > +1 WINE_DATA_FILE input.inf
> > > diff --git a/dlls/hidclass.sys/input.inf b/dlls/hidclass.sys/input.inf
> > > new file mode 100644
> > > index 00000000000..d9222b13672
> > > --- /dev/null
> > > +++ b/dlls/hidclass.sys/input.inf
> > > @@ -0,0 +1,13 @@
> > > +[Version]
> > > +Signature="$CHICAGO$"
> > > +ClassGuid={745a17a0-74d3-11d0-b6fe-00a0c90f57da}
> > > +Class=HIDClass
> > > -
> > >
> > > +[Manufacturer]
> > > +Wine=mfg_section
> > > -
> > >
> > > +[mfg_section]
> > > +Wine HID device=device_section,HID\
> > > -
> > >
> > > +[device_section.Services]
> > > +AddService = ,0x2
> > > diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> > > index c0251934dfc..afa344ffa80 100644
> > > --- a/loader/wine.inf.in
> > > +++ b/loader/wine.inf.in
> > > @@ -5717,6 +5717,7 @@ protocol,"@%11%\ws2_32.dll,-3"
> > >    services,"@%11%\ws2_32.dll,-4"
> > >    [InfFiles]
> > > +input.inf,"@%12%\hidclass.sys,-1"
> > >    winebus.inf,"@%12%\winebus.sys,-1"
> > >    winehid.inf,"@%12%\winehid.sys,-1"
> > >    wineusb.inf,"@%12%\wineusb.sys,-1"
> >
> > Looking at the SDL hidapi code it looks like they have much more HID devices
> > than just the DS4 and DS5.
> >
> > Isn't there a risk, if we satisfy all the requirements, that this would make
> > SDL try using the devices through HID where we don't support the same
> > reports, when it would currently just fallback to XInput / DInput?
>
> There's always a risk, but we were doing this for a long time as a hack
> in Proton 6.3 and there were no complaints. FWIW SDL always handles IG_
> devices through xinput.

Yeah, though Proton has a device filtering which isn't upstream, where 
most devices are going through hidraw when possible, and when not 
through SDL. I think we would want some of it to be upstream, as the 
Sony controllers for instance, are better handled when used through 
hidraw, but I would like to keep things more flexible than Proton.

I also don't know if anyone has tried the XBox wireless controllers 
ever, which are the one the SDL hidapi code seem to support and which 
I'm a bit skeptical whether they would work through hidraw.
> Maybe we should not match devices that come with fake HID reports built
> on top of SDL / evdev? We could solve that by having a specific
> CompatibleID assigned for the real HID devices and then using that as a
> matcher in the INF's model section.

Yeah that sounds like a good idea but although we could report some 
specific compatible id on the winebus.sys level for raw / pass-through 
devices, they will not be directly visible on the top HID devices.
> Some context on why I want to do this: some games provide custom SDL
> mappings or detect the controller type using SDLJoystickGUID. GUID's
> last two bytes depend on the API SDL is using. Hades for example won't
> display correct button glyphs for Sony controllers that are not handled
> through HID. That's why the hacks were originally added to Proton.

Sure, thanks for the details!

[Rémi Bernon]

Hi Arek,

On 1/21/22 17:10, Arkadiusz Hiler wrote:
> Signed-off-by: Arkadiusz Hiler
>
> dlls/winebus.sys/unix_private.h | 1 +
>   dlls/winebus.sys/unixlib.c      | 6 ++++++
>   2 files changed, 7 insertions(+)
>
> diff --git a/dlls/winebus.sys/unix_private.h b/dlls/winebus.sys/unix_private.h
> index efecf6cdbe3..e897251dea8 100644
> --- a/dlls/winebus.sys/unix_private.h
> +++ b/dlls/winebus.sys/unix_private.h
> @@ -266,5 +266,6 @@ extern void hid_device_set_effect_state(struct unix_device *iface, BYTE index, B
>
> BOOL is_xbox_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;
>   BOOL is_dualshock4_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;
> +BOOL is_dualsense_gamepad(WORD vid, WORD pid) DECLSPEC_HIDDEN;
>
> #endif /* __WINEBUS_UNIX_PRIVATE_H */
> diff --git a/dlls/winebus.sys/unixlib.c b/dlls/winebus.sys/unixlib.c
> index 1269ae05c2b..3a7c3a16428 100644
> --- a/dlls/winebus.sys/unixlib.c
> +++ b/dlls/winebus.sys/unixlib.c
> @@ -70,6 +70,12 @@ BOOL is_dualshock4_gamepad(WORD vid, WORD pid)
>       return FALSE;
>   }
>
> +BOOL is_dualsense_gamepad(WORD vid, WORD pid)
> +{
> - if (vid == 0x054c && pid == 0x0ce6) return TRUE;
> - return FALSE;
>
> +}
> - struct mouse_device
> {
>     struct unix_device unix_device;

I think this should be merged into the next patch so that we don't add 
code that's not used yet.