On Mon, Oct 28, 2024 at 11:48 PM Lazar, Lijo <lijo.lazar@xxxxxxx> wrote:
>
>
>
> On 10/28/2024 11:42 PM, Alex Deucher wrote:
> > On Thu, Oct 24, 2024 at 5:18 AM Kenneth Feng <kenneth.feng@xxxxxxx> wrote:
> >>
> >> Correct the workload setting in order not to mix the setting
> >> with the end user. Update the workload mask accordingly.
> >>
> >> v2: changes as below:
> >> 1. the end user can not erase the workload from driver except default workload.
> >> 2. always shows the real highest priority workoad to the end user.
> >> 3. the real workload mask is combined with driver workload mask and end user workload mask.
> >
> > I think this can be simplified. We just need to store the user
> > workload profile and the mask of all of the currently active workload
> > profiles (the user selected profile and the any transient ones like
> > COMPUTE for KFD, VIDEO for VCN, and POWERSAVE for SMU13, etc.). At
> > init time, the driver sets the user workload profile to FS3D or
> > DEFAULT per the current logic. Add a new parameter to
> > ppt_funcs->set_power_profile_mode(), bool
> > update_user_workload_profile, which we set to true in
> > smu_set_power_profile_mode() which is used by the sysfs code to set
> > the user workload profile, and set to false in
> > smu_switch_power_profile() which is used internally for KFD and VCN.
> > Then the user workload profile would only get changed when the user
> > changes it via sysfs. Meanwhile KFD and VCN can add their workload
> > types dynamically at runtime.
> >
>
> I think this approach doesn't work if we want to restore the user
> settings after a suspend/resume case (unless we expect it to be user
> driven).
We can set update_user_workload_profile = true in when init = true in
smu_adjust_power_state_dynamic(). That would cover init and resume,
but I guess we'd still need to handle the cases where a user sets
COMPUTE via sysfs and then a ROCm app runs and exits and we need to
keep compute selected. I guess, the solution goes back to my original
idea from years ago which was to ref count each of the profiles so
that they would stay set until each user dropped its reference.
Anyway for now, I guess Kenneth's patch is fine.
Alex
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>
> >> Signed-off-by: Kenneth Feng <kenneth.feng@xxxxxxx>
> >> ---
> >> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 44 +++++++++++++------
> >> drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 5 ++-
> >> .../drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 31 +++++++++++--
> >> .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 25 +++++++++--
> >> .../drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c | 28 +++++++++---
> >> 5 files changed, 106 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >> index 8d4aee4e2287..1de576461a70 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >> @@ -1261,25 +1261,31 @@ static int smu_sw_init(struct amdgpu_ip_block *ip_block)
> >> smu->watermarks_bitmap = 0;
> >> smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> >> smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> >> + smu->user_dpm_profile.user_workload_mask = 0;
> >> + smu->user_dpm_profile.prev_user_workload_mask = 0;
> >>
> >> atomic_set(&smu->smu_power.power_gate.vcn_gated, 1);
> >> atomic_set(&smu->smu_power.power_gate.jpeg_gated, 1);
> >> atomic_set(&smu->smu_power.power_gate.vpe_gated, 1);
> >> atomic_set(&smu->smu_power.power_gate.umsch_mm_gated, 1);
> >>
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT] = 0;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_FULLSCREEN3D] = 1;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_POWERSAVING] = 2;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_VIDEO] = 3;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_VR] = 4;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_COMPUTE] = 5;
> >> - smu->workload_prority[PP_SMC_POWER_PROFILE_CUSTOM] = 6;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT] = 0;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_FULLSCREEN3D] = 1;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_POWERSAVING] = 2;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_VIDEO] = 3;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_VR] = 4;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_COMPUTE] = 5;
> >> + smu->workload_priority[PP_SMC_POWER_PROFILE_CUSTOM] = 6;
> >>
> >> if (smu->is_apu ||
> >> - !smu_is_workload_profile_available(smu, PP_SMC_POWER_PROFILE_FULLSCREEN3D))
> >> - smu->workload_mask = 1 << smu->workload_prority[PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT];
> >> - else
> >> - smu->workload_mask = 1 << smu->workload_prority[PP_SMC_POWER_PROFILE_FULLSCREEN3D];
> >> + !smu_is_workload_profile_available(smu, PP_SMC_POWER_PROFILE_FULLSCREEN3D)) {
> >> + smu->workload_mask = 1 << smu->workload_priority[PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT];
> >> + } else {
> >> + smu->workload_mask = 1 << smu->workload_priority[PP_SMC_POWER_PROFILE_FULLSCREEN3D];
> >> + smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_FULLSCREEN3D;
> >> + }
> >> +
> >> + smu->driver_workload_mask = smu->workload_mask;
> >>
> >> smu->workload_setting[0] = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> >> smu->workload_setting[1] = PP_SMC_POWER_PROFILE_FULLSCREEN3D;
> >> @@ -2354,12 +2360,14 @@ static int smu_switch_power_profile(void *handle,
> >> return -EINVAL;
> >>
> >> if (!en) {
> >> - smu->workload_mask &= ~(1 << smu->workload_prority[type]);
> >> + smu->workload_mask &= ~(1 << smu->workload_priority[type]);
> >> + smu->driver_workload_mask &= ~(1 << smu->workload_priority[type]);
> >> index = fls(smu->workload_mask);
> >> index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> >> workload[0] = smu->workload_setting[index];
> >> } else {
> >> - smu->workload_mask |= (1 << smu->workload_prority[type]);
> >> + smu->workload_mask |= (1 << smu->workload_priority[type]);
> >> + smu->driver_workload_mask |= (1 << smu->workload_priority[type]);
> >> index = fls(smu->workload_mask);
> >> index = index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> >> workload[0] = smu->workload_setting[index];
> >> @@ -3054,12 +3062,20 @@ static int smu_set_power_profile_mode(void *handle,
> >> uint32_t param_size)
> >> {
> >> struct smu_context *smu = handle;
> >> + int ret;
> >>
> >> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled ||
> >> !smu->ppt_funcs->set_power_profile_mode)
> >> return -EOPNOTSUPP;
> >>
> >> - return smu_bump_power_profile_mode(smu, param, param_size);
> >> + smu->user_dpm_profile.prev_user_workload_mask =
> >> + smu->user_dpm_profile.user_workload_mask;
> >> + smu->user_dpm_profile.user_workload_mask = (1 << smu->workload_priority[param[param_size]]);
> >> + ret = smu_bump_power_profile_mode(smu, param, param_size);
> >> + smu->user_dpm_profile.prev_user_workload_mask =
> >> + smu->user_dpm_profile.user_workload_mask;
> >> +
> >> + return ret;
> >> }
> >>
> >> static int smu_get_fan_control_mode(void *handle, u32 *fan_mode)
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >> index 8bb32b3f0d9c..88294d986b36 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> >> @@ -240,6 +240,8 @@ struct smu_user_dpm_profile {
> >> /* user clock state information */
> >> uint32_t clk_mask[SMU_CLK_COUNT];
> >> uint32_t clk_dependency;
> >> + uint32_t user_workload_mask;
> >> + uint32_t prev_user_workload_mask;
> >> };
> >>
> >> #define SMU_TABLE_INIT(tables, table_id, s, a, d) \
> >> @@ -557,7 +559,8 @@ struct smu_context {
> >> bool disable_uclk_switch;
> >>
> >> uint32_t workload_mask;
> >> - uint32_t workload_prority[WORKLOAD_POLICY_MAX];
> >> + uint32_t driver_workload_mask;
> >> + uint32_t workload_priority[WORKLOAD_POLICY_MAX];
> >> uint32_t workload_setting[WORKLOAD_POLICY_MAX];
> >> uint32_t power_profile_mode;
> >> uint32_t default_power_profile_mode;
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> index 3e2277abc754..0733fd3efd8b 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
> >> @@ -2474,9 +2474,22 @@ static int smu_v13_0_0_set_power_profile_mode(struct smu_context *smu,
> >> &(activity_monitor_external.DpmActivityMonitorCoeffInt);
> >> int workload_type, ret = 0;
> >> u32 workload_mask;
> >> + uint32_t index;
> >>
> >> smu->power_profile_mode = input[size];
> >>
> >> + if (smu->user_dpm_profile.prev_user_workload_mask !=
> >> + smu->user_dpm_profile.user_workload_mask) {
> >> + if (smu->workload_mask & smu->user_dpm_profile.prev_user_workload_mask &&
> >> + !(smu->driver_workload_mask & smu->user_dpm_profile.prev_user_workload_mask))
> >> + smu->workload_mask &= ~smu->user_dpm_profile.prev_user_workload_mask;
> >> +
> >> + if (input[size] != smu->default_power_profile_mode) {
> >> + smu->workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + smu->driver_workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + }
> >> + }
> >
> > This is repeated in several places and could be split out into a
> > helper function.
> >
> >> +
> >> if (smu->power_profile_mode >= PP_SMC_POWER_PROFILE_COUNT) {
> >> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", smu->power_profile_mode);
> >> return -EINVAL;
> >> @@ -2555,12 +2568,24 @@ static int smu_v13_0_0_set_power_profile_mode(struct smu_context *smu,
> >> workload_mask |= 1 << workload_type;
> >> }
> >>
> >> + smu->workload_mask |= workload_mask;
> >> ret = smu_cmn_send_smc_msg_with_param(smu,
> >> SMU_MSG_SetWorkloadMask,
> >> - workload_mask,
> >> + smu->workload_mask,
> >> NULL);
> >> - if (!ret)
> >> - smu->workload_mask = workload_mask;
> >> + if (!ret) {
> >> + index = fls(smu->workload_mask);
> >> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> >> + smu->power_profile_mode = smu->workload_setting[index];
> >> + if (smu->power_profile_mode == PP_SMC_POWER_PROFILE_POWERSAVING) {
> >> + workload_type = smu_cmn_to_asic_specific_index(smu,
> >> + CMN2ASIC_MAPPING_WORKLOAD,
> >> + PP_SMC_POWER_PROFILE_FULLSCREEN3D);
> >> + smu->power_profile_mode = smu->workload_mask & (1 << workload_type)
> >> + ? PP_SMC_POWER_PROFILE_FULLSCREEN3D
> >> + : PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> >> + }
> >> + }
> >>
> >> return ret;
> >> }
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
> >> index 23f13388455f..2323c74ee50b 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
> >> @@ -2429,9 +2429,22 @@ static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *inp
> >> DpmActivityMonitorCoeffInt_t *activity_monitor =
> >> &(activity_monitor_external.DpmActivityMonitorCoeffInt);
> >> int workload_type, ret = 0;
> >> + uint32_t index;
> >>
> >> smu->power_profile_mode = input[size];
> >>
> >> + if (smu->user_dpm_profile.prev_user_workload_mask !=
> >> + smu->user_dpm_profile.user_workload_mask) {
> >> + if (smu->workload_mask & smu->user_dpm_profile.prev_user_workload_mask &&
> >> + !(smu->driver_workload_mask & smu->user_dpm_profile.prev_user_workload_mask))
> >> + smu->workload_mask &= ~smu->user_dpm_profile.prev_user_workload_mask;
> >> +
> >> + if (input[size] != smu->default_power_profile_mode) {
> >> + smu->workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + smu->driver_workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + }
> >> + }
> >> +
> >> if (smu->power_profile_mode > PP_SMC_POWER_PROFILE_WINDOW3D) {
> >> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", smu->power_profile_mode);
> >> return -EINVAL;
> >> @@ -2487,13 +2500,19 @@ static int smu_v13_0_7_set_power_profile_mode(struct smu_context *smu, long *inp
> >> smu->power_profile_mode);
> >> if (workload_type < 0)
> >> return -EINVAL;
> >> +
> >> + smu->workload_mask |= (1 << workload_type);
> >> ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetWorkloadMask,
> >> - 1 << workload_type, NULL);
> >> + smu->workload_mask, NULL);
> >>
> >> if (ret)
> >> dev_err(smu->adev->dev, "[%s] Failed to set work load mask!", __func__);
> >> - else
> >> - smu->workload_mask = (1 << workload_type);
> >> +
> >> + if (!ret) {
> >> + index = fls(smu->workload_mask);
> >> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> >> + smu->power_profile_mode = smu->workload_setting[index];
> >> + }
> >>
> >> return ret;
> >> }
> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c
> >> index cefe10b95d8e..c2fd47f04e2d 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c
> >> @@ -1787,9 +1787,22 @@ static int smu_v14_0_2_set_power_profile_mode(struct smu_context *smu,
> >> DpmActivityMonitorCoeffInt_t *activity_monitor =
> >> &(activity_monitor_external.DpmActivityMonitorCoeffInt);
> >> int workload_type, ret = 0;
> >> + uint32_t index;
> >> uint32_t current_profile_mode = smu->power_profile_mode;
> >> smu->power_profile_mode = input[size];
> >>
> >> + if (smu->user_dpm_profile.prev_user_workload_mask !=
> >> + smu->user_dpm_profile.user_workload_mask) {
> >> + if (smu->workload_mask & smu->user_dpm_profile.prev_user_workload_mask &&
> >> + !(smu->driver_workload_mask & smu->user_dpm_profile.prev_user_workload_mask))
> >> + smu->workload_mask &= ~smu->user_dpm_profile.prev_user_workload_mask;
> >> +
> >> + if (input[size] != smu->default_power_profile_mode) {
> >> + smu->workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + smu->driver_workload_mask &= ~(1 << smu->workload_priority[smu->default_power_profile_mode]);
> >> + }
> >> + }
> >> +
> >> if (smu->power_profile_mode >= PP_SMC_POWER_PROFILE_COUNT) {
> >> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", smu->power_profile_mode);
> >> return -EINVAL;
> >> @@ -1857,12 +1870,15 @@ static int smu_v14_0_2_set_power_profile_mode(struct smu_context *smu,
> >> if (workload_type < 0)
> >> return -EINVAL;
> >>
> >> - ret = smu_cmn_send_smc_msg_with_param(smu,
> >> - SMU_MSG_SetWorkloadMask,
> >> - 1 << workload_type,
> >> - NULL);
> >> - if (!ret)
> >> - smu->workload_mask = 1 << workload_type;
> >> + smu->workload_mask |= (1 << workload_type);
> >> + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetWorkloadMask,
> >> + smu->workload_mask, NULL);
> >> +
> >> + if (!ret) {
> >> + index = fls(smu->workload_mask);
> >> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> >> + smu->power_profile_mode = smu->workload_setting[index];
> >> + }
> >>
> >> return ret;
> >> }
> >> --
> >> 2.34.1
> >>
