On Wed, Mar 7, 2018 at 11:42 PM, Zhu, Rex <Rex.Zhu at amd.com> wrote: >>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. > > Rex: until vega10/raven, we do not handle the errors in windows/linux, just print out the response code for debug. And the smu dispatch functions always return true. So the check code is meanless. > > > There are five responses from smu. Success/not support/busy/no response/need prereq. > > For not support, smu just not recognize the command, hw still work normally. so print out a message is enough for debug or notify user. > For busy or not response, in most case, the hw is hung. and seems no way to reset/recovery smu engine currently. > For need prereq, I think print the error message is enough for debug. > > In the future, if we can handle the errors, we can add in the dispatch functions. Sounds good. Patch is: Reviewed-by: Alex Deucher <alexander.deucher at amd.com> > > Best Regards > Rex > > > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher at gmail.com] > Sent: Thursday, March 08, 2018 1:23 AM > To: Zhu, Rex > Cc: amd-gfx list > Subject: Re: [PATCH 4/8] drm/amd/pp: Clean up the code for RV in powerplay > > On Wed, Mar 7, 2018 at 5:46 AM, Rex Zhu <Rex.Zhu at 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 at 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 at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
