Re: [PATCH 0/3] On AMD platforms only offer s2idle w/ proper FW

From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>,
	"Natikar, Basavaraj" <Basavaraj.Natikar@amd.com>,
	"bjoern.daase@gmail.com" <bjoern.daase@gmail.com>
Subject: Re: [PATCH 0/3] On AMD platforms only offer s2idle w/ proper FW
Date: Tue, 11 Jan 2022 13:35:46 -0600	[thread overview]
Message-ID: <3e09ef49-6645-59d8-5e66-1640c4ca91de@amd.com> (raw)
In-Reply-To: <CAJZ5v0jiKfFE1pFS-vGefYpHdsUvRxrzUY-dbyvkPA1Ehssg-g@mail.gmail.com>

>>
>> That's why I thought my patch series made sense.
>>
>> I guess another (antisocial) response would be to to return false when
>> the suspend callback for amdgpu happens and dev_err mentioning that
>> firmware support is needed for suspend.
> 
> If you want to avoid exposing s2idle at all in some cases, you need to
> change the sysfs I/F in /sys/power/ so that it doesn't expose "s2idle"
> in mem_sleep or "freeze" in state at all.  

Right now they make it so that s2idle isn't exposed in "mem_sleep", but 
"freeze" is still exposed in "state".

I'd be worried about breaking userspace with taking "freeze" out of state.

> That's much more intrusive
> than just failing every s2idle attempt (which is what the patches do
> AFAICS)
As they stand right now when the BIOS is set to S3:
# cat /sys/power/mem_sleep
[deep]

# echo freeze > /sys/power/state
Returns a failure though.

And then when it's set to Modern Standby :

# cat /sys/power/mem_sleep
[s2idle]

# echo freeze > /sys/power/state
Works

> 
> Do you want to do that for platforms supporting S3, or for platforms
> that don't support any kind of suspend at all?

You know since this series went out 
6dc8265f9803ccb7e5da804e01601f0c14f270e0 was merged.  This will probably
have cleaned up the problem state that Bjoern found in the first place.

So this is really a philosophical discussion now if it's worth offering 
s2idle in /sys/power/mem_sleep on AMD systems.

Admittedly this has been an APU notebook oriented discussion.  Platforms 
that don't support any kind of suspend (like servers) will get caught up 
in this and could no longer do s2idle either.

With the assumption that 6dc8265f9803ccb7e5da804e01601f0c14f270e0 helps 
the problem state I'm leaning back on we should add some warnings to let 
people know how to help themselves for power consumption if they did this.

We can probably put those in amdgpu though.