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

["Yadav, Arvind" <arvyadav@amd.com>]

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);