Message-ID: <20240812201404.15484-1-derekjohn.clark@gmail.com>
Date: Mon, 12 Aug 2024 13:12:08 -0700
From: "Derek J. Clark" <derekjohn.clark@...il.com>
To: lkml@...heas.dev
Cc: luke@...nes.dev,
	bentiss@...nel.org,
	jikos@...nel.org,
	linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Derek J . Clark" <derekjohn.clark@...il.com>
Subject: Re: Re: Re: [PATCH] hid-asus-ally: Add full gamepad support

> 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

Powered by blists - more mailing lists