Re: [PATCH 2/2] drm/amd/powerplay: simplify the code of [get|set]_activity_monitor_coeff

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



@Quan, Evan

I remake a new patch v2 to fix it, please help me review it.

Thanks.


Best Regards,
Kevin


From: Quan, Evan
Sent: Tuesday, April 9, 2019 9:33:06 AM
To: Wang, Kevin(Yang); amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Huang, Ray; Feng, Kenneth; Wang, Kevin(Yang)
Subject: RE: [PATCH 2/2] drm/amd/powerplay: simplify the code of [get|set]_activity_monitor_coeff
 
Comment inline

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Wang, Kevin(Yang)
> Sent: Monday, April 08, 2019 4:43 PM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Feng, Kenneth
> <Kenneth.Feng@xxxxxxx>; Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>
> Subject: [PATCH 2/2] drm/amd/powerplay: simplify the code of
> [get|set]_activity_monitor_coeff
>
> use smu_update_table_with_arg to replace old code logic
>
> Signed-off-by: Kevin Wang <kevin1.wang@xxxxxxx>
> ---
>  .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h    |  6 --
>  drivers/gpu/drm/amd/powerplay/smu_v11_0.c     | 75 ++-----------------
>  2 files changed, 7 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index c146b5e884f8..26a7d2c7f4fa 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -524,12 +524,6 @@ struct smu_funcs
>                                               struct
> dm_pp_wm_sets_with_clock_ranges_soc15 *clock_ranges);
>        int (*set_od8_default_settings)(struct smu_context *smu,
>                                        bool initialize);
> -     int (*get_activity_monitor_coeff)(struct smu_context *smu,
> -                                   uint8_t *table,
> -                                   uint16_t workload_type);
> -     int (*set_activity_monitor_coeff)(struct smu_context *smu,
> -                                   uint8_t *table,
> -                                   uint16_t workload_type);
>        int (*conv_power_profile_to_pplib_workload)(int power_profile);
>        int (*get_power_profile_mode)(struct smu_context *smu, char
> *buf);
>        int (*set_power_profile_mode)(struct smu_context *smu, long
> *input, uint32_t size); diff --git
> a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> index 0e4b4b88af24..435cb606d7eb 100644
> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> @@ -1460,62 +1460,6 @@ static int
> smu_v11_0_set_od8_default_settings(struct smu_context *smu,
>        return 0;
>  }
>
> -static int smu_v11_0_set_activity_monitor_coeff(struct smu_context *smu,
> -                                   uint8_t *table, uint16_t workload_type)
> -{
> -     int ret = 0;
> -     memcpy(smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].cpu_addr,
> -            table, smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].size);
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_SetDriverDramAddrHigh,
> -                                       upper_32_bits(smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].mc_address));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Set Dram Addr High Failed!",
> __func__);
> -             return ret;
> -     }
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_SetDriverDramAddrLow,
> -                                       lower_32_bits(smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].mc_address));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Set Dram Addr Low Failed!",
> __func__);
> -             return ret;
> -     }
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_TransferTableSmu2Dram,
> -                                       TABLE_ACTIVITY_MONITOR_COEFF
> | (workload_type << 16));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Transfer Table From SMU Failed!",
> __func__);
> -             return ret;
> -     }
> -
> -     return ret;
> -}
> -
> -static int smu_v11_0_get_activity_monitor_coeff(struct smu_context *smu,
> -                                   uint8_t *table, uint16_t workload_type)
> -{
> -     int ret = 0;
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_SetDriverDramAddrHigh,
> -                                       upper_32_bits(smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].mc_address));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Set Dram Addr High Failed!",
> __func__);
> -             return ret;
> -     }
> -
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_SetDriverDramAddrLow,
> -                                       lower_32_bits(smu-
> >smu_table.tables[TABLE_ACTIVITY_MONITOR_COEFF].mc_address));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Set Dram Addr Low Failed!",
> __func__);
> -             return ret;
> -     }
> -
> -     ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_TransferTableSmu2Dram,
> -                                       TABLE_ACTIVITY_MONITOR_COEFF
> | (workload_type << 16));
> -     if (ret) {
> -             pr_err("[%s] Attempt to Transfer Table From SMU Failed!",
> __func__);
> -             return ret;
> -     }
> -
> -     return ret;
> -}
> -
>  static int smu_v11_0_conv_power_profile_to_pplib_workload(int
> power_profile)  {
>        int pplib_workload = 0;
> @@ -1584,9 +1528,8 @@ static int
> smu_v11_0_get_power_profile_mode(struct smu_context *smu, char *buf)
>        for (i = 0; i <= PP_SMC_POWER_PROFILE_CUSTOM; i++) {
>                /* conv PP_SMC_POWER_PROFILE* to
> WORKLOAD_PPLIB_*_BIT */
>                workload_type =
> smu_v11_0_conv_power_profile_to_pplib_workload(i);
> -             result = smu_v11_0_get_activity_monitor_coeff(smu,
> -                                                           (uint8_t
> *)(&activity_monitor),
> -                                                           workload_type);
> +             result = smu_update_table_with_arg(smu,
> TABLE_ACTIVITY_MONITOR_COEFF,
> +                                                workload_type,
> &activity_monitor, false);
>                if (result) {
>                        pr_err("[%s] Failed to get activity monitor!",
> __func__);
>                        return result;
> @@ -1658,7 +1601,7 @@ static int
> smu_v11_0_get_power_profile_mode(struct smu_context *smu, char *buf)
> static int smu_v11_0_set_power_profile_mode(struct smu_context *smu,
> long *input, uint32_t size)  {
>        DpmActivityMonitorCoeffInt_t activity_monitor;
> -     int workload_type, ret = 0;
> +     int workload_type = 0, ret = 0;
>
>        smu->power_profile_mode = input[size];
>
> @@ -1668,9 +1611,8 @@ static int
> smu_v11_0_set_power_profile_mode(struct smu_context *smu, long
> *input
>        }
>
>        if (smu->power_profile_mode ==
> PP_SMC_POWER_PROFILE_CUSTOM) {
> -             ret = smu_v11_0_get_activity_monitor_coeff(smu,
> -                                                        (uint8_t
> *)(&activity_monitor),
> -
> WORKLOAD_PPLIB_CUSTOM_BIT);
> +             ret = smu_update_table_with_arg(smu,
> TABLE_ACTIVITY_MONITOR_COEFF,
> +                                             workload_type,
[Quan, Evan] workload_type seems not initialzed as WORKLOAD_PPLIB_CUSTOM_BIT before use. Can you confirm that?
> &activity_monitor, false);
>                if (ret) {
>                        pr_err("[%s] Failed to get activity monitor!",
> __func__);
>                        return ret;
> @@ -1723,9 +1665,8 @@ static int
> smu_v11_0_set_power_profile_mode(struct smu_context *smu, long
> *input
>                        break;
>                }
>
> -             ret = smu_v11_0_set_activity_monitor_coeff(smu,
> -                                                        (uint8_t
> *)(&activity_monitor),
> -
> WORKLOAD_PPLIB_CUSTOM_BIT);
> +             ret = smu_update_table_with_arg(smu,
> TABLE_ACTIVITY_MONITOR_COEFF,
> +
>        WORKLOAD_PPLIB_COMPUTE_BIT, &activity_monitor, true);
>                if (ret) {
>                        pr_err("[%s] Failed to set activity monitor!",
> __func__);
>                        return ret;
> @@ -1994,8 +1935,6 @@ static const struct smu_funcs smu_v11_0_funcs = {
>        .get_sclk = smu_v11_0_dpm_get_sclk,
>        .get_mclk = smu_v11_0_dpm_get_mclk,
>        .set_od8_default_settings = smu_v11_0_set_od8_default_settings,
> -     .get_activity_monitor_coeff =
> smu_v11_0_get_activity_monitor_coeff,
> -     .set_activity_monitor_coeff =
> smu_v11_0_set_activity_monitor_coeff,
>        .conv_power_profile_to_pplib_workload =
> smu_v11_0_conv_power_profile_to_pplib_workload,
>        .get_power_profile_mode = smu_v11_0_get_power_profile_mode,
>        .set_power_profile_mode = smu_v11_0_set_power_profile_mode,
> --
> 2.21.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux