[PATCH 4/6] drm/amd/pm: Fix si_upload_smc_data

[Alex Deucher]

On Mon, Aug 4, 2025 at 9:42 AM Timur Kristóf <timur.kristof at 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.kristof at 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
>