Re: [Nouveau] [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list
On Sun, Oct 8, 2017 at 4:23 PM, Pierre Moreau 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
>> Reviewed-by: Martin Peres
>> ---
>> 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)
>>
>>
Re: [Nouveau] [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list
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
> Reviewed-by: Martin Peres
> ---
> 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)
> +
[Nouveau] [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list
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.
v2: reword commit message
Signed-off-by: Karol Herbst
Reviewed-by: Martin Peres
---
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;
+
+ 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;
-
