On 10/30/2024 3:13 PM, Kenneth Feng 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.
>
> v3: apply this to the other ASICs as well.
> v4: simplify the code
>
> Signed-off-by: Kenneth Feng <kenneth.feng@xxxxxxx>
> ---
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 39 ++++++++++++-------
> drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 4 +-
> .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 8 ++--
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 10 ++++-
> .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 10 ++++-
> .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 7 +++-
> .../gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 7 +++-
> .../drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 23 ++++++++---
> .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 12 ++++--
> .../drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c | 15 ++++---
> 10 files changed, 97 insertions(+), 38 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..57cbc41f8457 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1261,26 +1261,30 @@ 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;
>
> 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];
For adding clarity to code, the operation needs to be reversed. Instead
of smu->workload_mask, assign smu->driver_workload_mask here.
> + } else {
> + smu->workload_mask = 1 << smu->workload_priority[PP_SMC_POWER_PROFILE_FULLSCREEN3D];
Same as above, assign smu->driver_workload_mask here.
> + smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_FULLSCREEN3D;
> + }
>
> + smu->driver_workload_mask = smu->workload_mask;
Expecting the assignment here like
smu->workload_mask = smu->driver_workload_mask |
smu->user_dpm_profile.user_workload_mask // at this point user workload
mask will be 0.
If this varialbe needs to be kept, preferrably, this is done just before
sending final message to FW.
> smu->workload_setting[0] = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> smu->workload_setting[1] = PP_SMC_POWER_PROFILE_FULLSCREEN3D;
> smu->workload_setting[2] = PP_SMC_POWER_PROFILE_POWERSAVING;
> @@ -2354,12 +2358,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]);
Same comment as above. Assign only driver mask here and in else part.
Outside if..else, assign smu->driver_workload_mask.
smu->workload_mask = smu->driver_workload_mask |
smu->user_dpm_profile.user_workload_mask.
Basically, whenever workload mask is applied, it takes driver and user
mask as inputs. This makes sure that any user specified values are not
lost when driver prefers a different mode. We could let the FW decide
the final workload priority.
> 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 +3060,17 @@ 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.user_workload_mask = (1 << smu->workload_priority[param[param_size]]);
Not sure if a check is required here about the current
user_workload_mask and newly passed parameter. If required, please add
that here.
> + smu->workload_mask = smu->user_dpm_profile.user_workload_mask | smu->driver_workload_mask;
This is the expected code for every profile mode change which represents
the final mask. The bitwise mask operations are done only on the
respective masks.
> + ret = smu_bump_power_profile_mode(smu, param, param_size);
> +
> + 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..4ffed49ebb4b 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,7 @@ 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;
> };
>
> #define SMU_TABLE_INIT(tables, table_id, s, a, d) \
> @@ -557,7 +558,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/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> index 5ad09323a29d..f1e271e4f470 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -1449,13 +1449,13 @@ static int arcturus_set_power_profile_mode(struct smu_context *smu,
> int workload_type = 0;
> uint32_t profile_mode = input[size];
> int ret = 0;
> + uint32_t index;
>
> if (profile_mode > PP_SMC_POWER_PROFILE_CUSTOM) {
> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", profile_mode);
> return -EINVAL;
> }
>
> -
> if ((profile_mode == PP_SMC_POWER_PROFILE_CUSTOM) &&
> (smu->smc_fw_version >= 0x360d00)) {
> if (size != 10)
> @@ -1523,14 +1523,16 @@ static int arcturus_set_power_profile_mode(struct smu_context *smu,
>
> ret = smu_cmn_send_smc_msg_with_param(smu,
> SMU_MSG_SetWorkloadMask,
> - 1 << workload_type,
> + smu->workload_mask,
> NULL);
> if (ret) {
> dev_err(smu->adev->dev, "Fail to set workload type %d\n", workload_type);
> return ret;
> }
>
> - smu->power_profile_mode = profile_mode;
> + index = fls(smu->workload_mask);
> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> + smu->power_profile_mode = smu->workload_setting[index];
The above piece of code can be moved to something like
smu_cmn_assign/calculate_power_profile() and called from all IP funcs.
Thanks,
Lijo
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> index 9fa305ba6422..df158d789ced 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -2008,6 +2008,7 @@ static int navi10_set_power_profile_mode(struct smu_context *smu, long *input, u
> {
> DpmActivityMonitorCoeffInt_t activity_monitor;
> int workload_type, ret = 0;
> + uint32_t index;
>
> smu->power_profile_mode = input[size];
>
> @@ -2081,11 +2082,18 @@ static int navi10_set_power_profile_mode(struct smu_context *smu, long *input, u
> smu->power_profile_mode);
> if (workload_type < 0)
> return -EINVAL;
> +
> 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__);
>
> + 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/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> index 77e58eb46328..7263e53eafe1 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> @@ -1713,6 +1713,7 @@ static int sienna_cichlid_set_power_profile_mode(struct smu_context *smu, long *
> DpmActivityMonitorCoeffInt_t *activity_monitor =
> &(activity_monitor_external.DpmActivityMonitorCoeffInt);
> int workload_type, ret = 0;
> + uint32_t index;
>
> smu->power_profile_mode = input[size];
>
> @@ -1786,11 +1787,18 @@ static int sienna_cichlid_set_power_profile_mode(struct smu_context *smu, long *
> smu->power_profile_mode);
> if (workload_type < 0)
> return -EINVAL;
> +
> 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__);
>
> + 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/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index 6c43724c01dd..fa7b45208777 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -1058,6 +1058,7 @@ static int vangogh_set_power_profile_mode(struct smu_context *smu, long *input,
> {
> int workload_type, ret;
> uint32_t profile_mode = input[size];
> + uint32_t index;
>
> if (profile_mode >= PP_SMC_POWER_PROFILE_COUNT) {
> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", profile_mode);
> @@ -1079,7 +1080,7 @@ static int vangogh_set_power_profile_mode(struct smu_context *smu, long *input,
> }
>
> ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_ActiveProcessNotify,
> - 1 << workload_type,
> + smu->workload_mask,
> NULL);
> if (ret) {
> dev_err_once(smu->adev->dev, "Fail to set workload type %d\n",
> @@ -1087,7 +1088,9 @@ static int vangogh_set_power_profile_mode(struct smu_context *smu, long *input,
> return ret;
> }
>
> - smu->power_profile_mode = profile_mode;
> + index = fls(smu->workload_mask);
> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> + smu->power_profile_mode = smu->workload_setting[index];
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> index 0b210b1f2628..4aa3bf005850 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> @@ -866,6 +866,7 @@ static int renoir_set_power_profile_mode(struct smu_context *smu, long *input, u
> {
> int workload_type, ret;
> uint32_t profile_mode = input[size];
> + uint32_t index;
>
> if (profile_mode > PP_SMC_POWER_PROFILE_CUSTOM) {
> dev_err(smu->adev->dev, "Invalid power profile mode %d\n", profile_mode);
> @@ -890,14 +891,16 @@ static int renoir_set_power_profile_mode(struct smu_context *smu, long *input, u
> }
>
> ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_ActiveProcessNotify,
> - 1 << workload_type,
> + smu->workload_mask,
> NULL);
> if (ret) {
> dev_err_once(smu->adev->dev, "Fail to set workload type %d\n", workload_type);
> return ret;
> }
>
> - smu->power_profile_mode = profile_mode;
> + index = fls(smu->workload_mask);
> + index = index > 0 && index <= WORKLOAD_POLICY_MAX ? index - 1 : 0;
> + smu->power_profile_mode = smu->workload_setting[index];
>
> return 0;
> }
> 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 8d25cc1f218f..1023b39ac995 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
> @@ -2473,7 +2473,8 @@ static int smu_v13_0_0_set_power_profile_mode(struct smu_context *smu,
> DpmActivityMonitorCoeffInt_t *activity_monitor =
> &(activity_monitor_external.DpmActivityMonitorCoeffInt);
> int workload_type, ret = 0;
> - u32 workload_mask, selected_workload_mask;
> + u32 workload_mask;
> + uint32_t index;
>
> smu->power_profile_mode = input[size];
>
> @@ -2540,7 +2541,7 @@ static int smu_v13_0_0_set_power_profile_mode(struct smu_context *smu,
> if (workload_type < 0)
> return -EINVAL;
>
> - selected_workload_mask = workload_mask = 1 << workload_type;
> + workload_mask = 1 << workload_type;
>
> /* Add optimizations for SMU13.0.0/10. Reuse the power saving profile */
> if ((amdgpu_ip_version(smu->adev, MP1_HWIP, 0) == IP_VERSION(13, 0, 0) &&
> @@ -2555,12 +2556,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 = selected_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..056c45453a36 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,6 +2429,7 @@ 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];
>
> @@ -2487,13 +2488,18 @@ 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;
> +
> 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..f139c338e822 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,6 +1787,7 @@ 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];
>
> @@ -1857,12 +1858,14 @@ 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;
> + 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;
> }
