[PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting

[Christian König]

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 at amd.com>
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 at amd.com>
     Reviewed-by: Guchun Chen <guchun.chen at amd.com>
     Signed-off-by: Alex Deucher <alexander.deucher at amd.com>

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.

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@zn.tnic
>>>
>>> so that it is clear what the whole story way.
>>>
>>> Thx.
>>>
>>