Re: [PATCH 4/8] drm/amd/pp: Clean up the code for RV in powerplay

[Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Mar 7, 2018 at 5:46 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
> In send_message_to_smu helper functions,
> Print out the error code for debug if smu failed to response.
>
> and the helper functions always return true, so no need to
> check their return value.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

Is this consistent with our other smu implementations?  If we never
use the errors, I'm ok with removing them, but I don't want to have to
add them again later if we decide we actually need them.

Alex

>
> Change-Id: I27da8bf22d3cea0df968df7b0809dc727461762f
> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c   | 72 +++++------------
>  drivers/gpu/drm/amd/powerplay/smumgr/rv_smumgr.c | 98 ++++++++----------------
>  2 files changed, 53 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> index 8ddfb78..4b5c5fc 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
> @@ -243,8 +243,7 @@ static int rv_disable_gfx_off(struct pp_hwmgr *hwmgr)
>         struct rv_hwmgr *rv_data = (struct rv_hwmgr *)(hwmgr->backend);
>
>         if (rv_data->gfx_off_controled_by_driver)
> -               smum_send_msg_to_smc(hwmgr,
> -                                               PPSMC_MSG_DisableGfxOff);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_DisableGfxOff);
>
>         return 0;
>  }
> @@ -259,8 +258,7 @@ static int rv_enable_gfx_off(struct pp_hwmgr *hwmgr)
>         struct rv_hwmgr *rv_data = (struct rv_hwmgr *)(hwmgr->backend);
>
>         if (rv_data->gfx_off_controled_by_driver)
> -               smum_send_msg_to_smc(hwmgr,
> -                                               PPSMC_MSG_EnableGfxOff);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_EnableGfxOff);
>
>         return 0;
>  }
> @@ -387,24 +385,12 @@ static int rv_populate_clock_table(struct pp_hwmgr *hwmgr)
>         rv_get_clock_voltage_dependency_table(hwmgr, &pinfo->vdd_dep_on_phyclk,
>                                         ARRAY_SIZE(VddPhyClk), &VddPhyClk[0]);
>
> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_GetMinGfxclkFrequency),
> -                       "Attempt to get min GFXCLK Failed!",
> -                       return -1);
> -       PP_ASSERT_WITH_CODE(!rv_read_arg_from_smc(hwmgr,
> -                       &result),
> -                       "Attempt to get min GFXCLK Failed!",
> -                       return -1);
> +       smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetMinGfxclkFrequency);
> +       rv_read_arg_from_smc(hwmgr, &result);
>         rv_data->gfx_min_freq_limit = result * 100;
>
> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_GetMaxGfxclkFrequency),
> -                       "Attempt to get max GFXCLK Failed!",
> -                       return -1);
> -       PP_ASSERT_WITH_CODE(!rv_read_arg_from_smc(hwmgr,
> -                       &result),
> -                       "Attempt to get max GFXCLK Failed!",
> -                       return -1);
> +       smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetMaxGfxclkFrequency);
> +       rv_read_arg_from_smc(hwmgr, &result);
>         rv_data->gfx_max_freq_limit = result * 100;
>
>         return 0;
> @@ -739,14 +725,8 @@ static int rv_print_clock_levels(struct pp_hwmgr *hwmgr,
>
>         switch (type) {
>         case PP_SCLK:
> -               PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                               PPSMC_MSG_GetGfxclkFrequency),
> -                               "Attempt to get current GFXCLK Failed!",
> -                               return -1);
> -               PP_ASSERT_WITH_CODE(!rv_read_arg_from_smc(hwmgr,
> -                               &now),
> -                               "Attempt to get current GFXCLK Failed!",
> -                               return -1);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetGfxclkFrequency);
> +               rv_read_arg_from_smc(hwmgr, &now);
>
>                 size += sprintf(buf + size, "0: %uMhz %s\n",
>                                 data->gfx_min_freq_limit / 100,
> @@ -758,14 +738,8 @@ static int rv_print_clock_levels(struct pp_hwmgr *hwmgr,
>                                  == now) ? "*" : "");
>                 break;
>         case PP_MCLK:
> -               PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                               PPSMC_MSG_GetFclkFrequency),
> -                               "Attempt to get current MEMCLK Failed!",
> -                               return -1);
> -               PP_ASSERT_WITH_CODE(!rv_read_arg_from_smc(hwmgr,
> -                               &now),
> -                               "Attempt to get current MEMCLK Failed!",
> -                               return -1);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetFclkFrequency);
> +               rv_read_arg_from_smc(hwmgr, &now);
>
>                 for (i = 0; i < mclk_table->count; i++)
>                         size += sprintf(buf + size, "%d: %uMhz %s\n",
> @@ -935,7 +909,6 @@ static int rv_get_clock_by_type_with_voltage(struct pp_hwmgr *hwmgr,
>  int rv_display_clock_voltage_request(struct pp_hwmgr *hwmgr,
>                 struct pp_display_clock_request *clock_req)
>  {
> -       int result = 0;
>         struct rv_hwmgr *rv_data = (struct rv_hwmgr *)(hwmgr->backend);
>         enum amd_pp_clock_type clk_type = clock_req->clock_type;
>         uint32_t clk_freq = clock_req->clock_freq_in_khz / 1000;
> @@ -962,10 +935,9 @@ int rv_display_clock_voltage_request(struct pp_hwmgr *hwmgr,
>                 return -EINVAL;
>         }
>
> -       result = smum_send_msg_to_smc_with_parameter(hwmgr, msg,
> -                                                       clk_freq);
> +       smum_send_msg_to_smc_with_parameter(hwmgr, msg, clk_freq);
>
> -       return result;
> +       return 0;
>  }
>
>  static int rv_get_max_high_clocks(struct pp_hwmgr *hwmgr, struct amd_pp_simple_clock_info *clocks)
> @@ -998,22 +970,18 @@ static int rv_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>
>         switch (idx) {
>         case AMDGPU_PP_SENSOR_GFX_SCLK:
> -               ret = smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetGfxclkFrequency);
> -               if (!ret) {
> -                       rv_read_arg_from_smc(hwmgr, &sclk);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetGfxclkFrequency);
> +               rv_read_arg_from_smc(hwmgr, &sclk);
>                         /* in units of 10KHZ */
> -                       *((uint32_t *)value) = sclk * 100;
> -                       *size = 4;
> -               }
> +               *((uint32_t *)value) = sclk * 100;
> +               *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_GFX_MCLK:
> -               ret = smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetFclkFrequency);
> -               if (!ret) {
> -                       rv_read_arg_from_smc(hwmgr, &mclk);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetFclkFrequency);
> +               rv_read_arg_from_smc(hwmgr, &mclk);
>                         /* in units of 10KHZ */
> -                       *((uint32_t *)value) = mclk * 100;
> -                       *size = 4;
> -               }
> +               *((uint32_t *)value) = mclk * 100;
> +               *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_GPU_TEMP:
>                 *((uint32_t *)value) = rv_thermal_get_temperature(hwmgr);
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/rv_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/rv_smumgr.c
> index e6317fd..b64bfe3 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/rv_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/rv_smumgr.c
> @@ -139,20 +139,15 @@ int rv_copy_table_from_smc(struct pp_hwmgr *hwmgr,
>                         "Invalid SMU Table version!", return -EINVAL;);
>         PP_ASSERT_WITH_CODE(priv->smu_tables.entry[table_id].size != 0,
>                         "Invalid SMU Table Length!", return -EINVAL;);
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_SetDriverDramAddrHigh,
> -                       upper_32_bits(priv->smu_tables.entry[table_id].mc_addr)) == 0,
> -                       "[CopyTableFromSMC] Attempt to Set Dram Addr High Failed!", return -EINVAL;);
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       upper_32_bits(priv->smu_tables.entry[table_id].mc_addr));
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_SetDriverDramAddrLow,
> -                       lower_32_bits(priv->smu_tables.entry[table_id].mc_addr)) == 0,
> -                       "[CopyTableFromSMC] Attempt to Set Dram Addr Low Failed!",
> -                       return -EINVAL;);
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       lower_32_bits(priv->smu_tables.entry[table_id].mc_addr));
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_TransferTableSmu2Dram,
> -                       priv->smu_tables.entry[table_id].table_id) == 0,
> -                       "[CopyTableFromSMC] Attempt to Transfer Table From SMU Failed!",
> -                       return -EINVAL;);
> +                       priv->smu_tables.entry[table_id].table_id);
>
>         memcpy(table, (uint8_t *)priv->smu_tables.entry[table_id].table,
>                         priv->smu_tables.entry[table_id].size);
> @@ -176,21 +171,15 @@ int rv_copy_table_to_smc(struct pp_hwmgr *hwmgr,
>         memcpy(priv->smu_tables.entry[table_id].table, table,
>                         priv->smu_tables.entry[table_id].size);
>
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_SetDriverDramAddrHigh,
> -                       upper_32_bits(priv->smu_tables.entry[table_id].mc_addr)) == 0,
> -                       "[CopyTableToSMC] Attempt to Set Dram Addr High Failed!",
> -                       return -EINVAL;);
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       upper_32_bits(priv->smu_tables.entry[table_id].mc_addr));
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_SetDriverDramAddrLow,
> -                       lower_32_bits(priv->smu_tables.entry[table_id].mc_addr)) == 0,
> -                       "[CopyTableToSMC] Attempt to Set Dram Addr Low Failed!",
> -                       return -EINVAL;);
> -       PP_ASSERT_WITH_CODE(rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       lower_32_bits(priv->smu_tables.entry[table_id].mc_addr));
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
>                         PPSMC_MSG_TransferTableDram2Smu,
> -                       priv->smu_tables.entry[table_id].table_id) == 0,
> -                       "[CopyTableToSMC] Attempt to Transfer Table To SMU Failed!",
> -                       return -EINVAL;);
> +                       priv->smu_tables.entry[table_id].table_id);
>
>         return 0;
>  }
> @@ -199,61 +188,43 @@ static int rv_verify_smc_interface(struct pp_hwmgr *hwmgr)
>  {
>         uint32_t smc_driver_if_version;
>
> -       PP_ASSERT_WITH_CODE(!rv_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_GetDriverIfVersion),
> -                       "Attempt to get SMC IF Version Number Failed!",
> -                       return -EINVAL);
> -       PP_ASSERT_WITH_CODE(!rv_read_arg_from_smc(hwmgr,
> -                       &smc_driver_if_version),
> -                       "Attempt to read SMC IF Version Number Failed!",
> -                       return -EINVAL);
> +       rv_send_msg_to_smc(hwmgr,
> +                       PPSMC_MSG_GetDriverIfVersion);
> +       rv_read_arg_from_smc(hwmgr,
> +                       &smc_driver_if_version);
>
> -       if (smc_driver_if_version != SMU10_DRIVER_IF_VERSION)
> +       if (smc_driver_if_version != SMU10_DRIVER_IF_VERSION) {
> +               pr_err("Attempt to read SMC IF Version Number Failed!\n");
>                 return -EINVAL;
> +       }
>
>         return 0;
>  }
>
>  /* sdma is disabled by default in vbios, need to re-enable in driver */
> -static int rv_smc_enable_sdma(struct pp_hwmgr *hwmgr)
> +static void rv_smc_enable_sdma(struct pp_hwmgr *hwmgr)
>  {
> -       PP_ASSERT_WITH_CODE(!rv_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_PowerUpSdma),
> -                       "Attempt to power up sdma Failed!",
> -                       return -EINVAL);
> -
> -       return 0;
> +       rv_send_msg_to_smc(hwmgr,
> +                       PPSMC_MSG_PowerUpSdma);
>  }
>
> -static int rv_smc_disable_sdma(struct pp_hwmgr *hwmgr)
> +static void rv_smc_disable_sdma(struct pp_hwmgr *hwmgr)
>  {
> -       PP_ASSERT_WITH_CODE(!rv_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_PowerDownSdma),
> -                       "Attempt to power down sdma Failed!",
> -                       return -EINVAL);
> -
> -       return 0;
> +       rv_send_msg_to_smc(hwmgr,
> +                       PPSMC_MSG_PowerDownSdma);
>  }
>
>  /* vcn is disabled by default in vbios, need to re-enable in driver */
> -static int rv_smc_enable_vcn(struct pp_hwmgr *hwmgr)
> +static void rv_smc_enable_vcn(struct pp_hwmgr *hwmgr)
>  {
> -       PP_ASSERT_WITH_CODE(!rv_send_msg_to_smc_with_parameter(hwmgr,
> -                       PPSMC_MSG_PowerUpVcn, 0),
> -                       "Attempt to power up vcn Failed!",
> -                       return -EINVAL);
> -
> -       return 0;
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       PPSMC_MSG_PowerUpVcn, 0);
>  }
>
> -static int rv_smc_disable_vcn(struct pp_hwmgr *hwmgr)
> +static void rv_smc_disable_vcn(struct pp_hwmgr *hwmgr)
>  {
> -       PP_ASSERT_WITH_CODE(!rv_send_msg_to_smc_with_parameter(hwmgr,
> -                       PPSMC_MSG_PowerDownVcn, 0),
> -                       "Attempt to power down vcn Failed!",
> -                       return -EINVAL);
> -
> -       return 0;
> +       rv_send_msg_to_smc_with_parameter(hwmgr,
> +                       PPSMC_MSG_PowerDownVcn, 0);
>  }
>
>  static int rv_smu_fini(struct pp_hwmgr *hwmgr)
> @@ -289,11 +260,8 @@ static int rv_start_smu(struct pp_hwmgr *hwmgr)
>
>         if (rv_verify_smc_interface(hwmgr))
>                 return -EINVAL;
> -       if (rv_smc_enable_sdma(hwmgr))
> -               return -EINVAL;
> -       if (rv_smc_enable_vcn(hwmgr))
> -               return -EINVAL;
> -
> +       rv_smc_enable_sdma(hwmgr);
> +       rv_smc_enable_vcn(hwmgr);
>         return 0;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx