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

["Lazar, Lijo" <lijo.lazar@amd.com>]

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.

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