[PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend

[Alex Deucher]

On Thu, Aug 19, 2021 at 10:15 PM Quan, Evan <Evan.Quan at amd.com> wrote:
>
> [AMD Official Use Only]
>
>
>
>
>
>
>
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Thursday, August 19, 2021 10:36 PM
> To: Zhu, James <James.Zhu at amd.com>; Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Chen, Guchun <Guchun.Chen at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>
> Subject: RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
>
>
>
> [AMD Official Use Only]
>
>
>
> If that is done  –
>
>
>
> +               amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +                                                      AMD_PG_STATE_GATE);
> +               amdgpu_device_ip_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +                                                      AMD_CG_STATE_GATE);
>
>
>
> Usual order is CG followed by PG. It comes in the else part, so less likely to happen. Nice to fix for code correctness purpose.
>
> [Quan, Evan] Thanks Lijo. Make sense to me. However, actually these code were copied from amdgpu_uvd_idle_work_handler() of amdgpu_uvd.c. Same logic was used there. So, maybe @Zhu, James or @Liu, Leo can share some insights about this.
>

It looks like it is wrong there as well.  We should be gating the
clocks before the power.  The order is also wrong in
amdgpu_uvd_ring_begin_use().  We need to ungate the power before the
clocks

Alex


>
>
> BR
>
> Evan
>
>
>
> Thanks,
>
> Lijo
>
>
>
> From: Zhu, James <James.Zhu at amd.com>
> Sent: Thursday, August 19, 2021 7:49 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Chen, Guchun <Guchun.Chen at amd.com>; Lazar, Lijo <Lijo.Lazar at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>
> Subject: Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
>
>
>
> [AMD Official Use Only]
>
>
>
>
>
> Why not move changes into hw_fini?
>
>
>
> Best Regards!
>
>
>
> James Zhu
>
> ________________________________
>
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of Evan Quan <evan.quan at amd.com>
> Sent: Wednesday, August 18, 2021 11:08 PM
> To: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Chen, Guchun <Guchun.Chen at amd.com>; Lazar, Lijo <Lijo.Lazar at amd.com>; Quan, Evan <Evan.Quan at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>
> Subject: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
>
>
>
> Perform proper cleanups on UVD/VCE suspend: powergate enablement,
> clockgating enablement and dpm disablement. This can fix some hangs
> observed on suspending when UVD/VCE still using(e.g. issue
> "pm-suspend" when video is still playing).
>
> Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> Signed-off-by: xinhui pan <xinhui.pan at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 23 +++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index 4eebf973a065..d0fc6ec18c29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -554,6 +554,30 @@ static int uvd_v6_0_suspend(void *handle)
>          int r;
>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> +       /*
> +        * Proper cleanups before halting the HW engine:
> +        *   - cancel the delayed idle work
> +        *   - enable powergating
> +        *   - enable clockgating
> +        *   - disable dpm
> +        *
> +        * TODO: to align with the VCN implementation, move the
> +        * jobs for clockgating/powergating/dpm setting to
> +        * ->set_powergating_state().
> +        */
> +       cancel_delayed_work_sync(&adev->uvd.idle_work);
> +
> +       if (adev->pm.dpm_enabled) {
> +               amdgpu_dpm_enable_uvd(adev, false);
> +       } else {
> +               amdgpu_asic_set_uvd_clocks(adev, 0, 0);
> +               /* shutdown the UVD block */
> +               amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +                                                      AMD_PG_STATE_GATE);
> +               amdgpu_device_ip_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_UVD,
> +                                                      AMD_CG_STATE_GATE);
> +       }
> +
>          r = uvd_v6_0_hw_fini(adev);
>          if (r)
>                  return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> index 6d9108fa22e0..a594ade5d30a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> @@ -503,6 +503,29 @@ static int vce_v3_0_suspend(void *handle)
>          int r;
>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> +       /*
> +        * Proper cleanups before halting the HW engine:
> +        *   - cancel the delayed idle work
> +        *   - enable powergating
> +        *   - enable clockgating
> +        *   - disable dpm
> +        *
> +        * TODO: to align with the VCN implementation, move the
> +        * jobs for clockgating/powergating/dpm setting to
> +        * ->set_powergating_state().
> +        */
> +       cancel_delayed_work_sync(&adev->vce.idle_work);
> +
> +       if (adev->pm.dpm_enabled) {
> +               amdgpu_dpm_enable_vce(adev, false);
> +       } else {
> +               amdgpu_asic_set_vce_clocks(adev, 0, 0);
> +               amdgpu_device_ip_set_powergating_state(adev, AMD_IP_BLOCK_TYPE_VCE,
> +                                                      AMD_PG_STATE_GATE);
> +               amdgpu_device_ip_set_clockgating_state(adev, AMD_IP_BLOCK_TYPE_VCE,
> +                                                      AMD_CG_STATE_GATE);
> +       }
> +
>          r = vce_v3_0_hw_fini(adev);
>          if (r)
>                  return r;
> --
> 2.29.0