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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d178212d-65ee-a1a0-323b-b9b91931db33@amd.com>
Date:   Tue, 22 Aug 2023 17:52:27 +0530
From:   "Yadav, Arvind" <arvyadav@....com>
To:     "Lazar, Lijo" <lijo.lazar@....com>,
        Arvind Yadav <Arvind.Yadav@....com>, Christian.Koenig@....com,
        alexander.deucher@....com, shashank.sharma@....com,
        Xinhui.Pan@....com, airlied@...il.com, daniel@...ll.ch,
        Felix.Kuehling@....com, amd-gfx@...ts.freedesktop.org
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 4/7] drm/amdgpu: Add suspend function to clear the GPU
 power profile.


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@....com>
>> Cc: Christian Koenig <christian.koenig@....com>
>> Cc: Alex Deucher <alexander.deucher@....com>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@....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.

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