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

[Alex Deucher]

On Mon, Aug 21, 2023 at 1:54 PM Yadav, Arvind <arvyadav@xxxxxxx> wrote:
>
>
> On 8/21/2023 9:52 PM, Alex Deucher wrote:
> > On Mon, Aug 21, 2023 at 2:55 AM Arvind Yadav <Arvind.Yadav@xxxxxxx> 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@xxxxxxx>
> >> Cc: Christian Koenig <christian.koenig@xxxxxxx>
> >> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> >> Signed-off-by: Arvind Yadav <Arvind.Yadav@xxxxxxx>
> >> ---
> >>   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.

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