On Mon, Aug 4, 2025 at 9:42 AM Timur Kristóf <timur.kris...@gmail.com> wrote:
>
> The si_upload_smc_data function uses si_write_smc_soft_register
> to set some register values in the SMC, and expects the result
> to be PPSMC_Result_OK which is 1.
>
> The PPSMC_Result_OK / PPSMC_Result_Failed values are used for
> checking the result of a command sent to the SMC.
>
> However, the si_write_smc_soft_register actually doesn't send
> any commands to the SMC and returns zero on success,
> so this check was incorrect.
>
> Fix that by correctly interpreting zero as success.
> This seems to fix an SMC hang that happens in si_set_sw_state.
>
> Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
> Signed-off-by: Timur Kristóf <timur.kris...@gmail.com>
> ---
> drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 31 +++++++++++++---------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index 33b9d4beec84..e9f034ade214 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> @@ -5820,6 +5820,7 @@ static int si_upload_smc_data(struct amdgpu_device
> *adev)
> {
> struct amdgpu_crtc *amdgpu_crtc = NULL;
> int i;
> + int ret;
>
> if (adev->pm.dpm.new_active_crtc_count == 0)
> return 0;
> @@ -5837,20 +5838,26 @@ static int si_upload_smc_data(struct amdgpu_device
> *adev)
> if (amdgpu_crtc->line_time <= 0)
> return 0;
>
> - if (si_write_smc_soft_register(adev,
> - SI_SMC_SOFT_REGISTER_crtc_index,
> - amdgpu_crtc->crtc_id) !=
> PPSMC_Result_OK)
> - return 0;
> + ret = si_write_smc_soft_register(
> + adev,
> + SI_SMC_SOFT_REGISTER_crtc_index,
> + amdgpu_crtc->crtc_id);
> + if (ret)
> + return ret;
>
> - if (si_write_smc_soft_register(adev,
> -
> SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
> - amdgpu_crtc->wm_high /
> amdgpu_crtc->line_time) != PPSMC_Result_OK)
> - return 0;
> + ret = si_write_smc_soft_register(
> + adev,
> + SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
> + amdgpu_crtc->wm_high / amdgpu_crtc->line_time);
> + if (ret)
> + return ret;
>
> - if (si_write_smc_soft_register(adev,
> -
> SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
> - amdgpu_crtc->wm_low /
> amdgpu_crtc->line_time) != PPSMC_Result_OK)
> - return 0;
> + ret = si_write_smc_soft_register(
> + adev,
> + SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
> + amdgpu_crtc->wm_low / amdgpu_crtc->line_time);
> + if (ret)
> + return ret;
This patch changes the behavior of this function (i.e., it always
returns 0 before this patch). Not sure if that matters or not. I
think this could be simplified to something like the following to
retain the current behavior.
diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index 52e732be59e36..3dd0115aa15f8 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -5836,17 +5836,17 @@ static int si_upload_smc_data(struct
amdgpu_device *adev)
if (si_write_smc_soft_register(adev,
SI_SMC_SOFT_REGISTER_crtc_index,
- amdgpu_crtc->crtc_id) != PPSMC_Result_OK)
+ amdgpu_crtc->crtc_id))
return 0;
if (si_write_smc_soft_register(adev,
SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
- amdgpu_crtc->wm_high /
amdgpu_crtc->line_time) != PPSMC_Result_OK)
+ amdgpu_crtc->wm_high /
amdgpu_crtc->line_time))
return 0;
if (si_write_smc_soft_register(adev,
SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
- amdgpu_crtc->wm_low /
amdgpu_crtc->line_time) != PPSMC_Result_OK)
+ amdgpu_crtc->wm_low /
amdgpu_crtc->line_time))
return 0;
return 0;
>
> return 0;
> }
> --
> 2.50.1
>
