The patch seems fine but I found it super confusing that sometimes `pstate` is a pointer (for example `clk->pstate`), sometimes it is an int (for example `args->v0.pstate`).
On 2017-09-15 — 17:11, Karol Herbst wrote:
> We will access the current cstate at least every second and this saves us
> some CPU cycles looking them up every second.
>
> v2: Rewording commit message.
>
> Signed-off-by: Karol Herbst <karolher...@gmail.com>
> Reviewed-by: Martin Peres <martin.pe...@free.fr>
> ---
> drm/nouveau/include/nvkm/subdev/clk.h | 4 +++-
> drm/nouveau/nvkm/engine/device/ctrl.c | 5 ++++-
> drm/nouveau/nvkm/subdev/clk/base.c | 17 ++++++++++++-----
> drm/nouveau/nvkm/subdev/pmu/gk20a.c | 18 +++++++-----------
> 4 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
> b/drm/nouveau/include/nvkm/subdev/clk.h
> index 1340f5b8..ec537e08 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -10,6 +10,8 @@ struct nvkm_pll_vals;
> #define NVKM_CLK_CSTATE_BASE -2 /* pstate base */
> #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
>
> +#define NVKM_CLK_PSTATE_DEFAULT -1
> +
> enum nv_clk_src {
> nv_clk_src_crystal,
> nv_clk_src_href,
> @@ -95,7 +97,7 @@ struct nvkm_clk {
>
> struct nvkm_notify pwrsrc_ntfy;
> int pwrsrc;
> - int pstate; /* current */
> + struct nvkm_pstate *pstate; /* current */
> int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
> int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
> int astate; /* perfmon adjustment (base) */
> diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c
> b/drm/nouveau/nvkm/engine/device/ctrl.c
> index b0ece71a..da70626c 100644
> --- a/drm/nouveau/nvkm/engine/device/ctrl.c
> +++ b/drm/nouveau/nvkm/engine/device/ctrl.c
> @@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl,
> void *data, u32 size)
> args->v0.ustate_ac = clk->ustate_ac;
> args->v0.ustate_dc = clk->ustate_dc;
> args->v0.pwrsrc = clk->pwrsrc;
> - args->v0.pstate = clk->pstate;
> + if (clk->pstate)
> + args->v0.pstate = clk->pstate->pstate;
> + else
> + args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
> } else {
> args->v0.count = 0;
> args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c
> b/drm/nouveau/nvkm/subdev/clk/base.c
> index 07d530ed..0d4d9fdf 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
> struct nvkm_pstate *pstate;
> int ret, idx = 0;
>
> + if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
> + return 0;
> +
> list_for_each_entry(pstate, &clk->states, head) {
> if (idx++ == pstatei)
> break;
> }
>
> nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> - clk->pstate = pstatei;
> + clk->pstate = pstate;
>
> nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>
> @@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
> return;
> clk->pwrsrc = power_supply_is_system_supplied();
>
> + if (clk->pstate)
> + pstate = clk->pstate->pstate;
> + else
> + pstate = NVKM_CLK_PSTATE_DEFAULT;
> nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
> - clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> + pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> clk->astate, clk->temp);
>
> pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
In the if-statement following the previous line, the -1 could be replaced by
NVKM_CLK_PSTATE_DEFAULT.
> @@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
> pstate = (pstate < 0) ? clk->astate : pstate;
Can `pstate` have negative values other than -1?
> pstate = min(pstate, clk->state_nr - 1);
> } else {
> - pstate = clk->pstate = -1;
> + pstate = NVKM_CLK_PSTATE_DEFAULT;
Shouldn’t you also reset `clk->pstate` to NULL? Or maybe it is
`nvkm_pstate_prog()` which should do it if
`pstatei == NVKM_CLK_PSTATE_DEFAULT`.
> }
>
> nvkm_trace(subdev, "-> %d\n", pstate);
> - if (pstate != clk->pstate) {
> + if (!clk->pstate || pstate != clk->pstate->pstate) {
> int ret = nvkm_pstate_prog(clk, pstate);
> if (ret) {
> nvkm_error(subdev, "error setting pstate %d: %d\n",
> @@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
> return clk->func->init(clk);
>
> clk->astate = clk->state_nr - 1;
> - clk->pstate = -1;
> + clk->pstate = NULL;
> clk->temp = 90; /* reasonable default value */
> nvkm_clk_update(clk, true);
> return 0;
> diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> index 05e81855..de579726 100644
> --- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> +++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> @@ -55,24 +55,22 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
> return nvkm_clk_astate(clk, *state, 0, false);
> }
>
> -static void
> -gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
> -{
> - struct nvkm_clk *clk = pmu->base.subdev.device->clk;
> -
> - *state = clk->pstate;
> -}
> -
> static int
> gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
> int *state, int load)
> {
> struct gk20a_pmu_dvfs_data *data = pmu->data;
> struct nvkm_clk *clk = pmu->base.subdev.device->clk;
> + struct nvkm_pstate *pstate = clk->pstate;
> int cur_level, level;
>
> + if (!pstate) {
> + *state = 0;
> + return 1;
> + }
> +
> /* For GK20A, the performance level is directly mapped to pstate */
> - level = cur_level = clk->pstate;
> + level = cur_level = clk->pstate->pstate;
>
> if (load > data->p_load_max) {
> level = min(clk->state_nr - 1, level + (clk->state_nr / 3));
> @@ -142,8 +140,6 @@ gk20a_pmu_dvfs_work(struct nvkm_alarm *alarm)
> nvkm_trace(subdev, "utilization = %d %%, avg_load = %d %%\n",
> utilization, data->avg_load);
>
> - gk20a_pmu_dvfs_get_cur_state(pmu, &state);
> -
> if (gk20a_pmu_dvfs_get_target_state(pmu, &state, data->avg_load)) {
> nvkm_trace(subdev, "set new state to %d\n", state);
> gk20a_pmu_dvfs_target(pmu, &state);
> --
> 2.14.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
signature.asc
Description: PGP signature
_______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
