[PATCH 2/9] drm/amd/powerplay: some cosmetic fixes
Alex Deucher
alexdeucher at gmail.com
Tue Jun 2 14:54:37 UTC 2020
On Mon, Jun 1, 2020 at 3:30 AM Evan Quan <evan.quan at amd.com> wrote:
>
> To make code more clean and readable by moving ASIC
> specific code to its own file, more code sharing and
> dropping unused code.
There seem to be multiple things going on here. It's kind of hard to
follow all of the changes. Maybe split this patch up? One additional
comment below.
Alex
>
> Change-Id: I6b299f9e98c7678b48281cbed9beb17b644bb4cc
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 213 ++++++++-------------
> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 19 ++
> 2 files changed, 102 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 4998ea942760..b4f108cb52fa 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -817,22 +817,10 @@ int smu_get_atom_data_table(struct smu_context *smu, uint32_t table,
> return 0;
> }
>
> -static int smu_initialize_pptable(struct smu_context *smu)
> -{
> - /* TODO */
> - return 0;
> -}
> -
> static int smu_smc_table_sw_init(struct smu_context *smu)
> {
> int ret;
>
> - ret = smu_initialize_pptable(smu);
> - if (ret) {
> - pr_err("Failed to init smu_initialize_pptable!\n");
> - return ret;
> - }
> -
> /**
> * Create smu_table structure, and init smc tables such as
> * TABLE_PPTABLE, TABLE_WATERMARKS, TABLE_SMU_METRICS, and etc.
> @@ -860,6 +848,12 @@ static int smu_smc_table_sw_fini(struct smu_context *smu)
> {
> int ret;
>
> + ret = smu_fini_power(smu);
> + if (ret) {
> + pr_err("Failed to init smu_fini_power!\n");
> + return ret;
> + }
> +
> ret = smu_fini_smc_tables(smu);
> if (ret) {
> pr_err("Failed to smu_fini_smc_tables!\n");
> @@ -950,12 +944,6 @@ static int smu_sw_fini(void *handle)
> return ret;
> }
>
> - ret = smu_fini_power(smu);
> - if (ret) {
> - pr_err("Failed to init smu_fini_power!\n");
> - return ret;
> - }
> -
> return 0;
> }
>
> @@ -1125,36 +1113,22 @@ static int smu_smc_table_hw_init(struct smu_context *smu,
> if (ret)
> return ret;
>
> - if (adev->asic_type == CHIP_NAVI10) {
> - if ((adev->pdev->device == 0x731f && (adev->pdev->revision == 0xc2 ||
> - adev->pdev->revision == 0xc3 ||
> - adev->pdev->revision == 0xca ||
> - adev->pdev->revision == 0xcb)) ||
> - (adev->pdev->device == 0x66af && (adev->pdev->revision == 0xf3 ||
> - adev->pdev->revision == 0xf4 ||
> - adev->pdev->revision == 0xf5 ||
> - adev->pdev->revision == 0xf6))) {
> - ret = smu_disable_umc_cdr_12gbps_workaround(smu);
> - if (ret) {
> - pr_err("Workaround failed to disable UMC CDR feature on 12Gbps SKU!\n");
> - return ret;
> - }
> - }
> + ret = smu_disable_umc_cdr_12gbps_workaround(smu);
> + if (ret) {
> + pr_err("Workaround failed to disable UMC CDR feature on 12Gbps SKU!\n");
> + return ret;
> }
>
> - if (smu->ppt_funcs->set_power_source) {
> - /*
> - * For Navi1X, manually switch it to AC mode as PMFW
> - * may boot it with DC mode.
> - */
> - if (adev->pm.ac_power)
> - ret = smu_set_power_source(smu, SMU_POWER_SOURCE_AC);
> - else
> - ret = smu_set_power_source(smu, SMU_POWER_SOURCE_DC);
> - if (ret) {
> - pr_err("Failed to switch to %s mode!\n", adev->pm.ac_power ? "AC" : "DC");
> - return ret;
> - }
> + /*
> + * For Navi1X, manually switch it to AC mode as PMFW
> + * may boot it with DC mode.
> + */
> + ret = smu_set_power_source(smu,
> + adev->pm.ac_power ? SMU_POWER_SOURCE_AC :
> + SMU_POWER_SOURCE_DC);
> + if (ret) {
> + pr_err("Failed to switch to %s mode!\n", adev->pm.ac_power ? "AC" : "DC");
> + return ret;
> }
>
> ret = smu_notify_display_change(smu);
> @@ -1362,9 +1336,65 @@ static int smu_hw_init(void *handle)
> return ret;
> }
>
> -static int smu_stop_dpms(struct smu_context *smu)
> +static int smu_disable_dpms(struct smu_context *smu)
> {
> - return smu_system_features_control(smu, false);
> + struct amdgpu_device *adev = smu->adev;
> + int ret = 0;
> + bool use_baco = !smu->is_apu &&
> + ((adev->in_gpu_reset &&
> + (amdgpu_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)) ||
> + ((adev->in_runpm || adev->in_hibernate) && amdgpu_asic_supports_baco(adev)));
> +
> + /*
> + * For custom pptable uploading, skip the DPM features
> + * disable process on Navi1x ASICs.
> + * - As the gfx related features are under control of
> + * RLC on those ASICs. RLC reinitialization will be
> + * needed to reenable them. That will cost much more
> + * efforts.
> + *
> + * - SMU firmware can handle the DPM reenablement
> + * properly.
> + */
> + if (smu->uploading_custom_pp_table &&
> + (adev->asic_type >= CHIP_NAVI10) &&
> + (adev->asic_type <= CHIP_NAVI12))
> + return 0;
> +
> + /*
> + * Disable all enabled SMU features.
> + * This should be handled in SMU FW, as a backup
> + * driver can issue call to SMU FW until sequence
> + * in SMU FW is operational.
> + */
> + ret = smu_system_features_control(smu, false);
> + if (ret) {
> + pr_err("Failed to disable smu features.\n");
> + return ret;
> + }
> +
> + /*
> + * For baco, need to leave BACO feature enabled
> + *
> + * Correct the way for checking whether SMU_FEATURE_BACO_BIT
> + * is supported.
> + *
> + * Since 'smu_feature_is_enabled(smu, SMU_FEATURE_BACO_BIT)' will
> + * always return false as the 'smu_system_features_control(smu, false)'
> + * was just issued above which disabled all SMU features.
> + *
> + * Thus 'smu_feature_get_index(smu, SMU_FEATURE_BACO_BIT)' is used
> + * now for the checking.
> + */
> + if (use_baco && (smu_feature_get_index(smu, SMU_FEATURE_BACO_BIT) >= 0)) {
> + ret = smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, true);
> + if (ret) {
> + pr_warn("set BACO feature enabled failed, return %d\n", ret);
> + return ret;
> + }
> + }
> +
> + return ret;
> }
>
> static int smu_hw_fini(void *handle)
> @@ -1396,25 +1426,10 @@ static int smu_hw_fini(void *handle)
> return ret;
> }
>
> - /*
> - * For custom pptable uploading, skip the DPM features
> - * disable process on Navi1x ASICs.
> - * - As the gfx related features are under control of
> - * RLC on those ASICs. RLC reinitialization will be
> - * needed to reenable them. That will cost much more
> - * efforts.
> - *
> - * - SMU firmware can handle the DPM reenablement
> - * properly.
> - */
> - if (!smu->uploading_custom_pp_table ||
> - !((adev->asic_type >= CHIP_NAVI10) &&
> - (adev->asic_type <= CHIP_NAVI12))) {
> - ret = smu_stop_dpms(smu);
> - if (ret) {
> - pr_warn("Fail to stop Dpms!\n");
> - return ret;
> - }
> + ret = smu_disable_dpms(smu);
> + if (ret) {
> + pr_warn("Fail to stop Dpms!\n");
> + return ret;
> }
>
> kfree(table_context->driver_pptable);
> @@ -1453,68 +1468,6 @@ int smu_reset(struct smu_context *smu)
> return ret;
> }
>
> -static int smu_disable_dpm(struct smu_context *smu)
> -{
> - struct amdgpu_device *adev = smu->adev;
> - uint32_t smu_version;
> - int ret = 0;
> - bool use_baco = !smu->is_apu &&
> - ((adev->in_gpu_reset &&
> - (amdgpu_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)) ||
> - ((adev->in_runpm || adev->in_hibernate) && amdgpu_asic_supports_baco(adev)));
> -
> - ret = smu_get_smc_version(smu, NULL, &smu_version);
> - if (ret) {
> - pr_err("Failed to get smu version.\n");
> - return ret;
> - }
> -
> - /*
> - * Disable all enabled SMU features.
> - * This should be handled in SMU FW, as a backup
> - * driver can issue call to SMU FW until sequence
> - * in SMU FW is operational.
> - */
> - ret = smu_system_features_control(smu, false);
> - if (ret) {
> - pr_err("Failed to disable smu features.\n");
> - return ret;
> - }
> -
> - /*
> - * Arcturus does not have BACO bit in disable feature mask.
> - * Enablement of BACO bit on Arcturus should be skipped.
> - */
> - if (adev->asic_type == CHIP_ARCTURUS) {
> - if (use_baco && (smu_version > 0x360e00))
> - return 0;
> - }
> -
> - /* For baco, need to leave BACO feature enabled */
> - if (use_baco) {
> - /*
> - * Correct the way for checking whether SMU_FEATURE_BACO_BIT
> - * is supported.
> - *
> - * Since 'smu_feature_is_enabled(smu, SMU_FEATURE_BACO_BIT)' will
> - * always return false as the 'smu_system_features_control(smu, false)'
> - * was just issued above which disabled all SMU features.
> - *
> - * Thus 'smu_feature_get_index(smu, SMU_FEATURE_BACO_BIT)' is used
> - * now for the checking.
> - */
> - if (smu_feature_get_index(smu, SMU_FEATURE_BACO_BIT) >= 0) {
> - ret = smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, true);
> - if (ret) {
> - pr_warn("set BACO feature enabled failed, return %d\n", ret);
> - return ret;
> - }
> - }
> - }
> -
> - return ret;
> -}
> -
> static int smu_suspend(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> @@ -1537,7 +1490,7 @@ static int smu_suspend(void *handle)
> return ret;
> }
>
> - ret = smu_disable_dpm(smu);
> + ret = smu_disable_dpms(smu);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> index 68142f6798c6..652728f18271 100644
> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> @@ -2209,12 +2209,31 @@ static int navi10_dummy_pstate_control(struct smu_context *smu, bool enable)
> return result;
> }
>
> +static inline bool navi10_need_umc_cdr_12gbps_workaround(struct amdgpu_device *adev)
> +{
> + if (adev->asic_type != CHIP_NAVI10)
> + return false;
> +
> + if ((adev->pdev->device == 0x731f && (adev->pdev->revision == 0xc2 ||
> + adev->pdev->revision == 0xc3 || adev->pdev->revision == 0xca ||
> + adev->pdev->revision == 0xcb)) ||
> + (adev->pdev->device == 0x66af && (adev->pdev->revision == 0xf3 ||
> + adev->pdev->revision == 0xf4 || adev->pdev->revision == 0xf5 ||
> + adev->pdev->revision == 0xf6)))
> + return true;
> + else
> + return false;
> +}
Do we need a separate function for this or can we just inline this
code in the function below?
> +
> static int navi10_disable_umc_cdr_12gbps_workaround(struct smu_context *smu)
> {
> uint32_t uclk_count, uclk_min, uclk_max;
> uint32_t smu_version;
> int ret = 0;
>
> + if (!navi10_need_umc_cdr_12gbps_workaround(smu->adev))
> + return 0;
> +
> ret = smu_get_smc_version(smu, NULL, &smu_version);
> if (ret)
> return ret;
> --
> 2.26.2
>
> _______________________________________________
> 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