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

[Lazar, Lijo]

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 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.
> 

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