On 11/9/2021 12:46 PM, Christian König wrote:
Am 08.11.21 um 15:41 schrieb Lazar, Lijo:
On 11/8/2021 7:44 PM, Christian König wrote:
Am 08.11.21 um 12:15 schrieb Borislav Petkov:
On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
Please elaborate the kind of issues.
It fails to reboot on Carrizo-based laptops.
That doesn't necessary sounds like a good idea to me then.
What exactly is going wrong here? And what is the rational that we
must fix this by avoiding updating the current state?
Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now
there is an added logic to power gate the IP as part of suspend
sequence. In case of UVD/VCE, power gating would have already happened
as part of idle work execution.
In any case, power gating is done by SMU FW. The assumption here is -
the logic to power gate IP could involve register programming. AFAIK,
accessing some UVD/VCE regs during powergate state could cause a hang
unless the anti-hang mechanism is not programmed. That means either FW
or driver needs to track the state of IP before accessing those regs
and in this case probably FW is assuming driver to be responsible.
i.e., not to call power off again if it's already powered down.
Though that seems to be a bad assumption on part of FW, it is still a
possibility. Haven't seen the actual FW code, it's a very old program.
Ok guys I've double checked the git history and found that this here is
not as it is intended to be.
See the code in question was just added in August by the following commit:
commit 859e4659273f1df3a23e3990826bcb41e85f68a5
Author: Evan Quan <evan.quan@xxxxxxx>
Date: Thu Aug 19 12:07:59 2021 +0800
drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE suspend
This is a supplement for commit below:
"drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend".
Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
Reviewed-by: Guchun Chen <guchun.chen@xxxxxxx>
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
Before that we where just not touching the UVD power state at all during
suspend and so we won't had a problem in the first place.
So what we should do instead is to just revert commit
859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and
explanation why that is a bad idea.
Yeah, right. If I remember correctly, this commit was originally added
to fix hangs with S3 suspend/resume cycles triggered during video
playback. Reverting could bring back that one. Evan will know more
background details.
Thanks,
Lijo
Regards,
Christian.
Thanks,
Lijo
See we usually assume that updating to the already set state is
unproblematic and that here sounds like just trying to mitigated some
issues instead of fixing the root cause.
Regards,
Christian.
Whoever commits this, pls add
Link: https://lore.kernel.org/r/YV81vidWQLWvATMM@xxxxxxx
so that it is clear what the whole story way.
Thx.
