On 8/17/2021 3:13 PM, Quan, Evan wrote:
[AMD Official Use Only]
+Leo to share his insights
-----Original Message-----
From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
Sent: Tuesday, August 17, 2021 3:28 PM
To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Chen, Guchun
<Guchun.Chen@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE
on suspend
On 8/17/2021 12:10 PM, Evan Quan wrote:
If the powergating of UVD/VCE is in process, wait for its completion
before proceeding(suspending). 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@xxxxxxx>
Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 +++++
drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 5 +++++
2 files changed, 10 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..2fdce572baeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -554,6 +554,11 @@ static int uvd_v6_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+ /*
+ * If the powergating is in process, wait for its completion.
+ */
+ flush_delayed_work(&adev->uvd.idle_work);
+
If running idle is a prerequisite before going to suspend, then something else
is missing here.
Otherwise, the hang looks more like a pending work launched after
hardware is suspended and trying to access hardware. As the hardware is
going to be suspended anyway, doesn't it work with
cancel_delayed_work_sync - making sure that nothing is going to be
launched later to access hardware?
[Quan, Evan] The reason we chose flush_delayed_work instead of cancel_delayed_work_sync is we think those operations performed in idle_work(dpm disablement, powergating) seems needed considering the action is 'suspend'. So, instead of "cancel", maybe waiting for them completion is more proper.
But it will do so only if the work is scheduled - so it doesn't seem to
be a prerequisite for suspend. If it was a prerequisite, then the
existing code is missing that (so that it gets done for all cases).
Then this may be a potential issue for other suspend calls also where work is
pending to be launched when hardware is suspended.
[Quan, Evan] Do you mean we need to check whether there is similar issue for other IPs?
Yes, if there are cases where other IPs may schedule a delayed work and
call hw_fini without cancelling the work.
Thanks,
Lijo
BR
Evan
Thanks,
Lijo
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..f0adecd5ec0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
@@ -503,6 +503,11 @@ static int vce_v3_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+ /*
+ * If the powergating is in process, wait for its completion.
+ */
+ flush_delayed_work(&adev->vce.idle_work);
+
r = vce_v3_0_hw_fini(adev);
if (r)
return r;
