Re: [PATCH] hid-asus-ally: Add full gamepad support

Re: [PATCH] hid-asus-ally: Add full gamepad support

[Antheas Kapenekakis]

Hi Luke,
I reviewed this patch and leave some of my thoughts below.

I am the creator of the Handheld Daemon (HHD) (https://github.com/hhd-dev/hhd)
app, which offers a lot of this functionality using a userspace implementation,
and have thousands of handheld users, a lot of whom are on both Allies,
so I believe I have some valuable feedback on this.

First for some background:
The original Ally used a composite USB device with the following components:
a vendor interface for Armoury Crate, an XInput gamepad device, and a Keyboard
device for things such as WASD and Mouse mode support.
Armoury Crate uses the vendor interface to control device mappings and how the
device should act, including by sending faux commands to it for gyro emulation
using the Bosch gyro built into the screen. As such, no drivers are required for
it in Windows, and the same can be said of Linux. Although without Armoury Crate
in either Windows or Linux, the controller does not behave properly.

In Ally X, Asus seems to have been told by Microsoft that the XInput driver is
being deprecated and they've had to move on to a different one.
For Asus, this meant doing a HID implementation which seems to be
similar to the Xbox Adaptive Controller, and using a custom windows
driver for it
in Windows. Therefore, unlike the original Ally, which did not require Windows
drivers to have its controller work, Ally X does.

It is a similar situation in Linux, where without a kernel driver, the
controller
of the Ally reads as a HID Gamepad with a wrong mapping, and does not have
vibration support.

The patch I am replying to does three things (which ideally should be separated
into different patches): provide an in-kernel equivalent for Armoury
Crate options
that will configure both Ally and Ally X, a HID driver for the Ally X that
restores vibration functionality and a sane mapping to the controllers, and
finally, a cross-interface "hack" which only works on the X and pulls the vendor
"Armoury" and "ROG Crate" buttons into the new HID gamepad driver and turns them
into "BTN_GUIDE" and "BTN_GUIDE+BTN_SOUTH" (which is recognized by
Steam to open the
side menu and by now defunct yuzu to switch from windowed to full screen).

Let me add notes for each one.

## Gamepad HID Driver
For the HID gamepad driver: we are in agreement that such a driver should exist
for the X. It makes basic functionality accessible to users without a userspace
tool and restores mapping and vibration.

Your implementation seems straightforward and I only have a couple of
notes for it:

First, you are hardcoding `desc.bEndpointAddress`, which is error
prone and could
break in the future with an MCU/EC update. You should instead use the Usage Page
and Usage of the device, which in this case are unique. Referencing HHD, for the
HID gamepad, the usage page is 0xFF31 and the usage is 0x0080. And perhaps there
is a gamepad driver already that could support Ally X with a simple quirk.

Following, you attempted to mimic an Xinput device, but your driver does not set
the ABS values of the device properly. For your convenience, here are the values
from the Legion Go, which uses an XInput gamepad:
```
   0x0000: ABS_X
     > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
   0x0001: ABS_Y
     > [val -1, min -32768, max 32767, fuzz 16, flat 128, res 0]
   0x0002: ABS_Z
     > [val 0, min 0, max 255, fuzz 0, flat 0, res 0]
   0x0003: ABS_RX
     > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
   0x0004: ABS_RY
     > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
   0x0005: ABS_RZ
     > [val 0, min 0, max 255, fuzz 0, flat 0, res 0]
   0x0010: ABS_HAT0X
     > [val 0, min -1, max 1, fuzz 0, flat 0, res 0]
   0x0011: ABS_HAT0Y
     > [val 0, min -1, max 1, fuzz 0, flat 0, res 0]
```
Specifically, your ABS_X, ABS_Y, ABS_RX, ABS_RY values are set from 0
to ABS_MAX, which is
a deviation from XInput and more similar to DInput. This means that if I added
support for your patchset to HHD which supports over 30 XInput
devices, I would have to
quirk it only for the Ally X. If you break HHD, the chance you break
userspace in
general (e.g., random proton game) is very high.

## Armoury Crate Functionality
Let me move over to armoury crate functionality, which you first started in
December as the XPAD patchset, and as such I have a certain familiarity with it.
I will not reiterate the `desc.bEndpointAddress` quirk.
The vendor interface is a HID device and it is the case both in Linux
and Windows
that configuration is usually done by userspace programs.
I recall multiple examples of this in Linux as well, e.g., Solaar for Logitech,
asusctl (your daemon) for Asus, libratbag for mice, and OpenRGB for
rgb which includes
the Ally. Can you justify the deviation here and the need for a kernel driver?

I am afraid that it might be unreliable, and applying kernel patches
is something
very difficult that most users can not do. Furthermore, given the user chooses
to install a userspace program to manage their controller (e.g., HHD),
you should
ensure this driver functionality can be disabled, from boot, so that it does not
interfere with userspace programs.

For example, one problem I recall that before we removed the XPAD patchset
from the distribution Bazzite, I noticed during reviewing logs that your patch
regularly failed the ready check on certain allies, and after trying
the ready check
myself as part of HHD, I had to pull it after 10 hours because users
came out of
the woodwork to report their controller misbehaved.
Of course, in those cases HHD handled initialization, so your patch failing did
not affect functionality.

I know that some basic initialization is required regardless for the
gamepad to work
without e.g., HHD. In my experience changing the gamepad mode to
Gamepad (from Mouse
mode) with a single HID command should suffice. Then, the rest of the settings
are initialized to manufacturer defaults.

## Xbox/Xbox + A Quirk
As mentioned above, the vendor device of the Ally reports the front manufacturer
buttons as HID commands from the special vendor device. This allows Armoury
crate to open in windows. Of course, in Linux there is no Armoury crate, so
these buttons are unused by default. Userspace programs can fix this
quirk and your previous kernel patches do as much, by exposing the buttons as
F15, F16, F17, F18.

A final component to this patchset is taking this vendor device and making it
accessible from the gamepad driver. This is uniquely possible on the
Ally X, because
the gamepad happens to be HID and located on the same USB interface, so you can,
with a bit of trickery, receive the front button commands from the
gamepad driver.
This allows you to quirk them, and here specifically for steam, to
Xbox and Xbox+A
("BTN_GUIDE" and "BTN_GUIDE+BTN_SOUTH"), which have similar functionality to
the Steamdeck's steam and three dot buttons.

In my opinion, that borders a bit on being too opinionated, and perhaps e.g.,
Asus would also share this opinion. Imagine if Asus tried to add some sort of
official support to linux, only to find out the Linux driver is remapping the
buttons and they have to work around that. Also, combining two HID devices is
a bit unorthodox, and may cause other issues (see `desc.bEndpointAddress`).
So perhaps Asus needs to be consulted here?

Further, I have some advice for your button implementation.
80ms is too short for steam and is a delay I have tried before. It
will fail in low
device TDPs, as Steam will begin to lag and miss the commands. For
your reference,
HHD uses 150ms for this and I have tried 80ms in the past.

In general, I am trying to move away from Xbox+A as it tends to add delay and
since I prefer to multiplex the right vendor button for double
presses, it starts
to become noticable. It also does not work if the user presses the
Nintendo layout
button in the Steam UI.

In addition, you are freezing the kernel driver to send those commands for 240ms
which is around 100 reports.
I would advise that you store that the user has pressed the vendor button and
then when you process the following HID reports, you set the Xbox
button as 1 for
300ms after the button is pressed and the A button to 1 after 150ms until 300ms.
It is fine to release both buttons at the same time.

## Other issues
These are my overall thoughts. Since I went through the effort of
posting to lkml,
I will also draw your attention to 2 other patches you upstreamed and
cause issues
for the original Ally and now Ally X.

The first one is allowing users to enable mcu_powersave in February.
Beforehand, it was force disabled by the kernel by your previous patch.
This feature remains broken when the device is at low TDPs and unplugged.
We force disable it now as part of the distribution Bazzite now.
Applies to both Ally and Ally X.

The second occasion was 3 weeks ago when you pushed the Ally X quirk
and converted
`ally_mcu_usb_switch` into a DMI table. It seems that a DMI table does
not work in
this codepath, and it is something that we had tried over a week
before you posted the
patch and had reverted it. If you had asked me I would have told you as much.
After this patch got cherry picked by CachyOS and ChimeraOS, it ended
up breaking
both Allies, which is not a favorable user experience.

Moving forward, I will try to be more attentive in the kernel conversation and,
e.g., upstream patches. And perhaps you can be a bit more thorough
testing before you push your patches up to the kernel ;).

Good luck with this patch series!

Of course, once you fix the notes I have above and/or your patchset is
integrated
to the kernel, I will make sure HHD is forward compatible with it, while being
backwards compatible with older kernels.

Best Regards,
Antheas

Re: [PATCH] hid-asus-ally: Add full gamepad support

[Luke Jones]

On Fri, 9 Aug 2024, at 3:35 AM, Antheas Kapenekakis wrote:
> Hi Luke,
> I reviewed this patch and leave some of my thoughts below.
> 
> I am the creator of the Handheld Daemon (HHD) (https://github.com/hhd-dev/hhd)
> app, which offers a lot of this functionality using a userspace implementation,
> and have thousands of handheld users, a lot of whom are on both Allies,
> so I believe I have some valuable feedback on this.

Then you are aware that the work I do in kernel is almost always targeting ASUS ROG. You will also be aware that my goal is to enable default user experience *without* the requirement of any userspace blobs - this includes moving things from my own daemon to kernel where possible so that users do not require a userspace app at all if they so desire.

> In Ally X, Asus seems to have been told by Microsoft that the XInput driver is
> being deprecated and they've had to move on to a different one.
> For Asus, this meant doing a HID implementation which seems to be
> similar to the Xbox Adaptive Controller, and using a custom windows
> driver for it
> in Windows. Therefore, unlike the original Ally, which did not require Windows
> drivers to have its controller work, Ally X does.

You're repeating information that has come directly from me.

> First, you are hardcoding `desc.bEndpointAddress`, which is error
> prone and could
> break in the future with an MCU/EC update.

I have many records from many MCU updates. It doesn't happen. This is even true right across the ROG laptop range for as long as I've been tracking these. ASUS is remarkably consistent.

> HID gamepad, the usage page is 0xFF31 and the usage is 0x0080. And perhaps there
> is a gamepad driver already that could support Ally X with a simple quirk.

There isn't. The ones that might support it are not being upstreamed. And that is also *not* my goal here (a complete and accessible default user experience with zero dependency).

> Following, you attempted to mimic an Xinput device, but your driver does not set
> the ABS values of the device properly. For your convenience, here are the values
> from the Legion Go, which uses an XInput gamepad:
> ```
>    0x0000: ABS_X
>      > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
>    0x0001: ABS_Y
>      > [val -1, min -32768, max 32767, fuzz 16, flat 128, res 0]
>    0x0002: ABS_Z
>      > [val 0, min 0, max 255, fuzz 0, flat 0, res 0]
>    0x0003: ABS_RX
>      > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
>    0x0004: ABS_RY
>      > [val 0, min -32768, max 32767, fuzz 16, flat 128, res 0]
>    0x0005: ABS_RZ
>      > [val 0, min 0, max 255, fuzz 0, flat 0, res 0]
>    0x0010: ABS_HAT0X
>      > [val 0, min -1, max 1, fuzz 0, flat 0, res 0]
>    0x0011: ABS_HAT0Y
>      > [val 0, min -1, max 1, fuzz 0, flat 0, res 0]
> ```

"Attempted"... I *did*. You've failed to notice that what I've set is what is reported by the HID report. When you use the event and js interfaces it is shown as above. I invite you to modify the driver as you think and see the results for yourself instead of suggesting I don't know what I'm doing here.

> quirk it only for the Ally X. If you break HHD, the chance you break
> userspace in
> general (e.g., random proton game) is very high.

I don't know what HHD is doing, but it sounds like it is not very compatible with anything anyone else does - particularly in kernel when functionality *is being added*. InputPlumber on the other hand was easily able to support this driver.

If functionality is missing for a device the proper thing to do is add that upstream to the kernel for *everyone* to benefit from, not keep it local to a userspace application where it benefits only users of that application and results in yet more duplicated effort, fragmentation, lack of or less peer review.

> I recall multiple examples of this in Linux as well, e.g., Solaar for Logitech,
> asusctl (your daemon) for Asus, libratbag for mice, and OpenRGB for
> rgb which includes
> the Ally. Can you justify the deviation here and the need for a kernel driver?

As I said, default user experience *without* requirement for userspace - the appropriate place for this is in the kernel, not in userspace. The job of the kernel is to handle hardware and expose safe interfaces for it (asusctl exists to expose a safe userspace dbus API for the functions I add to the kernel that doesn't require root. And I code for kernel first, asusctl second - the exception being RGB led because that is difficult to handle well with mcled_class when so many laptops have varying levels of function here - thus I await the outcome of a very long discussion on LKML regarding this).

Throw any plain distro on Ally with this patch and it should just work. And again InputPlumber was able to work with it without any fuss and I believe has gained a lot from it. Why can't you?

> 
> I am afraid that it might be unreliable, and applying kernel patches
> is something
> very difficult that most users can not do. Furthermore, given the user chooses
> to install a userspace program to manage their controller (e.g., HHD),
> you should
> ensure this driver functionality can be disabled, from boot, so that it does not
> interfere with userspace programs.

It is so far more reliable than a python daemon and it all gets reviewed heavily before being accepted to mainstream. Most users do not apply patches, they use a distro that provides everything required.

> For example, one problem I recall that before we removed the XPAD patchset
> from the distribution Bazzite, I noticed during reviewing logs that your patch
> regularly failed the ready check on certain allies, and after trying
> the ready check
> myself as part of HHD, I had to pull it after 10 hours because users
> came out of
> the woodwork to report their controller misbehaved.
> Of course, in those cases HHD handled initialization, so your patch failing did
> not affect functionality.

I've had zero reports of this.

> ## Xbox/Xbox + A Quirk
> As mentioned above, the vendor device of the Ally reports the front manufacturer
> buttons as HID commands from the special vendor device. This allows Armoury
> crate to open in windows. Of course, in Linux there is no Armoury crate, so
> these buttons are unused by default. Userspace programs can fix this
> quirk and your previous kernel patches do as much, by exposing the buttons as
> F15, F16, F17, F18.
> 
> A final component to this patchset is taking this vendor device and making it
> accessible from the gamepad driver. This is uniquely possible on the
> Ally X, because
> the gamepad happens to be HID and located on the same USB interface, so you can,
> with a bit of trickery, receive the front button commands from the
> gamepad driver.

There is no trickery. It very simply takes the event and puts it through the shared input device that was created - dead easy with a driver with global state.

> This allows you to quirk them, and here specifically for steam, to
> Xbox and Xbox+A
> ("BTN_GUIDE" and "BTN_GUIDE+BTN_SOUTH"), which have similar functionality to
> the Steamdeck's steam and three dot buttons.
> 
> In my opinion, that borders a bit on being too opinionated, and perhaps e.g.,
> Asus would also share this opinion. Imagine if Asus tried to add some sort of
> official support to linux, only to find out the Linux driver is remapping the
> buttons and they have to work around that. Also, combining two HID devices is
> a bit unorthodox, and may cause other issues (see `desc.bEndpointAddress`).
> So perhaps Asus needs to be consulted here?

I talk to ASUS engineers regularly. I in fact have an agreement with them, and I'm under NDA for a lot of specifics.

> Further, I have some advice for your button implementation.
> 80ms is too short for steam and is a delay I have tried before. It
> will fail in low
> device TDPs, as Steam will begin to lag and miss the commands. For
> your reference,
> HHD uses 150ms for this and I have tried 80ms in the past.c.

It is a very different story in a kernel driver vs a python blob daemon subject to GC, userspace etc.

> In general, I am trying to move away from Xbox+A as it tends to add delay and
> since I prefer to multiplex the right vendor button for double
> presses, it starts
> to become noticable. It also does not work if the user presses the
> Nintendo layout
> button in the Steam UI.
> 
> In addition, you are freezing the kernel driver to send those commands for 240ms
> which is around 100 reports.

It is done on a worker. It is not blocking the kernel....

> The first one is allowing users to enable mcu_powersave in February.
> Beforehand, it was force disabled by the kernel by your previous patch.
> This feature remains broken when the device is at low TDPs and unplugged.
> We force disable it now as part of the distribution Bazzite now.
> Applies to both Ally and Ally X.

This is not my experience on both Ally and Ally X. Please describe how you think it is broken? If you mean that the USB devices are removed and re-added - then it is up to you to account for this, just as other tools now do. The powersaving gained from enabling this for suspend can be significant and in the case of the Ally X it drops right down to a 2% use of 80Wh over 10 hours. ChimeraOS now enables it by default, and this was heavily tested.

> 
> The second occasion was 3 weeks ago when you pushed the Ally X quirk
> and converted
> `ally_mcu_usb_switch` into a DMI table. It seems that a DMI table does
> not work in
> this codepath, and it is something that we had tried over a week
> before you posted the
> patch and had reverted it. If you had asked me I would have told you as much.
> After this patch got cherry picked by CachyOS and ChimeraOS, it ended
> up breaking
> both Allies, which is not a favorable user experience.

Does not work how? And what do you mean "Asked you?" I don't know you, and your work is not my target here - the default kernel is. This is what you need to be prepared to work with just like others are. Userspace needs to work with kernelspace, and hardware interaction is the purpose of the kernel - this driver exposes this functionality in a safe way *for* userspace to consume without requiring direct (raw) access to hardware.

> Moving forward, I will try to be more attentive in the kernel conversation and,
> e.g., upstream patches. And perhaps you can be a bit more thorough
> testing before you push your patches up to the kernel.

I test, mate. With the default kernel and empty userspace. This was also pulled in to ChimeraOS Unstable as part of that testing and validation - it has been quite thoroughly vetted at this point by myself and many others.

Luke.

Re: [PATCH] hid-asus-ally: Add full gamepad support

[Antheas Kapenekakis]

Hi Luke,
thank you for taking the time to reply.

And everyone else, thank you for putting up with my broken line spacing,
because of Gmail. And perhaps the lack of in-reply-to because of mailto.
Hopefully in-reply-to works this time ;).

First off, understand that I do not mean to attack your work. I tried to
make my response helpful to you. In fact, I took a lot of time in writing it
and preemptively saved you a lot of work in testing your patchset so you do
not have to spend time reaching the same conclusions that I have had to.

Provided you heed my comments of course, which is not clear from your reply.

As I currently represent what is the largest Linux ROG Ally community, I hope
it is reasonable that it is in my and my community's best interest that an
unstable patch which could affect Linux Ally support should be vetted before
it becomes part of the mainline kernel. Once it becomes part of the kernel,
workarounds around it will become very hard. We have achieved near perfect
controller support through userspace, so I would hate to jeopardize that.

In my previous email, I gave you my preliminary thoughts without having time
to test your patchset. Of course, as I noted in that email, I will be testing
and integrating support for your patchset, personally, with an Ally X unit
I will be getting access to close to the end of August.

I do not think there is value in arguing, therefore I am not happy continuing
the discussion under this tone. Hopefully, in a few days you, will have
another look at my comments, with a level head, and address them.
I still believe that they are valid and that if they are fixed, I am more
than happy with your patchset merging into the kernel.

Since you raised some technical points in your response, let me disambiguate.

> You're repeating information that has come directly from me.

Indeed, this specific point (XInput being deprecated) came from you.
I am just bringing everyone up to speed, since I feel your patch missed
some important context.

> I have many records from many MCU updates. It doesn't happen. (referring to
> relying to the endpoint descriptor instead of HID Usages)

In my opinion, using the standard Usage Page and Usage the controller reports
remains the proper solution (this is what the Windows driver does).
Remember that if there is a breakage due to a firmware update, users will
become unable to use their device as they can not update the kernel.

Using `desc.bEndpointAddress` may be appropriate for a userspace tool or an
out-of-tree kernel driver, but perhaps not for the mainline kernel.
I am happy to be proven wrong.

> "Attempted"... I *did*. You've failed to notice that what I've set is
> what is reported by the HID report.

I am a bit confused here. I thought the purpose of your patch was to convert
the HID report into what xpad would export. That means respecting xpad, not
a random HID report.

In this case, the absinfo (with `input_set_abs_params`) needs to be set
according to what is set by xpad, which is signed and from -32768 to 32767
(referencing both the Linux Gamepad Specification and the out-of-tree driver
xpadneo which seems to be the prominent driver providing support for
controllers similar to Ally X, i.e., Xbox One bluetooth controllers).

I know from experience that Handheld Daemon will have a problem with this.
But alas, I was not referring to Handheld Daemon being the problem here:
it is simple enough to fix it in there and I will do it when I add
support for your patchset (would rather avoid doing it or doing it and having
to revert it of course). I was moreso referring to other userspace
applications without this privilege.

As for why I have to add support for your patchset, it is simply because
it being there changes the controller mappings, so I simply need to add
an if statement that uses the standard XInput mappings when it is available.

This is not to say that the end result will be as reliable as without your
driver, as Handheld Daemon will then be at the mercy of your kernel driver.
So please, do extended testing and I think with e.g., ChimeraOS 46.2 being
released yesterday with your patchset, you will get some valuable
feedback soon enough.

I know that I have spent multiple weeks already optimizing my implementation,
having released it close to a month ago. Which is also why I am not in a
rush to add support for your experimental patchset.

> It is a very different story in a kernel driver... (referring to the 80ms
> delay used for Xbox+A)

Please understand that I have spent weeks of effort debugging and optimizing
the Side Menu behavior of Handheld Daemon. In fact, it currently implements
three different ways of opening the Steam Side Menu (keyboard, extest through
the gamescope X11 socket, and as a last fallback as Xbox + A). When I say
80ms is not enough, I know it is not enough. Otherwise Handheld Daemon would
be using 80ms. The rest is conjecture.

> It is done on a worker. It is not blocking the kernel....
> (referring to xbox+a holding a spinlock to send the key combo with msleep)

Repeating myself:

> > In addition, you are freezing the kernel **driver** to send those
> > commands for 240ms which is around 100 reports.

Which in my opinion will become more like 300ms. Freezing the controller for
that long is not ideal (I know you are not freezing the kernel).
Please revisit this.

> Please describe how you think it is broken? (referring to `mcu_powersave`)

Quoting myself:

> > This feature remains broken when the device is at low TDPs and unplugged.

e.g., when the Ally is set on its quiet mode and or is below 12W, and is
suspended unplugged, with Steam and a game running. In this case,
the USB controller of the Ally simply does not wake up and RGB breaks.
The occurrence of this given those conditions is around 40% of the time.
This includes testing with or without your DMI table patch by the way.

> Does not work how? (referring to `ally_mcu_usb_switch`)

Seems like a DMI table always sets it to 0. I do not know why. However I do
know that as part of our validation on the distribution Bazzite which took
place prior to you submitting your patch, we tested both adding an or for
ally x and a dmi table, and the dmi table did not work. Post submitting your
patch, there was a 5 day brief period where both the unstable ChimeraOS
kernel and the CachyOs kernel integrated your patch before they also
integrated the patch I am replying to (which makes Handheld Daemon not work;
for now). During this period both the original ally and the ally x regressed
when `mcu_powersave` was disabled.

> I test, mate. With the default kernel and empty userspace.

Unfortunately, this is not how users interact with my kernel patches and
Handheld Daemon. They usually play games with SteamUI running in the
background. Often, this includes setting an aggressively low TDP, and multiple
suspends back-to-back while in game. This is the standard I hold myself to.
And I would expect no less from a mainline kernel driver.

I hope I replied to all your technical claims. If I missed any, I am
happy to clarify and expand where appropriate.

Again, good luck with your patchset. Hopefully, it will merge with 6.12 and
Ally X owners will get an acceptable result without the need for userspace
tools (albeit without gyro, back buttons, and RGB being part of the
controller).

Best regards,
Antheas

Re: [PATCH] hid-asus-ally: Add full gamepad support

[Denis Benato]

On 11/08/24 18:10, Antheas Kapenekakis wrote:
> Hi Luke,
> thank you for taking the time to reply.
> 
> And everyone else, thank you for putting up with my broken line spacing,
> because of Gmail. And perhaps the lack of in-reply-to because of mailto.
> Hopefully in-reply-to works this time ;).
>> First off, understand that I do not mean to attack your work. I tried to
> make my response helpful to you. In fact, I took a lot of time in writing it
> and preemptively saved you a lot of work in testing your patchset so you do
> not have to spend time reaching the same conclusions that I have had to.
>> Provided you heed my comments of course, which is not clear from your reply.
> 
> As I currently represent what is the largest Linux ROG Ally community, I hope
> it is reasonable that it is in my and my community's best interest that an
> unstable patch which could affect Linux Ally support should be vetted before
> it becomes part of the mainline kernel. Once it becomes part of the kernel,
> workarounds around it will become very hard. We have achieved near perfect
> controller support through userspace, so I would hate to jeopardize that.
> 
Hello,

I'm Denis and I want to say a few things:

first of all I have been following the ROG Ally scene from quite a while now and
I submitted one of the oldest patches regarding it specifically:
https://lore.kernel.org/all/20231117011556.13067-1-luke@ljones.dev/
and in no way I feel represented by you.

We have talked about this matter many times in an open-to-everybody discord server,
and all the records are still available for whoever wants to read those in the chimeraos
server.

We all have warned you about taking control of devices from userspace, many and
many times again. More than nine months ago, you decided to ignore us all and
write an application you knew it would have played bad with future a driver that would
have been submitted upstream.

To be clear to readers your application started as a clone of my own application, meant
as a temporary workaround for a support that we all knew was coming:
mine: https://github.com/NeroReflex/ROGueENEMY (first commit: 2 Nov 2023)
yours: https://github.com/hhd-dev/hhd (first commit: 30 Nov 2023)
plus yours was targeting lenovo hardware specifically and added ROG support
after some time... Both have been developed in the same discord server, there
is absolutely no way you could have missed those messages.

The first tool supporting the legion go console was a port of my own tool by another
user (that I helped) in our server and we all knew it was a very suboptimal (and temporary)
way of arriving at the final goal of having these handhelds devices in a reliable
ready-to-game state, including you.

> In my previous email, I gave you my preliminary thoughts without having time
> to test your patchset. Of course, as I noted in that email, I will be testing
> and integrating support for your patchset, personally, with an Ally X unit
> I will be getting access to close to the end of August.
> 
> I do not think there is value in arguing, therefore I am not happy continuing
> the discussion under this tone. Hopefully, in a few days you, will have
> another look at my comments, with a level head, and address them.
> I still believe that they are valid and that if they are fixed, I am more
> than happy with your patchset merging into the kernel.
> 
> Since you raised some technical points in your response, let me disambiguate.
> 
>> You're repeating information that has come directly from me.
> 
> Indeed, this specific point (XInput being deprecated) came from you.
> I am just bringing everyone up to speed, since I feel your patch missed
> some important context.
> 
>> I have many records from many MCU updates. It doesn't happen. (referring to
>> relying to the endpoint descriptor instead of HID Usages)
> 
> In my opinion, using the standard Usage Page and Usage the controller reports
> remains the proper solution (this is what the Windows driver does).
> Remember that if there is a breakage due to a firmware update, users will
> become unable to use their device as they can not update the kernel.
> 
> Using `desc.bEndpointAddress` may be appropriate for a userspace tool or an
> out-of-tree kernel driver, but perhaps not for the mainline kernel.
> I am happy to be proven wrong.
> 
>> "Attempted"... I *did*. You've failed to notice that what I've set is
>> what is reported by the HID report.
> 
> I am a bit confused here. I thought the purpose of your patch was to convert
> the HID report into what xpad would export. That means respecting xpad, not
> a random HID report.
> 
> In this case, the absinfo (with `input_set_abs_params`) needs to be set
> according to what is set by xpad, which is signed and from -32768 to 32767
> (referencing both the Linux Gamepad Specification and the out-of-tree driver
> xpadneo which seems to be the prominent driver providing support for
> controllers similar to Ally X, i.e., Xbox One bluetooth controllers).
> 
> I know from experience that Handheld Daemon will have a problem with this.
> But alas, I was not referring to Handheld Daemon being the problem here:
> it is simple enough to fix it in there and I will do it when I add
> support for your patchset (would rather avoid doing it or doing it and having
> to revert it of course). I was moreso referring to other userspace
> applications without this privilege.
> 
> As for why I have to add support for your patchset, it is simply because
> it being there changes the controller mappings, so I simply need to add
> an if statement that uses the standard XInput mappings when it is available.
> 
> This is not to say that the end result will be as reliable as without your
> driver, as Handheld Daemon will then be at the mercy of your kernel driver.
> So please, do extended testing and I think with e.g., ChimeraOS 46.2 being
> released yesterday with your patchset, you will get some valuable
> feedback soon enough.
> 

To be fair, the submitted work has been ongoing for quite some time now,
and it has been tested on both ally and ally x by at least five developers,
including me who tested even more scenarios than input, especially regarding s2idle.
Moreover users of other distributions tested the driver too.

Our distribution version was released as stable when we felt it was completed in all features
and we have finished testing what we could have possibly taught of (this happens to be yesterday),
but anyway I don't believe the release date of a distribution is a valid point of argument for a kernel driver.

If there are bugs please file an actual bug report them so we can fix them.

> I know that I have spent multiple weeks already optimizing my implementation,
> having released it close to a month ago. Which is also why I am not in a
> rush to add support for your experimental patchset.
> 

Again, you were totally aware of what you were doing and what was going on,
so this is not "unfortunate accident".

>> It is a very different story in a kernel driver... (referring to the 80ms
>> delay used for Xbox+A)
> 
> Please understand that I have spent weeks of effort debugging and optimizing
> the Side Menu behavior of Handheld Daemon. In fact, it currently implements
> three different ways of opening the Steam Side Menu (keyboard, extest through
> the gamescope X11 socket, and as a last fallback as Xbox + A). When I say
> 80ms is not enough, I know it is not enough. Otherwise Handheld Daemon would
> be using 80ms. The rest is conjecture.
> 

I am not understanding the point here. If your userspace tool was working before all of these
additions why can't you simply ship a blacklist file for the new driver?

If you are relying in some initialization code done from the driver
I offer myself in adding a kernel argument to keep only that initialization code.

>> It is done on a worker. It is not blocking the kernel....
>> (referring to xbox+a holding a spinlock to send the key combo with msleep)
> 
> Repeating myself:
> 
>>> In addition, you are freezing the kernel **driver** to send those
>>> commands for 240ms which is around 100 reports.
> 
> Which in my opinion will become more like 300ms. Freezing the controller for
> that long is not ideal (I know you are not freezing the kernel).
> Please revisit this.
> 
>> Please describe how you think it is broken? (referring to `mcu_powersave`)
> 
> Quoting myself:
> 
>>> This feature remains broken when the device is at low TDPs and unplugged.
> 
> e.g., when the Ally is set on its quiet mode and or is below 12W, and is
> suspended unplugged, with Steam and a game running. In this case,
> the USB controller of the Ally simply does not wake up and RGB breaks.
> The occurrence of this given those conditions is around 40% of the time.
> This includes testing with or without your DMI table patch by the way.
> 
>> Does not work how? (referring to `ally_mcu_usb_switch`)
> 
> Seems like a DMI table always sets it to 0. I do not know why. However I do
> know that as part of our validation on the distribution Bazzite which took
> place prior to you submitting your patch, we tested both adding an or for
> ally x and a dmi table, and the dmi table did not work. Post submitting your
> patch, there was a 5 day brief period where both the unstable ChimeraOS
> kernel and the CachyOs kernel integrated your patch before they also
> integrated the patch I am replying to (which makes Handheld Daemon not work;
> for now). During this period both the original ally and the ally x regressed
> when `mcu_powersave` was disabled.
> 
>> I test, mate. With the default kernel and empty userspace.

MCU powersave is not even part of this driver. It directly affects the device to be
clear, but nobody can do anything about it: it is how hardware has been designed.

If you need said feature to be disabled you can use an udev rule, that will indeed
make the device consume more while in s2idle state and is therefore suboptimal,
otherwise please brings this up where it is relevant.

Again all of this is knowledge that has been around for about a year, and none of
this comes at a surprise to you.

> 
> Unfortunately, this is not how users interact with my kernel patches and
> Handheld Daemon. They usually play games with SteamUI running in the
> background. Often, this includes setting an aggressively low TDP, and multiple
> suspends back-to-back while in game. This is the standard I hold myself to.
> And I would expect no less from a mainline kernel driver.
> 

The vast majority of users are using a distribution meant for handheld hardware
because tradition distributions requires having a mouse and keyboard around,
however we have records in our server of people asking how to have their controllers
working in archlinux, so what you say is not an absolute truth and there are people who
actually wants being able to use theirs hardware without any of our userspace tooling:
"ours" means all of them, my unmaintained proof-of-concept project, your hdd and inputplumber.

Plus you are also talking about TDP here, and this is a totally separate topic from my understanding.

> I hope I replied to all your technical claims. If I missed any, I am
> happy to clarify and expand where appropriate.
> 
> Again, good luck with your patchset. Hopefully, it will merge with 6.12 and
> Ally X owners will get an acceptable result without the need for userspace
> tools (albeit without gyro, back buttons, and RGB being part of the
> controller).

Gyroscope is on a different subsystem on the kernel and there is no way of having it part of
the input device without userspace drivers. This is true for every OS, including windows.

Back buttons are mappable and can be made part of the controller, and this is even documented:
it comes with some caveats, but if an user wants to have X and Y buttons mapped on back paddles
it will now be possible.

RGB support is once again another example of a peripheral that is impossible to be made
part of an input device, so I am not getting the point:
the fact that you control leds via the emulated controller because some other user
space drivers for that specific emulated device can does mean nothing in this context
and to be fair since you are sending raw hidraw commands to that device it means you are
already driving the current driver in an inconsistent state (reported state vs hardware state)
and therefore blacklisting it is what you should have done months ago, when we told you were
already conflicting with what was upstreamed already, before your work.

> Best regards,
> Antheas

Best regards,
Denis

Re: Re: [PATCH] hid-asus-ally: Add full gamepad support

[Derek J. Clark]

Antheas,

Having been one of the many testers of this patch set, having been the one
who added support for it to InputPlumber, and being in the position of
actually having a device in hand to validate the patches in ChimeraOS, I am
fully qualified to address your "concerns".

> Hi Luke,
> thank you for taking the time to reply.
>
> And everyone else, thank you for putting up with my broken line spacing,
> because of Gmail. And perhaps the lack of in-reply-to because of mailto.
> Hopefully in-reply-to works this time ;).
>
> First off, understand that I do not mean to attack your work. I tried to
> make my response helpful to you. In fact, I took a lot of time in writing it
> and preemptively saved you a lot of work in testing your patchset so you do
> not have to spend time reaching the same conclusions that I have had to.
>
> Provided you heed my comments of course, which is not clear from your reply.
>
> As I currently represent what is the largest Linux ROG Ally community, I hope
> it is reasonable that it is in my and my community's best interest that an
> unstable patch which could affect Linux Ally support should be vetted before
> it becomes part of the mainline kernel. Once it becomes part of the kernel,
> workarounds around it will become very hard. We have achieved near perfect
> controller support through userspace, so I would hate to jeopardize that.

I think you might have a misunderstanding of the purpose of kernel space and
user space. Directly implementing many of the features that you have into HHD
as a user level systemd service violates the separation of the kernel and user
protection rings put in place by the kernel architecture. It is not incumbent on
kernel maintainers to respect that just because you have a lot of users,
especially considering that you have created this problem for yourself.

> In my previous email, I gave you my preliminary thoughts without having time
> to test your patchset. Of course, as I noted in that email, I will be testing
> and integrating support for your patchset, personally, with an Ally X unit
> I will be getting access to close to the end of August.
>
> I do not think there is value in arguing, therefore I am not happy continuing
> the discussion under this tone. Hopefully, in a few days you, will have
> another look at my comments, with a level head, and address them.
> I still believe that they are valid and that if they are fixed, I am more
> than happy with your patchset merging into the kernel.
>
> Since you raised some technical points in your response, let me disambiguate.
>
> > You're repeating information that has come directly from me.
>
> Indeed, this specific point (XInput being deprecated) came from you.
> I am just bringing everyone up to speed, since I feel your patch missed
> some important context.
>
> > I have many records from many MCU updates. It doesn't happen. (referring to
> > relying to the endpoint descriptor instead of HID Usages)
>
> In my opinion, using the standard Usage Page and Usage the controller reports
> remains the proper solution (this is what the Windows driver does).
> Remember that if there is a breakage due to a firmware update, users will
> become unable to use their device as they can not update the kernel.
>
> Using `desc.bEndpointAddress` may be appropriate for a userspace tool or an
> out-of-tree kernel driver, but perhaps not for the mainline kernel.
> I am happy to be proven wrong.

Multiple kernel drivers use desc.bEndpointAddress, with subsystems ranging from
i2c, isdn, and multiple usb drivers. If you want some concrete examples of HID
drivers, look at hid_thrustmaster or usbtouchscreen.

> > "Attempted"... I *did*. You've failed to notice that what I've set is
> > what is reported by the HID report.
>
> I am a bit confused here. I thought the purpose of your patch was to convert
> the HID report into what xpad would export. That means respecting xpad, not
> a random HID report.
>
> In this case, the absinfo (with `input_set_abs_params`) needs to be set
> according to what is set by xpad, which is signed and from -32768 to 32767
> (referencing both the Linux Gamepad Specification and the out-of-tree driver
> xpadneo which seems to be the prominent driver providing support for
> controllers similar to Ally X, i.e., Xbox One bluetooth controllers).
>
> I know from experience that Handheld Daemon will have a problem with this.
> But alas, I was not referring to Handheld Daemon being the problem here:
> it is simple enough to fix it in there and I will do it when I add
> support for your patchset (would rather avoid doing it or doing it and having
> to revert it of course). I was moreso referring to other userspace
> applications without this privilege.

Steam has no issues detecting a normal i16 axis range from the u16 provided by
this driver. It isn't an issue with InputPlumber either. This seems to be an
issue you with you not properly detecting and normalizing axes for translation
in your userspace implementation.

> As for why I have to add support for your patchset, it is simply because
> it being there changes the controller mappings, so I simply need to add
> an if statement that uses the standard XInput mappings when it is available.
>
> This is not to say that the end result will be as reliable as without your
> driver, as Handheld Daemon will then be at the mercy of your kernel driver.
> So please, do extended testing and I think with e.g., ChimeraOS 46.2 being
> released yesterday with your patchset, you will get some valuable
> feedback soon enough.

> I know that I have spent multiple weeks already optimizing my implementation,
> having released it close to a month ago. Which is also why I am not in a
> rush to add support for your experimental patchset.

I have dozens of hours of testing done with these patches on both the Ally X
and the original hardware. There aren't any issues that I have found that fall
under the scope of this patch set. Calling it "experimental" is expressly
unfounded, especially so considering you yourself admit you haven't actually
tested it.

> > It is a very different story in a kernel driver... (referring to the 80ms
> > delay used for Xbox+A)
>
> Please understand that I have spent weeks of effort debugging and optimizing
> the Side Menu behavior of Handheld Daemon. In fact, it currently implements
> three different ways of opening the Steam Side Menu (keyboard, extest through
> the gamescope X11 socket, and as a last fallback as Xbox + A). When I say
> 80ms is not enough, I know it is not enough. Otherwise Handheld Daemon would
> be using 80ms. The rest is conjecture.

This is 100% an issue with your software. I just completed an exhaustive battery
of testing at 8w STAPM/FPPT/SPPT on Quiet power profile with only 2 cores
active. The tests included using Steam by itself and the kernel implementation,
as well as running InputPlumber (which also has an 80ms delay) and an
OpenGamepadUI Overlay. In both the kernel implementation and the InputPlumber
implementation there was no difference between the low power mode and running
without restriction on either the Ally or Ally X.

> > It is done on a worker. It is not blocking the kernel....
> > (referring to xbox+a holding a spinlock to send the key combo with msleep)
>
> Repeating myself:
>
> > > In addition, you are freezing the kernel **driver** to send those
> > > commands for 240ms which is around 100 reports.
>
> Which in my opinion will become more like 300ms. Freezing the controller for
> that long is not ideal (I know you are not freezing the kernel).
> Please revisit this.
>
> > Please describe how you think it is broken? (referring to `mcu_powersave`)
>
> Quoting myself:
>
> > > This feature remains broken when the device is at low TDPs and unplugged.
>
> e.g., when the Ally is set on its quiet mode and or is below 12W, and is
> suspended unplugged, with Steam and a game running. In this case,
> the USB controller of the Ally simply does not wake up and RGB breaks.
> The occurrence of this given those conditions is around 40% of the time.
> This includes testing with or without your DMI table patch by the way.

Your RGB interface in HHD is another good example of bypassing the kernel
with userspace. Use the mc_led interface provided by this patch set. It works
the same as the out of tree Ayn and Ayaneo drivers I wrote that you're already
using.

> > Does not work how? (referring to `ally_mcu_usb_switch`)

> Seems like a DMI table always sets it to 0. I do not know why. However I do
> know that as part of our validation on the distribution Bazzite which took
> place prior to you submitting your patch, we tested both adding an or for
> ally x and a dmi table, and the dmi table did not work. Post submitting your
> patch, there was a 5 day brief period where both the unstable ChimeraOS
> kernel and the CachyOs kernel integrated your patch before they also
> integrated the patch I am replying to (which makes Handheld Daemon not work;
> for now). During this period both the original ally and the ally x regressed
> when `mcu_powersave` was disabled.

File a bug report. mcu_powersave is not relevant to this patch series. The LKML
is not a forum to voice grievances over any tangentially related issues you find.

> > I test, mate. With the default kernel and empty userspace.
>
> Unfortunately, this is not how users interact with my kernel patches and
> Handheld Daemon. They usually play games with SteamUI running in the
> background. Often, this includes setting an aggressively low TDP, and multiple
> suspends back-to-back while in game. This is the standard I hold myself to.
> And I would expect no less from a mainline kernel driver.

> I hope I replied to all your technical claims. If I missed any, I am
> happy to clarify and expand where appropriate.
>
> Again, good luck with your patchset. Hopefully, it will merge with 6.12 and
> Ally X owners will get an acceptable result without the need for userspace
> tools (albeit without gyro, back buttons, and RGB being part of the
> controller).

> Best regards,
> Antheas

Preeminent regards,
Derek J. Clark

Re: Re: [PATCH] hid-asus-ally: Add full gamepad support

[Antheas Kapenekakis]

Hi Derek and Denis,

Let us be civil. If I could have bug reported you I would have bug reported
you. However, for some weird coincidence, I do not have access to the
ShadowBlip bug tracker or the relevant communities. Nevertheless, this is
not relevant public discussion. Let us refrain from this discussion further,
including e.g., name-calling.

The MCU of the Ally is the embedded microcontroller that runs the RGB and the
controller of the Ally. Therefore, the discussion of the MCU powersave and
wake is relevant here. What is not proper is that I should also have replied
to the original patch. I admitted that much in my original email. However,
since you are now aware of it, I trust that you can block the patch for
merging until you review it.

If the patch does not function under normal operation, this is relevant here.
It means this extended patchset is built on reliance of the broken patch,
which raises questions. Nevertheless, calling the patchset "experimental"
is hearsay. Therefore, I will refer to it as ambitious from now on :).

> This is 100% an issue with your software. I just completed an exhaustive
> battery of testing at 8w STAPM/FPPT/SPPT on Quiet power profile with only
> 2 cores active. The tests included using Steam by itself and the kernel
> implementation, as well as running InputPlumber (which also has an
> 80ms delay).

Please refrain from name-calling. I was very specific to say that the issue
here is that under load when in a game, Steam will either let A leak through
to the game or not respond, sporadically. While in Steam UI the combination
always works, regardless of TDP. Since you did not test while in a game,
this renders your test invalid.

To save you some additional invalid testing for the other issues: using
ryzenadj on the Ally causes misbehavior, especially after suspend, where the
TDP will reset. In addition, modifying SMP and core parking can freeze the
Ally during suspend. Therefore, for further testing, I expect you to disable
your "PowerStation" application and instead use the low-power platform
profile, which is provided by asus-wmi, and is the vendor specific way for
setting TDP on the Ally. Or use asusctl, which does the same.

As for using direct HID commands to program RGB, asusctl does the same,
including many other userspace apps, and prior to this patchset there was
no way to do different, so I do not see the problem here.

Best,
Antheas

Re: [PATCH] hid-asus-ally: Add full gamepad support

[Denis Benato]

On 12/08/24 10:31, Antheas Kapenekakis wrote:
> Hi Derek and Denis,
> 
> Let us be civil. If I could have bug reported you I would have bug reported
> you. However, for some weird coincidence, I do not have access to the
> ShadowBlip bug tracker or the relevant communities. Nevertheless, this is
> not relevant public discussion. Let us refrain from this discussion further,
> including e.g., name-calling.
Hello,

First and foremost we have been civil, we vigorously stated the truth and
enriched this discussions with information and context you initially left out
(for reasons that at this point are very clear to everyone that has been reading)
and I can assure that we will remain civil as we are not going to make fools
out of ourselves.

We are discussing kernel drivers here, those userspace software are none of your
concerns as those are none of kernel maintainers concerns either. 

> 
> The MCU of the Ally is the embedded microcontroller that runs the RGB and the
> controller of the Ally. Therefore, the discussion of the MCU powersave and
> wake is relevant here. What is not proper is that I should also have replied
> to the original patch. I admitted that much in my original email. However,
> since you are now aware of it, I trust that you can block the patch for
> merging until you review it.
> 

The MCU powersave is a feature driven by the APCI interface and here we are discussing
an USB driver: there is absolutely no reasons to block this patch for a problem you
claim you have found (but never reported) in a totally different driver that even lives
in a totally different subsystem, so if you want those issue solved file the proper bug
report where it is relevant (not here).

> If the patch does not function under normal operation, this is relevant here.
> It means this extended patchset is built on reliance of the broken patch,
> which raises questions. Nevertheless, calling the patchset "experimental"
> is hearsay. Therefore, I will refer to it as ambitious from now on :).
> 

The patch does work under normal operation and it has been tested by many as stated already.

The device firmware performs the wake-up of the device and I don't
see any problem assuming the hardware works to submit a driver:
in fact this assumption is made by every kernel driver.

Again, you claim, without providing evidence, that the hardware is not properly waking up
when a certain ACPI attribute is set to one. Just set it to zero. Over these nine months
you had absolutely no problems is driving the hardware via acpi_call driver and tools
making SMU calls.

If you know a way that I can trigger this problem within my hardware
please, use the relevant mailing list reporting a bug so we can solve
it as we have solved other problems before.

Do not obstruct a fully working kernel driver for reasons unrelated to it.

>> This is 100% an issue with your software. I just completed an exhaustive
>> battery of testing at 8w STAPM/FPPT/SPPT on Quiet power profile with only
>> 2 cores active. The tests included using Steam by itself and the kernel
>> implementation, as well as running InputPlumber (which also has an
>> 80ms delay).
> > Please refrain from name-calling. I was very specific to say that the issue
> here is that under load when in a game, Steam will either let A leak through
> to the game or not respond, sporadically. While in Steam UI the combination
> always works, regardless of TDP. Since you did not test while in a game,
> this renders your test invalid.
> 

I have opened the overlay in game with no problems.

Why should we refrain from name-calling? You started this discussion with the
name of your own software, and in the kernel no anonymous contributions are
allowed either.

Plus, if steam (another userspace software) has a problem with combo keys
recognition and timing we are not entitled to solve that in a kernel driver anyway.

> To save you some additional invalid testing for the other issues: using
> ryzenadj on the Ally causes misbehavior, especially after suspend, where the
> TDP will reset. In addition, modifying SMP and core parking can freeze the
> Ally during suspend. Therefore, for further testing, I expect you to disable
> your "PowerStation" application and instead use the low-power platform
> profile, which is provided by asus-wmi, and is the vendor specific way for
> setting TDP on the Ally. Or use asusctl, which does the same.
> 

All of these information are given to you directly by us nine months ago,
while you were incorrectly driving the hardware via ryzenadj doing exactly
what we told you not to do, so you can expect we know this already.

Here you also make assumptions on how more than four people made their tests,
and while I don't speak for others I can ensure mine were done using the
firmware interface ASUS put in place and is exposed via asus-wmi platform driver:
the driver also in charge of handling the MCU powersave feature you are using as
an unreasonable point of arguments here.

In fact I have been telling you to use that interface for (ance again) more than nine months now,
and you only recently did so in your software because problems we told you were
going to manifest finally manifested, and you have been left with no other choice
but to use the proper way of driving the hardware in question. 

> As for using direct HID commands to program RGB, asusctl does the same,
> including many other userspace apps, and prior to this patchset there was
> no way to do different, so I do not see the problem here.
> 

There is no problem here: your software is driving the reported state inconsistent
with the hardware state and that's a you problem.

Our userspace tooling have been using what the kernel has been exposing, especially
Asusctl that is software written by the same person that contributes to said drivers
and is very well informed about other lkml discussion he partecipates.

> Best,
> Antheas

Best regards,
Denis

Re: Re: Re: [PATCH] hid-asus-ally: Add full gamepad support

[Derek J. Clark]

> Hi Derek and Denis,
> 
> Let us be civil. If I could have bug reported you I would have bug reported
> you. However, for some weird coincidence, I do not have access to the
> ShadowBlip bug tracker or the relevant communities. Nevertheless, this is
> not relevant public discussion. Let us refrain from this discussion further,
> including e.g., name-calling.

I haven't called you any names or levied any personal attacks, so I will
continue not to. I'll also help you, here is the resource for reporting kernel
bugs. You will find the ShadowBlip GitHub is notably absent from it, so I'm not
sure why you brought that up.

https://www.kernel.org/doc/html/v4.19/admin-guide/reporting-bugs.html

> The MCU of the Ally is the embedded microcontroller that runs the RGB and the
> controller of the Ally. Therefore, the discussion of the MCU powersave and
> wake is relevant here. What is not proper is that I should also have replied
> to the original patch. I admitted that much in my original email. However,
> since you are now aware of it, I trust that you can block the patch for
> merging until you review it.
>
> If the patch does not function under normal operation, this is relevant here.
> It means this extended patchset is built on reliance of the broken patch,
> which raises questions. Nevertheless, calling the patchset "experimental"
> is hearsay. Therefore, I will refer to it as ambitious from now on :).

It is not relevant to the functionality provided by this patch, and the bug
exists with or without this patch. The bug cannot be resolved with this patch,
and it is not exacerbated by this patch. Therefore it follows that there is
no reason to block this patch because of the mcu_powersave issue. See above
for how to report the issue properly.

> > This is 100% an issue with your software. I just completed an exhaustive
> > battery of testing at 8w STAPM/FPPT/SPPT on Quiet power profile with only
> > 2 cores active. The tests included using Steam by itself and the kernel
> > implementation, as well as running InputPlumber (which also has an
> > 80ms delay).
> 
> Please refrain from name-calling. I was very specific to say that the issue
> here is that under load when in a game, Steam will either let A leak through
> to the game or not respond, sporadically. While in Steam UI the combination
> always works, regardless of TDP. Since you did not test while in a game,
> this renders your test invalid.
>
> To save you some additional invalid testing for the other issues: using
> ryzenadj on the Ally causes misbehavior, especially after suspend, where the
> TDP will reset. In addition, modifying SMP and core parking can freeze the
> Ally during suspend. Therefore, for further testing, I expect you to disable
> your "PowerStation" application and instead use the low-power platform
> profile, which is provided by asus-wmi, and is the vendor specific way for
> setting TDP on the Ally. Or use asusctl, which does the same.

I should have been more clear about my testing as you have made a lot of
assumptions about how they were conducted. All of my tests were done multiple
times in different games. I chose not to list them all for brevity in the LKML.
PowerStation was disabled for all of tests and I used the sysfs devnodes at
/sys/class/firmware-attributes/asus-armoury/attributes/ to set STAPM/FPPT/SPPT
and asusctl to set the "quiet" power profile. ryzenadj -i was used only to
verify current setting of each PPT before and after sleep to ensure it did not
reset. Also, I only disabled cores to provide a “worst-case” scenario to ensure 
that the test was as comprehensive as possible. These tests meet all of the 
criteria you have specified so thank you for providing a second validation of my 
methodology.

I will point out that the issue of Steam passing events through to a game can
occur when a user has their settings too low to run the game at a reasonable
framerate. In such cases it is recommended they lower their settings or adjust
their power profile to increase performance. Kernel drivers do not need to
account for misbehavior in userspace, especially when those issues are
driven by misconfiguration. In any case, this is a bug in Steam so you should
report it to Valve directly using their SteamForLinux GitHub, rather than on
unrelated kernel patches.

Furthermore, we both know you won't be using the default button combination
and will instead use the included configuration attributes to set the buttons
to the F14-F16 keys you already support so you can determine the discrete
events, just as InputPlumber is doing. This patch will not affect your ability
to use your multiple fallback methods and only provides a usable default
without the use of any userspace tools. Within the scope of the patch and its
purpose there is no issue here as it functions as intended.

> As for using direct HID commands to program RGB, asusctl does the same,
> including many other userspace apps, and prior to this patchset there was
> no way to do different, so I do not see the problem here.
>
> Best,
> Antheas

R/
Derek J. Clark