Re: [PATCH v2 4/7] drm/amdgpu: Add suspend function to clear the GPU power profile.

From: Lazar, Lijo
Date: Tue Aug 22 2023 - 08:54:33 EST




On 8/22/2023 5:52 PM, Yadav, Arvind wrote:

On 8/22/2023 12:01 PM, Lazar, Lijo wrote:


On 8/21/2023 12:17 PM, Arvind Yadav wrote:
This patch adds a suspend function that will clear the GPU
power profile before going into suspend state.

v2:
- Add the new suspend function based on review comment.

Cc: Shashank Sharma <shashank.sharma@xxxxxxx>
Cc: Christian Koenig <christian.koenig@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Signed-off-by: Arvind Yadav <Arvind.Yadav@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  2 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c  | 23 +++++++++++++++++++
  drivers/gpu/drm/amd/include/amdgpu_workload.h |  2 ++
  3 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index cd3bf641b630..3b70e657b439 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4212,6 +4212,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
        amdgpu_ras_suspend(adev);
  +    amdgpu_workload_profile_suspend(adev);
+
      amdgpu_device_ip_suspend_phase1(adev);
        if (!adev->in_s0ix)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
index 6367eb88a44d..44ca8e986984 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
@@ -174,6 +174,29 @@ void amdgpu_workload_profile_set(struct amdgpu_device *adev,
      mutex_unlock(&workload->workload_lock);
  }
  +void amdgpu_workload_profile_suspend(struct amdgpu_device *adev)
+{
+    struct amdgpu_smu_workload *workload = &adev->smu_workload;
+    int ret;
+
+    mutex_lock(&workload->workload_lock);
+ cancel_delayed_work_sync(&workload->smu_delayed_work);

Another deadlock candidate. Between fini() and suspend(), the only difference probably could be initialization status. If so, just use a helper that is used during fini() and suspend().

Before going to suspend(), we need to cancel the work and clear all the profiles but in fini() we are destroying the mutex. also it will be called when we are unloading everything.


What I meant is for both suspend/fini, you need to cancel any work scheduled, clear refcounts and set the profile back to default profile. Keep this in a helper and reuse.

Thanks,
Lijo

~Arvind

Thanks,
Lijo

+
+    /* Clear all the set GPU power profile*/
+    for (int index = fls(workload->submit_workload_status);
+         index > 0; index--) {
+        if (workload->submit_workload_status & (1 << index)) {
+ atomic_set(&workload->power_profile_ref[index], 0);
+            ret = amdgpu_power_profile_clear(adev, index);
+            if (ret)
+                DRM_WARN("Failed to clear power profile %s, err = %d\n",
+                     amdgpu_workload_mode_name[index], ret);
+        }
+    }
+    workload->submit_workload_status = 0;
+    mutex_unlock(&workload->workload_lock);
+}
+
  void amdgpu_workload_profile_init(struct amdgpu_device *adev)
  {
      adev->smu_workload.adev = adev;
diff --git a/drivers/gpu/drm/amd/include/amdgpu_workload.h b/drivers/gpu/drm/amd/include/amdgpu_workload.h
index ee1f87257f2d..0acd8769ec52 100644
--- a/drivers/gpu/drm/amd/include/amdgpu_workload.h
+++ b/drivers/gpu/drm/amd/include/amdgpu_workload.h
@@ -52,6 +52,8 @@ void amdgpu_workload_profile_put(struct amdgpu_device *adev,
  void amdgpu_workload_profile_set(struct amdgpu_device *adev,
                   uint32_t ring_type);
  +void amdgpu_workload_profile_suspend(struct amdgpu_device *adev);
+
  void amdgpu_workload_profile_init(struct amdgpu_device *adev);
    void amdgpu_workload_profile_fini(struct amdgpu_device *adev);