Messages in this thread | | | Date | Tue, 22 Aug 2023 18:26:57 +0530 | Subject | Re: [PATCH v2 4/7] drm/amdgpu: Add suspend function to clear the GPU power profile. | From | "Yadav, Arvind" <> |
| |
On 8/22/2023 6:24 PM, Lazar, Lijo wrote: > > > 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@amd.com> >>>> Cc: Christian Koenig <christian.koenig@amd.com> >>>> Cc: Alex Deucher <alexander.deucher@amd.com> >>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> >>>> --- >>>> 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. > Noted.
Thank you, ~Arvind
> 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);
| |