[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? 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
