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

[Timur Kristóf]

On Mon, 2025-08-04 at 11:32 -0400, Alex Deucher wrote:
> 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.

Actually now that I think of it more, I think it may be entirely
unnecessary to check the return value.

si_upload_smc_data calls:
si_write_smc_soft_register
amdgpu_si_write_smc_sram_dword
si_set_smc_sram_address

This last one, si_set_smc_sram_address returns -EINVAL when its
smc_address parameter is not dword-aligned or out of bounds. Otherwise
all of the above functions return 0 (success). Considering that all of
the addresses passed by si_upload_smc_data are compile time constants,
we know they are correct so there is no reason why any of those
functions would return an error.

Looking at other callers of si_write_smc_soft_register, I see that they
don't check the return value at all.

So, I'd actually simplify this even more and just not check the return
values. What do you think about that?


> 
> 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
> >