* Re: [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization
2020-09-30 4:07 [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization Evan Quan
@ 2020-09-30 9:53 ` Dirk Gouders
2020-09-30 10:42 ` Nirmoy
2020-09-30 13:10 ` Alex Deucher
2 siblings, 0 replies; 4+ messages in thread
From: Dirk Gouders @ 2020-09-30 9:53 UTC (permalink / raw)
To: Evan Quan; +Cc: Alexander.Deucher, Ray.Huang, amd-gfx
Evan Quan <evan.quan@amd.com> writes:
> As the dpm clock table is needed during DC HW initialization.
> And that (DC HW initialization) comes before smu_late_init()
> where current APU dpm clock table setup is performed. So, NULL
> pointer dereference will be triggered. By moving APU dpm clock
> table setup to smu_hw_init(), this can be avoided.
Thanks for the quick response. I tested the patch and it fixes the call
trace I initially reportet (#1 in the table below). #2 is unaffected by
this patch. I could try to bisect it as well bud did not do it, so far.
Probably, I caused some confusion in the original thread and I will try to
order it a bit. What I noticed is:
with amdgpu.discovery value | noticed issue
===============================================================
1) unset or "1" | call trace because of
| assert(0) in rn_clk_mgr_helper_populate_bw_params()
-------------------------------+-------------------------------
2) 0 | NULL pointer dereference in soc15_set_ip_blocks()
This patch fixes #1, i.e. avoids the assert() in following code in
drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
for (i = PP_SMU_NUM_FCLK_DPM_LEVELS - 1; i >= 0; i--) {
if (clock_table->FClocks[i].Freq != 0 && clock_table->FClocks[i].Vol != 0) {
j = i;
break;
}
}
if (j == -1) {
/* clock table is all 0s, just use our own hardcode */
ASSERT(0);
return;
}
To me, the commit message sounds as if the patch fixes #2 whereas it
really is #1 that gets fixed. I also wonder if we probably want a
fixes-line for completeness:
Fixes: 02cf91c113ea (drm/amd/powerplay: postpone operations not required for hw setup to late_init)
Dirk
> Change-Id: I2bb1f9ba26f9c8820c08241da62f7be64ab75840
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Reported-by: Dirk Gouders <dirk@gouders.net>
> ---
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index f46cf9ea355e..8f6045def272 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -482,17 +482,6 @@ static int smu_late_init(void *handle)
> return ret;
> }
>
> - /*
> - * Set initialized values (get from vbios) to dpm tables context such as
> - * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> - * type of clks.
> - */
> - ret = smu_set_default_dpm_table(smu);
> - if (ret) {
> - dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> - return ret;
> - }
> -
> ret = smu_populate_umd_state_clk(smu);
> if (ret) {
> dev_err(adev->dev, "Failed to populate UMD state clocks!\n");
> @@ -1021,6 +1010,17 @@ static int smu_smc_hw_setup(struct smu_context *smu)
> return ret;
> }
>
> + /*
> + * Set initialized values (get from vbios) to dpm tables context such as
> + * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> + * type of clks.
> + */
> + ret = smu_set_default_dpm_table(smu);
> + if (ret) {
> + dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> + return ret;
> + }
> +
> ret = smu_notify_display_change(smu);
> if (ret)
> return ret;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization
2020-09-30 4:07 [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization Evan Quan
2020-09-30 9:53 ` Dirk Gouders
@ 2020-09-30 10:42 ` Nirmoy
2020-09-30 13:10 ` Alex Deucher
2 siblings, 0 replies; 4+ messages in thread
From: Nirmoy @ 2020-09-30 10:42 UTC (permalink / raw)
To: amd-gfx
Acked-by: Nirmoy Das <nirmoy.das@amd.com>
On 9/30/20 6:07 AM, Evan Quan wrote:
> As the dpm clock table is needed during DC HW initialization.
> And that (DC HW initialization) comes before smu_late_init()
> where current APU dpm clock table setup is performed. So, NULL
> pointer dereference will be triggered. By moving APU dpm clock
> table setup to smu_hw_init(), this can be avoided.
>
> Change-Id: I2bb1f9ba26f9c8820c08241da62f7be64ab75840
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Reported-by: Dirk Gouders <dirk@gouders.net>
> ---
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index f46cf9ea355e..8f6045def272 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -482,17 +482,6 @@ static int smu_late_init(void *handle)
> return ret;
> }
>
> - /*
> - * Set initialized values (get from vbios) to dpm tables context such as
> - * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> - * type of clks.
> - */
> - ret = smu_set_default_dpm_table(smu);
> - if (ret) {
> - dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> - return ret;
> - }
> -
> ret = smu_populate_umd_state_clk(smu);
> if (ret) {
> dev_err(adev->dev, "Failed to populate UMD state clocks!\n");
> @@ -1021,6 +1010,17 @@ static int smu_smc_hw_setup(struct smu_context *smu)
> return ret;
> }
>
> + /*
> + * Set initialized values (get from vbios) to dpm tables context such as
> + * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> + * type of clks.
> + */
> + ret = smu_set_default_dpm_table(smu);
> + if (ret) {
> + dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> + return ret;
> + }
> +
> ret = smu_notify_display_change(smu);
> if (ret)
> return ret;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization
2020-09-30 4:07 [PATCH] drm/amd/pm: setup APU dpm clock table in SMU HW initialization Evan Quan
2020-09-30 9:53 ` Dirk Gouders
2020-09-30 10:42 ` Nirmoy
@ 2020-09-30 13:10 ` Alex Deucher
2 siblings, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2020-09-30 13:10 UTC (permalink / raw)
To: Evan Quan; +Cc: Deucher, Alexander, Dirk Gouders, Huang Rui, amd-gfx list
On Wed, Sep 30, 2020 at 12:08 AM Evan Quan <evan.quan@amd.com> wrote:
>
> As the dpm clock table is needed during DC HW initialization.
> And that (DC HW initialization) comes before smu_late_init()
> where current APU dpm clock table setup is performed. So, NULL
> pointer dereference will be triggered. By moving APU dpm clock
> table setup to smu_hw_init(), this can be avoided.
>
> Change-Id: I2bb1f9ba26f9c8820c08241da62f7be64ab75840
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Reported-by: Dirk Gouders <dirk@gouders.net>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index f46cf9ea355e..8f6045def272 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -482,17 +482,6 @@ static int smu_late_init(void *handle)
> return ret;
> }
>
> - /*
> - * Set initialized values (get from vbios) to dpm tables context such as
> - * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> - * type of clks.
> - */
> - ret = smu_set_default_dpm_table(smu);
> - if (ret) {
> - dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> - return ret;
> - }
> -
> ret = smu_populate_umd_state_clk(smu);
> if (ret) {
> dev_err(adev->dev, "Failed to populate UMD state clocks!\n");
> @@ -1021,6 +1010,17 @@ static int smu_smc_hw_setup(struct smu_context *smu)
> return ret;
> }
>
> + /*
> + * Set initialized values (get from vbios) to dpm tables context such as
> + * gfxclk, memclk, dcefclk, and etc. And enable the DPM feature for each
> + * type of clks.
> + */
> + ret = smu_set_default_dpm_table(smu);
> + if (ret) {
> + dev_err(adev->dev, "Failed to setup default dpm clock tables!\n");
> + return ret;
> + }
> +
> ret = smu_notify_display_change(smu);
> if (ret)
> return ret;
> --
> 2.28.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread