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

From: "Christian König" <christian.koenig@amd.com>
To: "Lazar, Lijo" <lijo.lazar@amd.com>, Borislav Petkov <bp@suse.de>,
	Paul Menzel <pmenzel@molgen.mpg.de>, "Liu, Leo" <Leo.Liu@amd.com>
Cc: Alexander.Deucher@amd.com, Evan Quan <evan.quan@amd.com>,
	amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting
Date: Tue, 9 Nov 2021 08:44:11 +0100	[thread overview]
Message-ID: <dd7aaabf-0c32-1db4-0105-c6181a8923b3@amd.com> (raw)
In-Reply-To: <00fc5d97-5228-1935-a544-91f2a9b1b803@amd.com>

Am 09.11.21 um 08:28 schrieb Lazar, Lijo:
> [SNIP]
>>
>> 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@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@amd.com>
>>      Reviewed-by: Guchun Chen <guchun.chen@amd.com>
>>      Signed-off-by: Alex Deucher <alexander.deucher@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.

Exactly that's my memory as well. So what happens here is that we go in 
circles with one patch fixing a bug and the next patch effectively 
reverting it again to fix another bug.

What we need to do is stop this madness and take a look at the 
underlying problems instead of trying to work around them.

Thanks,
Christian.

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