[PATCH 4/8] drm/amd/pp: Clean up the code for RV in powerplay
Alex Deucher
alexdeucher at gmail.com
Thu Mar 8 13:37:57 UTC 2018
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
More information about the amd-gfx
mailing list