Re: [PATCH] drm/radeon/si_dpm: Fix SMU power state load

2021-05-10 Thread Alex Deucher 
Applied.  Thanks!

Alex

On Mon, May 10, 2021 at 1:51 AM Kai-Heng Feng
 wrote:
>
> On Mon, May 10, 2021 at 6:54 AM Gustavo A. R. Silva
>  wrote:
> >
> > Create new structure SISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
> > and ACPIState.levels are never actually used as flexible arrays. Those
> > arrays can be used as simple objects of type
> > SISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
> >
> > Currently, the code fails because flexible array _levels_ in
> > struct SISLANDS_SMC_SWSTATE doesn't allow for code that access
> > the first element of initialState.levels and ACPIState.levels
> > arrays:
> >
> > 4353 table->initialState.levels[0].mclk.vDLL_CNTL =
> > 4354 cpu_to_be32(si_pi->clock_registers.dll_cntl);
> > ...
> > 4555 table->ACPIState.levels[0].mclk.vDLL_CNTL =
> > 4556 cpu_to_be32(dll_cntl);
> >
> > because such element cannot exist without previously allocating
> > any dynamic memory for it (which never actually happens).
> >
> > That's why struct SISLANDS_SMC_SWSTATE should only be used as type
> > for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
> > created as type for objects initialState, ACPIState and ULVState.
> >
> > Also, with the change from one-element array to flexible-array member
> > in commit 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array
> > with flexible-array in struct SISLANDS_SMC_SWSTATE"), the size of
> > dpmLevels in struct SISLANDS_SMC_STATETABLE should be fixed to be
> > SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
> > SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
> >
> > Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1583
> > Fixes: 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array with 
> > flexible-array in struct SISLANDS_SMC_SWSTATE")
> > Cc: sta...@vger.kernel.org
> > Reported-by: Kai-Heng Feng 
> > Signed-off-by: Gustavo A. R. Silva 
>
> Tested-by: Kai-Heng Feng 
>
> > ---
> >  drivers/gpu/drm/radeon/si_dpm.c   | 174 +-
> >  drivers/gpu/drm/radeon/sislands_smc.h |  34 +++--
> >  2 files changed, 109 insertions(+), 99 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/si_dpm.c 
> > b/drivers/gpu/drm/radeon/si_dpm.c
> > index 91bfc4762767..2a8b9680cf6b 100644
> > --- a/drivers/gpu/drm/radeon/si_dpm.c
> > +++ b/drivers/gpu/drm/radeon/si_dpm.c
> > @@ -4350,70 +4350,70 @@ static int si_populate_smc_initial_state(struct 
> > radeon_device *rdev,
> > u32 reg;
> > int ret;
> >
> > -   table->initialState.levels[0].mclk.vDLL_CNTL =
> > +   table->initialState.level.mclk.vDLL_CNTL =
> > cpu_to_be32(si_pi->clock_registers.dll_cntl);
> > -   table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
> > +   table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
> > cpu_to_be32(si_pi->clock_registers.mclk_pwrmgt_cntl);
> > -   table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> > +   table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
> > cpu_to_be32(si_pi->clock_registers.mpll_ad_func_cntl);
> > -   table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
> > +   table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
> > cpu_to_be32(si_pi->clock_registers.mpll_dq_func_cntl);
> > -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL =
> > +   table->initialState.level.mclk.vMPLL_FUNC_CNTL =
> > cpu_to_be32(si_pi->clock_registers.mpll_func_cntl);
> > -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_1 =
> > +   table->initialState.level.mclk.vMPLL_FUNC_CNTL_1 =
> > cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_1);
> > -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_2 =
> > +   table->initialState.level.mclk.vMPLL_FUNC_CNTL_2 =
> > cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_2);
> > -   table->initialState.levels[0].mclk.vMPLL_SS =
> > +   table->initialState.level.mclk.vMPLL_SS =
> > cpu_to_be32(si_pi->clock_registers.mpll_ss1);
> > -   table->initialState.levels[0].mclk.vMPLL_SS2 =
> > +   table->initialState.level.mclk.vMPLL_SS2 =
> > cpu_to_be32(si_pi->clock_registers.mpll_ss2);
> >
> > -   table->initialState.levels[0].mclk.mclk_value =
> > +   table->initialState.level.mclk.mclk_value =
> > cpu_to_be32(initial_state->performance_levels[0].mclk);
> >
> > -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
> > +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
> > cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl);
> > -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
> > +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
> > cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_2);
> > -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
> > +

Re: [PATCH] drm/radeon/si_dpm: Fix SMU power state load

2021-05-09 Thread Kai-Heng Feng 
On Mon, May 10, 2021 at 6:54 AM Gustavo A. R. Silva
 wrote:
>
> Create new structure SISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
> and ACPIState.levels are never actually used as flexible arrays. Those
> arrays can be used as simple objects of type
> SISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
>
> Currently, the code fails because flexible array _levels_ in
> struct SISLANDS_SMC_SWSTATE doesn't allow for code that access
> the first element of initialState.levels and ACPIState.levels
> arrays:
>
> 4353 table->initialState.levels[0].mclk.vDLL_CNTL =
> 4354 cpu_to_be32(si_pi->clock_registers.dll_cntl);
> ...
> 4555 table->ACPIState.levels[0].mclk.vDLL_CNTL =
> 4556 cpu_to_be32(dll_cntl);
>
> because such element cannot exist without previously allocating
> any dynamic memory for it (which never actually happens).
>
> That's why struct SISLANDS_SMC_SWSTATE should only be used as type
> for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
> created as type for objects initialState, ACPIState and ULVState.
>
> Also, with the change from one-element array to flexible-array member
> in commit 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array
> with flexible-array in struct SISLANDS_SMC_SWSTATE"), the size of
> dpmLevels in struct SISLANDS_SMC_STATETABLE should be fixed to be
> SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
> SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
>
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1583
> Fixes: 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array with 
> flexible-array in struct SISLANDS_SMC_SWSTATE")
> Cc: sta...@vger.kernel.org
> Reported-by: Kai-Heng Feng 
> Signed-off-by: Gustavo A. R. Silva 

Tested-by: Kai-Heng Feng 

> ---
>  drivers/gpu/drm/radeon/si_dpm.c   | 174 +-
>  drivers/gpu/drm/radeon/sislands_smc.h |  34 +++--
>  2 files changed, 109 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
> index 91bfc4762767..2a8b9680cf6b 100644
> --- a/drivers/gpu/drm/radeon/si_dpm.c
> +++ b/drivers/gpu/drm/radeon/si_dpm.c
> @@ -4350,70 +4350,70 @@ static int si_populate_smc_initial_state(struct 
> radeon_device *rdev,
> u32 reg;
> int ret;
>
> -   table->initialState.levels[0].mclk.vDLL_CNTL =
> +   table->initialState.level.mclk.vDLL_CNTL =
> cpu_to_be32(si_pi->clock_registers.dll_cntl);
> -   table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
> +   table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
> cpu_to_be32(si_pi->clock_registers.mclk_pwrmgt_cntl);
> -   table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> +   table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
> cpu_to_be32(si_pi->clock_registers.mpll_ad_func_cntl);
> -   table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
> +   table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
> cpu_to_be32(si_pi->clock_registers.mpll_dq_func_cntl);
> -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL =
> +   table->initialState.level.mclk.vMPLL_FUNC_CNTL =
> cpu_to_be32(si_pi->clock_registers.mpll_func_cntl);
> -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_1 =
> +   table->initialState.level.mclk.vMPLL_FUNC_CNTL_1 =
> cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_1);
> -   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_2 =
> +   table->initialState.level.mclk.vMPLL_FUNC_CNTL_2 =
> cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_2);
> -   table->initialState.levels[0].mclk.vMPLL_SS =
> +   table->initialState.level.mclk.vMPLL_SS =
> cpu_to_be32(si_pi->clock_registers.mpll_ss1);
> -   table->initialState.levels[0].mclk.vMPLL_SS2 =
> +   table->initialState.level.mclk.vMPLL_SS2 =
> cpu_to_be32(si_pi->clock_registers.mpll_ss2);
>
> -   table->initialState.levels[0].mclk.mclk_value =
> +   table->initialState.level.mclk.mclk_value =
> cpu_to_be32(initial_state->performance_levels[0].mclk);
>
> -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
> +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
> cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl);
> -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
> +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
> cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_2);
> -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
> +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
> cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_3);
> -   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
> +   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
>

[PATCH] drm/radeon/si_dpm: Fix SMU power state load

2021-05-09 Thread Gustavo A. R. Silva 
Create new structure SISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
and ACPIState.levels are never actually used as flexible arrays. Those
arrays can be used as simple objects of type
SISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.

Currently, the code fails because flexible array _levels_ in
struct SISLANDS_SMC_SWSTATE doesn't allow for code that access
the first element of initialState.levels and ACPIState.levels
arrays:

4353 table->initialState.levels[0].mclk.vDLL_CNTL =
4354 cpu_to_be32(si_pi->clock_registers.dll_cntl);
...
4555 table->ACPIState.levels[0].mclk.vDLL_CNTL =
4556 cpu_to_be32(dll_cntl);

because such element cannot exist without previously allocating
any dynamic memory for it (which never actually happens).

That's why struct SISLANDS_SMC_SWSTATE should only be used as type
for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
created as type for objects initialState, ACPIState and ULVState.

Also, with the change from one-element array to flexible-array member
in commit 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array
with flexible-array in struct SISLANDS_SMC_SWSTATE"), the size of
dpmLevels in struct SISLANDS_SMC_STATETABLE should be fixed to be
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.

Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1583
Fixes: 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array with 
flexible-array in struct SISLANDS_SMC_SWSTATE")
Cc: sta...@vger.kernel.org
Reported-by: Kai-Heng Feng 
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/gpu/drm/radeon/si_dpm.c   | 174 +-
 drivers/gpu/drm/radeon/sislands_smc.h |  34 +++--
 2 files changed, 109 insertions(+), 99 deletions(-)

diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
index 91bfc4762767..2a8b9680cf6b 100644
--- a/drivers/gpu/drm/radeon/si_dpm.c
+++ b/drivers/gpu/drm/radeon/si_dpm.c
@@ -4350,70 +4350,70 @@ static int si_populate_smc_initial_state(struct 
radeon_device *rdev,
u32 reg;
int ret;
 
-   table->initialState.levels[0].mclk.vDLL_CNTL =
+   table->initialState.level.mclk.vDLL_CNTL =
cpu_to_be32(si_pi->clock_registers.dll_cntl);
-   table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
+   table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(si_pi->clock_registers.mclk_pwrmgt_cntl);
-   table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
+   table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_ad_func_cntl);
-   table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
+   table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_dq_func_cntl);
-   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL =
+   table->initialState.level.mclk.vMPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl);
-   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_1 =
+   table->initialState.level.mclk.vMPLL_FUNC_CNTL_1 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_1);
-   table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_2 =
+   table->initialState.level.mclk.vMPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_2);
-   table->initialState.levels[0].mclk.vMPLL_SS =
+   table->initialState.level.mclk.vMPLL_SS =
cpu_to_be32(si_pi->clock_registers.mpll_ss1);
-   table->initialState.levels[0].mclk.vMPLL_SS2 =
+   table->initialState.level.mclk.vMPLL_SS2 =
cpu_to_be32(si_pi->clock_registers.mpll_ss2);
 
-   table->initialState.levels[0].mclk.mclk_value =
+   table->initialState.level.mclk.mclk_value =
cpu_to_be32(initial_state->performance_levels[0].mclk);
 
-   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
+   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl);
-   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
+   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_2);
-   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
+   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_3);
-   table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
+   table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_4);
-   table->initialState.levels[0].sclk.vCG_SPLL_SPREAD_SPECTRUM =
+   table->initialState.level.sclk.vCG_SPLL_SPREAD_SPECTRUM =
cpu_to_be32(si_pi->clock_registers.cg_spll_spread_spectrum);