Re: [PATCH 1/2] drm/amdgpu/gfx: adjust workload profile handling

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

On 3/14/2025 8:28 PM, Alex Deucher wrote:
> On Fri, Mar 14, 2025 at 10:53 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>>
>>
>>
>> On 3/14/2025 7:17 PM, Alex Deucher wrote:
>>> No need to make the workload profile setup dependent
>>> on the results of cancelling the delayed work thread.
>>> We have all of the necessary checking in place for the
>>> workload profile reference counting, so separate the
>>> two.  As it is now, we can theoretically end up with
>>> the call from begin_use happening while the worker
>>> thread is executing which would result in the profile
>>> not getting set for that submission.  It should not
>>> affect the reference counting.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index 099329d15b9ff..20424f8c4925f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -2188,18 +2188,18 @@ void amdgpu_gfx_profile_ring_begin_use(struct amdgpu_ring *ring)
>>>
>>>       atomic_inc(&adev->gfx.total_submission_cnt);
>>>
>>> -     if (!cancel_delayed_work_sync(&adev->gfx.idle_work)) {
>>> -             mutex_lock(&adev->gfx.workload_profile_mutex);
>>> -             if (!adev->gfx.workload_profile_active) {
>>> -                     r = amdgpu_dpm_switch_power_profile(adev, profile, true);
>>> -                     if (r)
>>> -                             dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
>>> -                                      profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
>>> -                                      "fullscreen 3D" : "compute");
>>> -                     adev->gfx.workload_profile_active = true;
>>> -             }
>>> -             mutex_unlock(&adev->gfx.workload_profile_mutex);
>>> +     cancel_delayed_work_sync(&adev->gfx.idle_work);
>>> +
>>
>> To avoid locking/unlocking mutex for each begin_use, I think here we
>> could do like
>>
>> if (adev->gfx.workload_profile_active)
>>         return;
> 
> But that is what the mutex is protecting.
> 

I think once we cancelled the work, there is no one to turn it to false.
We don't mind if somebody else changed to true already.

Thanks,
Lijo

> Alex
> 
>>
>> Thanks,
>> Lijo
>>
>>> +     mutex_lock(&adev->gfx.workload_profile_mutex);
>>> +     if (!adev->gfx.workload_profile_active) {
>>> +             r = amdgpu_dpm_switch_power_profile(adev, profile, true);
>>> +             if (r)
>>> +                     dev_warn(adev->dev, "(%d) failed to disable %s power profile mode\n", r,
>>> +                              profile == PP_SMC_POWER_PROFILE_FULLSCREEN3D ?
>>> +                              "fullscreen 3D" : "compute");
>>> +             adev->gfx.workload_profile_active = true;
>>>       }
>>> +     mutex_unlock(&adev->gfx.workload_profile_mutex);
>>>  }
>>>
>>>  void amdgpu_gfx_profile_ring_end_use(struct amdgpu_ring *ring)
>>