[PATCH] drm/amdgpu: When the VCN(1.0) block is suspended, powergating is explicitly enabled

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Friday, December 10, 2021 8:25 PM
> To: Gong, Curry <Curry.Gong at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Zhu, James
> <James.Zhu at amd.com>; Liu, Leo <Leo.Liu at amd.com>; Quan, Evan
> <Evan.Quan at amd.com>
> Subject: Re: [PATCH] drm/amdgpu: When the VCN(1.0) block is suspended,
> powergating is explicitly enabled
> 
> 
> 
> On 12/10/2021 5:11 PM, chen gong wrote:
> > 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.
> >
> > [how]
> > In the suspend function of the vcn block, explicitly change the power
> > state flag of the vcn block to POWER_STATE_OFF.
> >
> > Signed-off-by: chen gong <curry.gong at amd.com>
> > ---
> >   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;
> > +
> > +	cancel_success = cancel_delayed_work_sync(&adev-
> >vcn.idle_work);
> > +	if (cancel_success) {
> > +		if (adev->pm.dpm_enabled)
> > +			amdgpu_dpm_enable_uvd(adev, false);
> > +	}
> >
> 
> Probably this is a common issue. Can you try moving this to
> amdgpu_vcn_suspend?
[Quan, Evan] Yes, amdgpu_vcn_suspend seems a more proper place. However, the vcn code is not consistent.
For vcn v2 and later, they already do the manual gate operation in their suspend routine(like vcn_v2_0_stop).
So, it is actually only vcn_v1_0.c suffers this issue.
> 
> if (adev->pm.dpm_enabled)
>     amdgpu_device_ip_set_powergating_state(adev,
> AMD_IP_BLOCK_TYPE_VCN,AMD_PG_STATE_GATE);
> 
> Call this after cancel_delayed_work_sync. Shouldn't have any effect if idle
> work already put it in PG state. Evan, what do you think?
[Quan, Evan] Should not for now. But I'm considering to raise the dev_dbg prompt in amdgpu_dpm_set_powergating_by_smu for double gate/ungate to dev_info or dev_warn.
Hopefully that can help to capture some potential issues like this. So, better to keep the check for the return value of cancel_delayed_work_sync here.

BR
Evan
> 
> Thanks,
> Lijo
> 
> >   	r = vcn_v1_0_hw_fini(adev);
> >   	if (r)
> >