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. A few more comments below > v2: reword commit message > > Signed-off-by: Karol Herbst <karolher...@gmail.com> > Reviewed-by: Martin Peres <martin.pe...@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()`? > + > + 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@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