Re: [PATCH v1 3/4] acpi/x86: s2idle: call screen on and off as part of callbacks

[Denis Benato <benato.denis96@gmail.com>]

On 21/09/24 21:44, Antheas Kapenekakis wrote:
> Hi Denis,
> 

Hello Antheas,

>> Beside, as for necessary kernel/software quirks, the new firmware is
>> expected to require none, at least for asus-wmi, so I ask you to leave
>> csee calls where they are now as in the future those will be used only
>> on non-updated firmware.
> I'm happy you said that, as it means this patch will not conflict with
> the new firmware and will just fix older devices.
> 

You are currently attempting to fix a problem that does not exists (or
you haven't made us aware of), in a way that is causing regressions:
no -- calling csee twice is not a problem and we have absolutely no reasons
to suspect it is.

First: the onus is on you to justify, with logs and a thorough explanation,
why you are modifying a module. You have so far failed to do so.

Second: give us explanation of why you think downloading games with screen off
has something to do with the asus-wmi driver and belongs to the same patch series.

> The only change my patch does on the Ally specifically is pull CSEE
> earlier and remove the extra call. There is nothing in this patch to
> explain what you are experiencing. There are a lot of causes I could
> point my finger to, but there is no point.
> 

A compiled version of the kernel tag v6.11 has mcu_powersave=0 working flawlessly,
that very same kernel, compiled with the very same .config, with your patches on
top exhibits what was documented: there is no reason to use fingers here.

So... for one your patches do something else (and they seems to be related to
activities done in background while in s2idle), and two you are saying that
since you don't understand why a regression is happening it is okay to ignore that.

> We will conduct our own testing, and this will include the original
> Ally as well. A lot of them actually, and after initial testing this
> will include thousands of devices, as we plan to fully dogfood this
> patch.
> 

I will be here waiting for the result, when you have identified the reason of regression I
documented, include me in CC or reach me out privately with the work ready to test.

> I was a bit busy today, so I did not update the patch. I want to
> rewrite part of the cover letter, as it includes some inconsistencies,
> and rename some variables. The inconsistencies have to do with how I
> describe the sleep stage, as I read up on some additional
> documentation, it is not related to the contents of the patch. In
> addition, it seems those sleep _DSMs cause problems on the Ally too,
> related to TDP. And no, I will not wait half a year for a BIOS update
> to fix those.
> 
> I am also looking into how to integrate Modern Standby into the
> kernel, in a more full featured way. Downloading games in the
> background is a very requested feature after all, and since looking
> into the Ally's _DSM entries, it seems like it is built to support it.
> Background here would mean the fan will be off and the suspend light
> will be pulsing, so you can safely stow it in a bag while it is
> downloading games. However, this is conjecture until the patch for
> that is built and tested.
> 

Again, userspace software running in s2idle has absolutely nothing to do with
acpi entries. You need to decide what this patch is all about:
If it is about asus-wmi then identify and solve regressions before resubmitting.
If it is about downloading games while sleeping then create a new idle driver
(this way you won't risk breaking what has been confirmed working for months or years),
do a request for comment on a proposal, or propose an interface to the userspace
that applications will use to carry on tasks while hardware is in s2idle
and drop the useless asus-wmi part as it does not belong there.

As it stands this work does not solve any problem and does not allow downloads
to happen while the console is sleeping: pick one and follow that route.

> Antheas

Best regards,
Denis