[PATCH 4/6] drm/amd/pm: Fix si_upload_smc_data
Timur Kristóf
timur.kristof at gmail.com
Fri Aug 8 08:22:46 UTC 2025
On Mon, 2025-08-04 at 13:31 -0400, Alex Deucher wrote:
> On Mon, Aug 4, 2025 at 12:16 PM Timur Kristóf
> <timur.kristof at gmail.com> wrote:
> >
> > 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?
>
> Sure. Works for me.
>
> Alex
Alex, before I send a new version of this series, can you please
clarify what these registers are and verify that the actual programming
of these SMC registers is correct?
The reason I ask is because due the the bug being fixed by these patch,
these registers were never actually written, which makes me wonder if
the value we program them to is actually correct.
I mean the values that we program these registers to:
SI_SMC_SOFT_REGISTER_crtc_index - we just program the index of the
first active CRTC, seems straightforward enough, but it's unclear what
the SMC uses this for. Why does the SMC care which crtc we use?
SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min - programmed to the high
display watermark divided by the line time. But I can't find any
information about what this information represents or what the SMC uses
it for. Judging by the name it has to do with mclk switching?
SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max - same concern as _min.
Thanks,
Timur
>
> >
> >
> > >
> > > 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
> > > >
More information about the amd-gfx
mailing list