On Sun, Oct 8, 2017 at 1:47 PM, Pierre Moreau <[email protected]> wrote: > 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`). >
yeah I still plan to clean the names up when I find some time to finish those patches for real > 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 <[email protected]> >> Reviewed-by: Martin Peres <[email protected]> >> --- >> 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? not yet, but >=0 means a pstate is selected by an id, <0 but a type, but it might be that we'll never need more than the default one. > >> 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`. > no. We can't go back to the default clocks except we run devinit again. Default means: state after powering on the GPU. and clk->pstate points to the current pstate we reclocked to last. Which means if we hit the else branch here, clk->pstate is already NULL. >> } >> >> 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 >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/nouveau _______________________________________________ Nouveau mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/nouveau
