Re: [PATCH] drm/amd/pm: update deep sleep status on smu v14.0.2/3

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

On 10/17/2024 5:16 AM, Feng, Kenneth wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> The logic is that when entering the compute workload, enable deep sleep.
> When exiting the compute workload, disable deep sleep.
> It's not required to do the deep sleep setting in other workload type.
> Thanks.
> 

This patch looks good. Please name temp_profile_mode as current_profile
for clarity and may be comment about smu->power_profile_mode as the
new/target mode.

With those -
	Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo


> 
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Wednesday, October 16, 2024 8:38 PM
> To: Feng, Kenneth <Kenneth.Feng@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Feng, Kenneth <Kenneth.Feng@amd.com>
> Subject: RE: [PATCH] drm/amd/pm: update deep sleep status on smu v14.0.2/3
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> -----Original Message-----
> From: Kenneth Feng <kenneth.feng@amd.com>
> Sent: Wednesday, October 16, 2024 3:19 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com>
> Subject: [PATCH] drm/amd/pm: update deep sleep status on smu v14.0.2/3
> 
> disable deep sleep during the compute workload for the potential performance loss on smu v14.0.2/3
> 
> Signed-off-by: Kenneth Feng <kenneth.feng@amd.com>
> ---
>  drivers/gpu/drm/amd/pm/swsmu/smu14/smu_v14_0_2_ppt.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 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 27f4e0ce447d..ecdd11a87311 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
> @@ -1784,7 +1784,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 temp_profile_mode = smu->power_profile_mode;
>         smu->power_profile_mode = input[size];
> 
>         if (smu->power_profile_mode >= PP_SMC_POWER_PROFILE_COUNT) { @@ -1842,6 +1842,11 @@ static int smu_v14_0_2_set_power_profile_mode(struct smu_context *smu,
>                 }
>         }
> 
> +       if (smu->power_profile_mode == PP_SMC_POWER_PROFILE_COMPUTE)
> +               smu_v14_0_deep_sleep_control(smu, false);
> +       else if (temp_profile_mode == PP_SMC_POWER_PROFILE_COMPUTE)
> +               smu_v14_0_deep_sleep_control(smu, true);
> [Kevin]:
> 
> This code logic doesn't seem easy to understand, If I understand correctly, can I modify it to the following writing style:
> 
> If (new profile != current profile) {
>             smu_v14_0_deep_sleep_control(smu, new profile == compute profile ? false : true);
>             smu->power_profile_mode = new profile.
> }
> 
> Best Regards,
> Kevin
>         /* conv PP_SMC_POWER_PROFILE* to WORKLOAD_PPLIB_*_BIT */
>         workload_type = smu_cmn_to_asic_specific_index(smu,
>                                                        CMN2ASIC_MAPPING_WORKLOAD,
> --
> 2.34.1
> 
>