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

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Tuesday, November 9, 2021 3:29 PM
> To: Koenig, Christian <Christian.Koenig at amd.com>; Borislav Petkov
> <bp at suse.de>; Paul Menzel <pmenzel at molgen.mpg.de>; Liu, Leo
> <Leo.Liu at amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
> <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> 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.
[Quan, Evan] Yes, 859e4659273f1df3a23e3990826bcb41e85f68a5 aimed the issue below. It cannot be reverted.
"If the UVD/VCE is still using on reboot/suspend triggered, idle work will be not triggered and the add-on power gate logic can help to put the Ips into correct gate state."

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