Re: [PATCH v2 2/7] drm/amdgpu: Add new function to set GPU power profile

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

On 8/21/2023 11:40 PM, Alex Deucher wrote:
> On Mon, Aug 21, 2023 at 1:54 PM Yadav, Arvind <arvyadav@amd.com> wrote:
>>
>> On 8/21/2023 9:52 PM, Alex Deucher wrote:
>>> On Mon, Aug 21, 2023 at 2:55 AM Arvind Yadav <Arvind.Yadav@amd.com> wrote:
>>>> This patch adds a function which will change the GPU
>>>> power profile based on a submitted job. This can optimize
>>>> the power performance when the workload is on.
>>>>
>>>> v2:
>>>> - Splitting workload_profile_set and workload_profile_put
>>>>     into two separate patches.
>>>> - Addressed 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_workload.c  | 56 +++++++++++++++++++
>>>>    drivers/gpu/drm/amd/include/amdgpu_workload.h |  3 +
>>>>    2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
>>>> index 32166f482f77..e661cc5b3d92 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c
>>>> @@ -24,6 +24,62 @@
>>>>
>>>>    #include "amdgpu.h"
>>>>
>>>> +static enum PP_SMC_POWER_PROFILE
>>>> +ring_to_power_profile(uint32_t ring_type)
>>>> +{
>>>> +       switch (ring_type) {
>>>> +       case AMDGPU_RING_TYPE_GFX:
>>>> +               return PP_SMC_POWER_PROFILE_FULLSCREEN3D;
>>>> +       case AMDGPU_RING_TYPE_COMPUTE:
>>>> +               return PP_SMC_POWER_PROFILE_COMPUTE;
>>>> +       case AMDGPU_RING_TYPE_UVD:
>>>> +       case AMDGPU_RING_TYPE_VCE:
>>>> +       case AMDGPU_RING_TYPE_UVD_ENC:
>>>> +       case AMDGPU_RING_TYPE_VCN_DEC:
>>>> +       case AMDGPU_RING_TYPE_VCN_ENC:
>>>> +       case AMDGPU_RING_TYPE_VCN_JPEG:
>>>> +               return PP_SMC_POWER_PROFILE_VIDEO;
>>>> +       default:
>>>> +               return PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
>>>> +       }
>>>> +}
>>>> +
>>>> +static int
>>>> +amdgpu_power_profile_set(struct amdgpu_device *adev,
>>>> +                        enum PP_SMC_POWER_PROFILE profile)
>>>> +{
>>>> +       int ret = amdgpu_dpm_switch_power_profile(adev, profile, true);
>>>> +
>>>> +       if (!ret) {
>>>> +               /* Set the bit for the submitted workload profile */
>>>> +               adev->smu_workload.submit_workload_status |= (1 << profile);
>>>> +               atomic_inc(&adev->smu_workload.power_profile_ref[profile]);
>>>> +       }
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +void amdgpu_workload_profile_set(struct amdgpu_device *adev,
>>>> +                                uint32_t ring_type)
>>>> +{
>>>> +       struct amdgpu_smu_workload *workload = &adev->smu_workload;
>>>> +       enum PP_SMC_POWER_PROFILE profile = ring_to_power_profile(ring_type);
>>>> +       int ret;
>>>> +
>>>> +       if (profile == PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT)
>>>> +               return;
>>> Why is this one skipped?  How do we get back to the boot up profile?
>> Hi Alex,
>>
>> enum PP_SMC_POWER_PROFILE {
>>       PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT = 0x0,
>>       PP_SMC_POWER_PROFILE_FULLSCREEN3D = 0x1,
>>       PP_SMC_POWER_PROFILE_POWERSAVING  = 0x2,
>>       PP_SMC_POWER_PROFILE_VIDEO        = 0x3,
>>       PP_SMC_POWER_PROFILE_VR           = 0x4,
>>       PP_SMC_POWER_PROFILE_COMPUTE      = 0x5,
>>       PP_SMC_POWER_PROFILE_CUSTOM       = 0x6,
>>       PP_SMC_POWER_PROFILE_WINDOW3D     = 0x7,
>>       PP_SMC_POWER_PROFILE_COUNT,
>> };
>>
>> These are all the profiles. We are using which is >
>> PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT.
>> Now suppose the profile was DEFAULT and we set it to VIDEO, SMU will
>> move the profile to a higher level.
>> When we reset the VIDEO profile then SMU will move back to the DEFAULT one.
>>
>> Our job is to set the profile and reset it after the job is done.
>> SMU will take care to move to a higher profile and after reset, it will
>> move back to DEFAULT.
> I guess that is the part I'm missing.  How does the call to the SMU to
> set the profile back to DEFAULT actually happen?  It seems that both
> the put and get functions return early in this case.
SMU is calculating a workload for given the profile and setting it when 
we call the *get and *put function.
When we call *set function for VIDEO then SMU will calculate a workload 
for VIDEO and set it. Now We call
*put function for the same profile then SMU calculates a workload which 
will be lower or DEFAULT (0)
and then it will set it.

Suppose we have called *set function for VIDEO profile then SMU will 
calculate the workload = 4 and set it.
Now we have called *put function for the same profile then SMU will 
calculate the workload = 0 and set it.

please see the below smu code where index will be DEFAULT (0) or lower 
for *put function.

if (!en) { // put function
         smu->workload_mask &= ~(1 << smu->workload_prority[type]);
         index = fls(smu->workload_mask);
         index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
         workload = smu->workload_setting[index];
} else { // set function.
         smu->workload_mask |= (1 << smu->workload_prority[type]);
         index = fls(smu->workload_mask);
         index = index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
         workload = smu->workload_setting[index];
}

In our case the *set function will set the GPU  power profile and the 
*put function will schedule
the smu_delayed_work task after 100ms delay. This smu_delayed_work task 
will clear a GPU power profile if any
new jobs are not scheduled within 100 ms. But if any new job comes 
within 100ms then the *set function
will cancel this work and set the GPU power profile based on preferences.

Thank You
~Arvind

>
> Alex
>
>
> Alex
>
>
>> ThankYou,
>> ~Arvind
>>
>>> Alex
>>>
>>>> +
>>>> +       mutex_lock(&workload->workload_lock);
>>>> +
>>>> +       ret = amdgpu_power_profile_set(adev, profile);
>>>> +       if (ret) {
>>>> +               DRM_WARN("Failed to set workload profile to %s, error = %d\n",
>>>> +                        amdgpu_workload_mode_name[profile], ret);
>>>> +       }
>>>> +
>>>> +       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 5d0f068422d4..5022f28fc2f9 100644
>>>> --- a/drivers/gpu/drm/amd/include/amdgpu_workload.h
>>>> +++ b/drivers/gpu/drm/amd/include/amdgpu_workload.h
>>>> @@ -46,6 +46,9 @@ static const char * const amdgpu_workload_mode_name[] = {
>>>>           "Window3D"
>>>>    };
>>>>
>>>> +void amdgpu_workload_profile_set(struct amdgpu_device *adev,
>>>> +                                uint32_t ring_type);
>>>> +
>>>>    void amdgpu_workload_profile_init(struct amdgpu_device *adev);
>>>>
>>>>    void amdgpu_workload_profile_fini(struct amdgpu_device *adev);
>>>> --
>>>> 2.34.1
>>>>