[AMD Official Use Only] > -----Original Message----- > From: Lazar, Lijo <Lijo.Lazar@xxxxxxx> > Sent: Tuesday, November 9, 2021 3:29 PM > To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Borislav Petkov > <bp@xxxxxxx>; Paul Menzel <pmenzel@xxxxxxxxxxxxx>; Liu, Leo > <Leo.Liu@xxxxxxx> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Quan, Evan > <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx > 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@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. [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@xxxxxxx > >>>> > >>>> so that it is clear what the whole story way. > >>>> > >>>> Thx. > >>>> > >>> > >
