On Fri, Dec 10, 2021 at 11:06 AM Quan, Evan <Evan.Quan@xxxxxxx> wrote: > > [AMD Official Use Only] > > Hi Curry, > > Some nitpicks below. With them fixed, the patch is reviewed-by: Evan Quan <evan.quan@xxxxxxx> > > @Deucher, Alexander this should be able address the issue reported by https://gitlab.freedesktop.org/drm/amd/-/issues/1828. Can you help to confirm this? Yes, the patch works according to the reporter. Alex > > BR > Evan > > -----Original Message----- > > From: chen gong <curry.gong@xxxxxxx> > > Sent: Friday, December 10, 2021 7:42 PM > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > Cc: Liu, Leo <Leo.Liu@xxxxxxx>; Zhu, James <James.Zhu@xxxxxxx>; Quan, > > Evan <Evan.Quan@xxxxxxx>; Deucher, Alexander > > <Alexander.Deucher@xxxxxxx>; Gong, Curry <Curry.Gong@xxxxxxx> > > Subject: [PATCH] drm/amdgpu: When the VCN(1.0) block is suspended, > > powergating is explicitly enabled > > > > Play a video on the raven (or PCO, raven2) platform, and then do the S3 > > test. When resume, the following error will be reported: > > > > amdgpu 0000:02:00.0: [drm:amdgpu_ring_test_helper [amdgpu]] *ERROR* > > ring > > vcn_dec test failed (-110) > > [drm:amdgpu_device_ip_resume_phase2 [amdgpu]] *ERROR* resume of IP > > block > > <vcn_v1_0> failed -110 > > amdgpu 0000:02:00.0: amdgpu: amdgpu_device_ip_resume failed (-110). > > PM: dpm_run_callback(): pci_pm_resume+0x0/0x90 returns -110 > > > > [why] > > When playing the video: The power state flag of the vcn block is set to > > POWER_STATE_ON. > > > > When doing suspend: There is no change to the power state flag of the > > vcn block, it is still POWER_STATE_ON. > > > > When doing resume: Need to open the power gate of the vcn block and set > > the power state flag of the VCN block to POWER_STATE_ON. > > But at this time, the power state flag of the vcn block is already > > POWER_STATE_ON. The power status flag check in the "8f2cdef > > drm/amd/pm: > > avoid duplicate powergate/ungate setting" patch will return the > > amdgpu_dpm_set_powergating_by_smu function directly. > > As a result, the gate of the power was not opened, causing the > > subsequent ring test to fail. > Better to update this as some descriptions below: > adev-> vcn.idle_work will be triggered when the VCN ring idle for 1S. And we rely on it to do the VCN gate(power down). > However, if the VCN ring is still using on suspend kicked, there will be no chance for adev->vcn.idle_work to do the VCN gate. > That will make driver wrongly treat VCN as ungated(power on) and prevent further VCN ungate(power on) operation(which is actually needed) on resume. > > > > [how] > > In the suspend function of the vcn block, explicitly change the power > > state flag of the vcn block to POWER_STATE_OFF. > Maybe some descriptions as below is better: > Manually do the VCN gate(power down) in the suspend routine if adev->vcn.idle_work does not(have no chance) do that. > > > > Signed-off-by: chen gong <curry.gong@xxxxxxx> > > --- > > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > index d54d720..d73676b 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > @@ -246,6 +246,13 @@ static int vcn_v1_0_suspend(void *handle) > > { > > int r; > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > + bool cancel_success; > This seems not a good naming since the cancel always succeed. The difference is whether the idle_work get actually executed. > Better to rename it as "idle_work_unexecuted" or just "vcn_not_gated"? > > + > > + cancel_success = cancel_delayed_work_sync(&adev- > > >vcn.idle_work); > > + if (cancel_success) { > > + if (adev->pm.dpm_enabled) > > + amdgpu_dpm_enable_uvd(adev, false); > > + } > > > > r = vcn_v1_0_hw_fini(adev); > > if (r) > > -- > > 2.7.4
