[Nouveau] [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list
Karol Herbst
karolherbst at gmail.com
Sun Oct 8 14:43:26 UTC 2017
On Sun, Oct 8, 2017 at 4:23 PM, Pierre Moreau <pierre.morrow at free.fr> wrote:
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> This makes the code easier, because we can compare the id with
>> pstate->pstate and saves us from the trouble of iterating over the pstates
>> to match the index.
>
> I don’t remember whether I have already done this comment before, but I am not
> sure where your iterating over the pstates savings are, as in this patch at
> least, you iterate as much as you did before.
>
yeah I think I ended up adding one loop again to check whether the
input was actual an existing pstate or not, but it shouldn't be needed
anymore. It was required before, now it's only there for error
checking.
> A few more comments below
>
>> v2: reword commit message
>>
>> Signed-off-by: Karol Herbst <karolherbst at gmail.com>
>> Reviewed-by: Martin Peres <martin.peres at free.fr>
>> ---
>> drm/nouveau/nouveau_debugfs.c | 6 +--
>> drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
>> 2 files changed, 41 insertions(+), 43 deletions(-)
>>
>> diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
>> index 963a4dba..27281c4e 100644
>> --- a/drm/nouveau/nouveau_debugfs.c
>> +++ b/drm/nouveau/nouveau_debugfs.c
>> @@ -96,11 +96,11 @@ nouveau_debugfs_pstate_get(struct seq_file *m, void *data)
>> } while (attr.index);
>>
>> if (state >= 0) {
>> - if (info.ustate_ac == state)
>> + if (info.ustate_ac == attr.state)
>> seq_printf(m, " AC");
>> - if (info.ustate_dc == state)
>> + if (info.ustate_dc == attr.state)
>> seq_printf(m, " DC");
>> - if (info.pstate == state)
>> + if (info.pstate == attr.state)
>> seq_printf(m, " *");
>> } else {
>> if (info.ustate_ac < -1)
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
>> index d37c13b7..1d71bf09 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -272,23 +272,26 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
>> * P-States
>> *****************************************************************************/
>> static int
>> -nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>> +nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
>> {
>> struct nvkm_subdev *subdev = &clk->subdev;
>> struct nvkm_fb *fb = subdev->device->fb;
>> struct nvkm_pci *pci = subdev->device->pci;
>> struct nvkm_pstate *pstate;
>> - int ret, idx = 0;
>> + int ret;
>>
>> - if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
>> + if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
>> return 0;
>>
>> list_for_each_entry(pstate, &clk->states, head) {
>> - if (idx++ == pstatei)
>> + if (pstate->pstate == pstateid)
>> break;
>> }
>>
>> - nvkm_debug(subdev, "setting performance state %d\n", pstatei);
>> + if (!pstate)
>> + return -EINVAL;
>> +
>> + nvkm_debug(subdev, "setting performance state %x\n", pstateid);
>> clk->pstate = pstate;
>>
>> nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>> @@ -329,7 +332,6 @@ nvkm_clk_update_work(struct work_struct *work)
>> pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>> if (clk->state_nr && pstate != -1) {
>> pstate = (pstate < 0) ? clk->astate : pstate;
>> - pstate = min(pstate, clk->state_nr - 1);
>> } else {
>> pstate = NVKM_CLK_PSTATE_DEFAULT;
>> }
>> @@ -490,33 +492,10 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
>> /******************************************************************************
>> * Adjustment triggers
>> *****************************************************************************/
>> -static int
>> -nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
>> -{
>> - struct nvkm_pstate *pstate;
>> - int i = 0;
>> -
>> - if (!clk->allow_reclock)
>> - return -ENOSYS;
>> -
>> - if (req != -1 && req != -2) {
>> - list_for_each_entry(pstate, &clk->states, head) {
>> - if (pstate->pstate == req)
>> - break;
>> - i++;
>> - }
>> -
>> - if (pstate->pstate != req)
>> - return -EINVAL;
>> - req = i;
>> - }
>> -
>> - return req + 2;
>> -}
>> -
>> static int
>> nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>> {
>> + struct nvkm_pstate *pstate;
>> int ret = 1;
>>
>> if (clk->allow_reclock && !strncasecmpz(mode, "auto", arglen))
>> @@ -528,27 +507,46 @@ nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>>
>> ((char *)mode)[arglen] = '\0';
>> if (!kstrtol(mode, 0, &v)) {
>> - ret = nvkm_clk_ustate_update(clk, v);
>> + ret = v;
>> if (ret < 0)
>> ret = 1;
>> }
>> ((char *)mode)[arglen] = save;
>> }
>>
>> - return ret - 2;
>> + if (ret < 0)
>> + return ret;
>
> Don’t you need to check for `clk->allow_reclock`, as it was done in
> `nvkm_clk_ustate_update()`?
>
well ret ended up being 1 due to that check inside nvkm_clk_ustate_update.
>> +
>> + list_for_each_entry(pstate, &clk->states, head) {
>> + if (pstate->pstate == ret)
>> + return ret;
>> + }
>> + return -EINVAL;
>> }
>>
>> int
>> nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>> {
>> - int ret = nvkm_clk_ustate_update(clk, req);
>> - if (ret >= 0) {
>> - if (ret -= 2, pwr) clk->ustate_ac = ret;
>> - else clk->ustate_dc = ret;
>> - clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>> - return nvkm_clk_update(clk, true);
>> + struct nvkm_pstate *pstate;
>> + bool valid = false;
>
> Same here, don’t you need to check for `clk->allow_reclock`, as it was done in
> `nvkm_clk_ustate_update()`?
>
>> +
>> + list_for_each_entry(pstate, &clk->states, head) {
>> + if (pstate->pstate == req) {
>> + valid = true;
>> + break;
>> + }
>> }
>> - return ret;
>> +
>> + if (!valid)
>> + return -EINVAL;
>> +
>> + if (pwr)
>> + clk->ustate_ac = req;
>> + else
>> + clk->ustate_dc = req;
>> +
>> + clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>> + return nvkm_clk_update(clk, true);
>> }
>>
>> int
>> @@ -627,7 +625,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>> if (clk->func->init)
>> return clk->func->init(clk);
>>
>> - clk->astate = clk->state_nr - 1;
>> + clk->astate = NVKM_CLK_PSTATE_DEFAULT;
>> clk->pstate = NULL;
>> clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
>> clk->cstate = NULL;
>> --
>> 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