* [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
@ 2021-08-19 3:08 Evan Quan
2021-08-19 4:30 ` Chen, Guchun
2021-08-19 14:19 ` Zhu, James
0 siblings, 2 replies; 9+ messages in thread
From: Evan Quan @ 2021-08-19 3:08 UTC (permalink / raw)
To: amd-gfx; +Cc: Alexander.Deucher, Guchun.Chen, Lijo.Lazar, Evan Quan, xinhui pan
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@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-19 3:08 [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend Evan Quan
@ 2021-08-19 4:30 ` Chen, Guchun
2021-08-19 14:19 ` Zhu, James
1 sibling, 0 replies; 9+ messages in thread
From: Chen, Guchun @ 2021-08-19 4:30 UTC (permalink / raw)
To: Quan, Evan, amd-gfx, Liu, Leo, Zhu, James
Cc: Deucher, Alexander, Lazar, Lijo, Pan, Xinhui
[Public]
+Leo and James to review as well.
This patch is:
Acked-by: Guchun Chen <guchun.chen@amd.com>
Regards,
Guchun
-----Original Message-----
From: Quan, Evan <Evan.Quan@amd.com>
Sent: Thursday, August 19, 2021 11:09 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Quan, Evan <Evan.Quan@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-19 3:08 [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend Evan Quan
2021-08-19 4:30 ` Chen, Guchun
@ 2021-08-19 14:19 ` Zhu, James
2021-08-19 14:36 ` Lazar, Lijo
2021-08-20 2:02 ` Quan, Evan
1 sibling, 2 replies; 9+ messages in thread
From: Zhu, James @ 2021-08-19 14:19 UTC (permalink / raw)
To: Quan, Evan, amd-gfx
Cc: Deucher, Alexander, Chen, Guchun, Lazar, Lijo, Pan, Xinhui
[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]
[AMD Official Use Only]
Why not move changes into hw_fini?
Best Regards!
James Zhu
________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Evan Quan <evan.quan@amd.com>
Sent: Wednesday, August 18, 2021 11:08 PM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Quan, Evan <Evan.Quan@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@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
[-- Attachment #2: Type: text/html, Size: 9285 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-19 14:19 ` Zhu, James
@ 2021-08-19 14:36 ` Lazar, Lijo
2021-08-20 2:15 ` Quan, Evan
2021-08-20 2:02 ` Quan, Evan
1 sibling, 1 reply; 9+ messages in thread
From: Lazar, Lijo @ 2021-08-19 14:36 UTC (permalink / raw)
To: Zhu, James, Quan, Evan, amd-gfx
Cc: Deucher, Alexander, Chen, Guchun, Pan, Xinhui
[-- Attachment #1: Type: text/plain, Size: 5354 bytes --]
[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.
Thanks,
Lijo
From: Zhu, James <James.Zhu@amd.com>
Sent: Thursday, August 19, 2021 7:49 PM
To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Pan, Xinhui <Xinhui.Pan@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@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Evan Quan <evan.quan@amd.com<mailto:evan.quan@amd.com>>
Sent: Wednesday, August 18, 2021 11:08 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Chen, Guchun <Guchun.Chen@amd.com<mailto:Guchun.Chen@amd.com>>; Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>; Quan, Evan <Evan.Quan@amd.com<mailto:Evan.Quan@amd.com>>; Pan, Xinhui <Xinhui.Pan@amd.com<mailto:Xinhui.Pan@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@amd.com<mailto:evan.quan@amd.com>>
Signed-off-by: xinhui pan <xinhui.pan@amd.com<mailto:xinhui.pan@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
[-- Attachment #2: Type: text/html, Size: 13960 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-19 14:19 ` Zhu, James
2021-08-19 14:36 ` Lazar, Lijo
@ 2021-08-20 2:02 ` Quan, Evan
1 sibling, 0 replies; 9+ messages in thread
From: Quan, Evan @ 2021-08-20 2:02 UTC (permalink / raw)
To: Zhu, James, amd-gfx
Cc: Deucher, Alexander, Chen, Guchun, Lazar, Lijo, Pan, Xinhui
[-- Attachment #1: Type: text/plain, Size: 4956 bytes --]
[AMD Official Use Only]
From: Zhu, James <James.Zhu@amd.com>
Sent: Thursday, August 19, 2021 10:19 PM
To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Pan, Xinhui <Xinhui.Pan@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?
[Quan, Evan] Sure, I can do that. Will send out a new patch with that updated.
BR
Evan
Best Regards!
James Zhu
________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Evan Quan <evan.quan@amd.com<mailto:evan.quan@amd.com>>
Sent: Wednesday, August 18, 2021 11:08 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Chen, Guchun <Guchun.Chen@amd.com<mailto:Guchun.Chen@amd.com>>; Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>; Quan, Evan <Evan.Quan@amd.com<mailto:Evan.Quan@amd.com>>; Pan, Xinhui <Xinhui.Pan@amd.com<mailto:Xinhui.Pan@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@amd.com<mailto:evan.quan@amd.com>>
Signed-off-by: xinhui pan <xinhui.pan@amd.com<mailto:xinhui.pan@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
[-- Attachment #2: Type: text/html, Size: 12994 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-19 14:36 ` Lazar, Lijo
@ 2021-08-20 2:15 ` Quan, Evan
2021-08-20 14:23 ` Alex Deucher
0 siblings, 1 reply; 9+ messages in thread
From: Quan, Evan @ 2021-08-20 2:15 UTC (permalink / raw)
To: Lazar, Lijo, Zhu, James, amd-gfx, Liu, Leo
Cc: Deucher, Alexander, Chen, Guchun, Pan, Xinhui
[-- Attachment #1: Type: text/plain, Size: 6285 bytes --]
[AMD Official Use Only]
From: Lazar, Lijo <Lijo.Lazar@amd.com>
Sent: Thursday, August 19, 2021 10:36 PM
To: Zhu, James <James.Zhu@amd.com>; Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@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<mailto:James.Zhu@amd.com> or @Liu, Leo<mailto:Leo.Liu@amd.com> can share some insights about this.
BR
Evan
Thanks,
Lijo
From: Zhu, James <James.Zhu@amd.com<mailto:James.Zhu@amd.com>>
Sent: Thursday, August 19, 2021 7:49 PM
To: Quan, Evan <Evan.Quan@amd.com<mailto:Evan.Quan@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Chen, Guchun <Guchun.Chen@amd.com<mailto:Guchun.Chen@amd.com>>; Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>; Pan, Xinhui <Xinhui.Pan@amd.com<mailto:Xinhui.Pan@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@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Evan Quan <evan.quan@amd.com<mailto:evan.quan@amd.com>>
Sent: Wednesday, August 18, 2021 11:08 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Chen, Guchun <Guchun.Chen@amd.com<mailto:Guchun.Chen@amd.com>>; Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>; Quan, Evan <Evan.Quan@amd.com<mailto:Evan.Quan@amd.com>>; Pan, Xinhui <Xinhui.Pan@amd.com<mailto:Xinhui.Pan@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@amd.com<mailto:evan.quan@amd.com>>
Signed-off-by: xinhui pan <xinhui.pan@amd.com<mailto:xinhui.pan@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
[-- Attachment #2: Type: text/html, Size: 16261 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-20 2:15 ` Quan, Evan
@ 2021-08-20 14:23 ` Alex Deucher
2021-08-23 7:59 ` Quan, Evan
0 siblings, 1 reply; 9+ messages in thread
From: Alex Deucher @ 2021-08-20 14:23 UTC (permalink / raw)
To: Quan, Evan
Cc: Lazar, Lijo, Zhu, James, amd-gfx, Liu, Leo, Deucher, Alexander,
Chen, Guchun, Pan, Xinhui
On Thu, Aug 19, 2021 at 10:15 PM Quan, Evan <Evan.Quan@amd.com> wrote:
>
> [AMD Official Use Only]
>
>
>
>
>
>
>
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, August 19, 2021 10:36 PM
> To: Zhu, James <James.Zhu@amd.com>; Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
> Sent: Thursday, August 19, 2021 7:49 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Pan, Xinhui <Xinhui.Pan@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@lists.freedesktop.org> on behalf of Evan Quan <evan.quan@amd.com>
> Sent: Wednesday, August 18, 2021 11:08 PM
> To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Quan, Evan <Evan.Quan@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-20 14:23 ` Alex Deucher
@ 2021-08-23 7:59 ` Quan, Evan
2021-08-23 13:30 ` Alex Deucher
0 siblings, 1 reply; 9+ messages in thread
From: Quan, Evan @ 2021-08-23 7:59 UTC (permalink / raw)
To: Alex Deucher
Cc: Lazar, Lijo, Zhu, James, amd-gfx, Liu, Leo, Deucher, Alexander,
Chen, Guchun, Pan, Xinhui
[AMD Official Use Only]
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Friday, August 20, 2021 10:23 PM
> To: Quan, Evan <Evan.Quan@amd.com>
> Cc: Lazar, Lijo <Lijo.Lazar@amd.com>; Zhu, James <James.Zhu@amd.com>;
> amd-gfx@lists.freedesktop.org; Liu, Leo <Leo.Liu@amd.com>; Deucher,
> Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12
> UVD/VCE on suspend
>
> On Thu, Aug 19, 2021 at 10:15 PM Quan, Evan <Evan.Quan@amd.com> wrote:
> >
> > [AMD Official Use Only]
> >
> >
> >
> >
> >
> >
> >
> > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > Sent: Thursday, August 19, 2021 10:36 PM
> > To: Zhu, James <James.Zhu@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>;
> > amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@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
[Quan, Evan] I created a patch for this. But during the verification, I got the errors below
[ 87.420822] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 88.443029] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 89.465386] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 90.487629] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 91.510380] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 92.533782] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 93.557400] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 94.580708] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 95.603832] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 96.627727] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
[ 96.657453] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, giving up!!!
[ 96.665892] [drm:amdgpu_device_ip_set_powergating_state [amdgpu]] *ERROR* set_powergating_state of IP block <uvd_v6_0> failed -1
[ 97.697422] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd (-110).
[ 98.721432] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd_enc0 (-110).
[ 99.745407] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd_enc1 (-110).
[ 99.857784] [drm:amdgpu_device_delayed_init_work_handler [amdgpu]] *ERROR* ib ring test failed (-110).
After checking the related source code roughly. It seems the underlaying implementation of -> set_powergating_state(e.g. uvd_v6_0_set_powergating_state ) performs more jobs than just power gating. And I guess maybe some of those jobs needs to be performed after -> set_clockgating_state. James and Leo may comment more.
BR
Evan
>
> Alex
>
>
> >
> >
> > BR
> >
> > Evan
> >
> >
> >
> > Thanks,
> >
> > Lijo
> >
> >
> >
> > From: Zhu, James <James.Zhu@amd.com>
> > Sent: Thursday, August 19, 2021 7:49 PM
> > To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Pan, Xinhui
> > <Xinhui.Pan@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@lists.freedesktop.org> on behalf of
> > Evan Quan <evan.quan@amd.com>
> > Sent: Wednesday, August 18, 2021 11:08 PM
> > To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Quan, Evan
> > <Evan.Quan@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
> > Signed-off-by: xinhui pan <xinhui.pan@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend
2021-08-23 7:59 ` Quan, Evan
@ 2021-08-23 13:30 ` Alex Deucher
0 siblings, 0 replies; 9+ messages in thread
From: Alex Deucher @ 2021-08-23 13:30 UTC (permalink / raw)
To: Quan, Evan
Cc: Lazar, Lijo, Zhu, James, amd-gfx, Liu, Leo, Deucher, Alexander,
Chen, Guchun, Pan, Xinhui
On Mon, Aug 23, 2021 at 3:59 AM Quan, Evan <Evan.Quan@amd.com> wrote:
>
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com>
> > Sent: Friday, August 20, 2021 10:23 PM
> > To: Quan, Evan <Evan.Quan@amd.com>
> > Cc: Lazar, Lijo <Lijo.Lazar@amd.com>; Zhu, James <James.Zhu@amd.com>;
> > amd-gfx@lists.freedesktop.org; Liu, Leo <Leo.Liu@amd.com>; Deucher,
> > Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
> > Subject: Re: [PATCH] drm/amdgpu: add missing cleanups for Polaris12
> > UVD/VCE on suspend
> >
> > On Thu, Aug 19, 2021 at 10:15 PM Quan, Evan <Evan.Quan@amd.com> wrote:
> > >
> > > [AMD Official Use Only]
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > > Sent: Thursday, August 19, 2021 10:36 PM
> > > To: Zhu, James <James.Zhu@amd.com>; Quan, Evan
> > <Evan.Quan@amd.com>;
> > > amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > > <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@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
> [Quan, Evan] I created a patch for this. But during the verification, I got the errors below
> [ 87.420822] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 88.443029] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 89.465386] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 90.487629] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 91.510380] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 92.533782] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 93.557400] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 94.580708] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 95.603832] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 96.627727] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, trying to reset the VCPU!!!
> [ 96.657453] [drm:uvd_v6_0_start [amdgpu]] *ERROR* UVD not responding, giving up!!!
> [ 96.665892] [drm:amdgpu_device_ip_set_powergating_state [amdgpu]] *ERROR* set_powergating_state of IP block <uvd_v6_0> failed -1
> [ 97.697422] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd (-110).
> [ 98.721432] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd_enc0 (-110).
> [ 99.745407] amdgpu 0000:02:00.0: [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on uvd_enc1 (-110).
> [ 99.857784] [drm:amdgpu_device_delayed_init_work_handler [amdgpu]] *ERROR* ib ring test failed (-110).
>
> After checking the related source code roughly. It seems the underlaying implementation of -> set_powergating_state(e.g. uvd_v6_0_set_powergating_state ) performs more jobs than just power gating. And I guess maybe some of those jobs needs to be performed after -> set_clockgating_state. James and Leo may comment more.
>
Ok. Thanks for checking. I have no objections to the patch as is.
We can address the ordering questions later.
Alex
> BR
> Evan
> >
> > Alex
> >
> >
> > >
> > >
> > > BR
> > >
> > > Evan
> > >
> > >
> > >
> > > Thanks,
> > >
> > > Lijo
> > >
> > >
> > >
> > > From: Zhu, James <James.Zhu@amd.com>
> > > Sent: Thursday, August 19, 2021 7:49 PM
> > > To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > > <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Pan, Xinhui
> > > <Xinhui.Pan@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@lists.freedesktop.org> on behalf of
> > > Evan Quan <evan.quan@amd.com>
> > > Sent: Wednesday, August 18, 2021 11:08 PM
> > > To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
> > > <Guchun.Chen@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Quan, Evan
> > > <Evan.Quan@amd.com>; Pan, Xinhui <Xinhui.Pan@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@amd.com>
> > > Signed-off-by: xinhui pan <xinhui.pan@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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-23 13:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 3:08 [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend Evan Quan
2021-08-19 4:30 ` Chen, Guchun
2021-08-19 14:19 ` Zhu, James
2021-08-19 14:36 ` Lazar, Lijo
2021-08-20 2:15 ` Quan, Evan
2021-08-20 14:23 ` Alex Deucher
2021-08-23 7:59 ` Quan, Evan
2021-08-23 13:30 ` Alex Deucher
2021-08-20 2:02 ` Quan, Evan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.