[Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate
Karol Herbst
karolherbst at gmail.com
Sun Oct 8 14:23:23 UTC 2017
On Sun, Oct 8, 2017 at 1:47 PM, Pierre Moreau <pierre.morrow at free.fr> 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 <karolherbst at gmail.com>
>> Reviewed-by: Martin Peres <martin.peres at 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?
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
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
More information about the Nouveau
mailing list