[Nouveau] [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list

Pierre Moreau
2017-10-08T14:23:23Z

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 <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()`?

> +
> +	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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20171008/198fba4e/attachment.sig>