On Fri, Dec 10, 2021 at 11:06 AM Quan, Evan <evan.q...@amd.com> wrote:
>
> [AMD Official Use Only]
>
> Hi Curry,
>
> Some nitpicks below. With them fixed, the patch is reviewed-by: Evan Quan
> <evan.q...@amd.com>
>
> @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.g...@amd.com>
> > Sent: Friday, December 10, 2021 7:42 PM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Liu, Leo <leo....@amd.com>; Zhu, James <james....@amd.com>; Quan,
> > Evan <evan.q...@amd.com>; Deucher, Alexander
> > <alexander.deuc...@amd.com>; Gong, Curry <curry.g...@amd.com>
> > 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.g...@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;
> 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
