Re: [PATCH v4 22/37] clk: rename nvkm_pstate_calc to nvkm_clk_update

[Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>]

On 18/04/16 22:13, Karol Herbst wrote:
> this function will be used to update the current clock state.
>
> This will happen for various reasons:
>   * temperature changes (may change cstate and/or voltage)
>   * user changes boost mode
>   * load changes
>
> v2: add wait parameter
>
> Signed-off-by: Karol Herbst <nouveau@karolherbst.de>
> ---
>   drm/nouveau/include/nvkm/subdev/clk.h |  1 +
>   drm/nouveau/nvkm/subdev/clk/base.c    | 46 ++++++++++++++++++++---------------
>   2 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index 61d99fd..77d94c1 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -120,6 +120,7 @@ int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
>   int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
>   int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
>   int nvkm_clk_tstate(struct nvkm_clk *, int req, int rel);
> +int nvkm_clk_update(struct nvkm_clk *, bool wait);
>   
>   int nv04_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
>   int nv40_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 1ca25dd..bfc6a49 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -274,11 +274,14 @@ static int
>   nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>   {
>   	struct nvkm_subdev *subdev = &clk->subdev;
> -	struct nvkm_ram *ram = subdev->device->fb->ram;
> +	struct nvkm_fb *fb = subdev->device->fb;
>   	struct nvkm_pci *pci = subdev->device->pci;
>   	struct nvkm_pstate *pstate;
>   	int ret, idx = 0;
>   
> +	if (pstatei == -1)
> +		return 0;
> +
>   	list_for_each_entry(pstate, &clk->states, head) {
>   		if (idx++ == pstatei)
>   			break;
> @@ -289,7 +292,8 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>   
>   	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>   
> -	if (ram && ram->func->calc) {
> +	if (fb && fb->ram && fb->ram->func->calc) {
> +		struct nvkm_ram *ram = fb->ram;
>   		int khz = pstate->base.domain[nv_clk_src_mem];
>   		do {
>   			ret = ram->func->calc(ram, khz);

All the changes to this file do not belong to this patch. What are you 
doing here anyway?
> @@ -303,11 +307,11 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>   }
>   
>   static void
> -nvkm_pstate_work(struct work_struct *work)
> +nvkm_clk_update_work(struct work_struct *work)
>   {
>   	struct nvkm_clk *clk = container_of(work, typeof(*clk), work);
>   	struct nvkm_subdev *subdev = &clk->subdev;
> -	int pstate;
> +	int pstate, ret;
>   
>   	if (!atomic_xchg(&clk->waiting, 0))
>   		return;
> @@ -327,21 +331,25 @@ nvkm_pstate_work(struct work_struct *work)
>   	}
>   
>   	nvkm_trace(subdev, "-> %d\n", pstate);
> -	if (pstate != clk->pstate) {
> -		int ret = nvkm_pstate_prog(clk, pstate);
> -		if (ret) {
> -			nvkm_error(subdev, "error setting pstate %d: %d\n",
> -				   pstate, ret);
> -		}
> +	ret = nvkm_pstate_prog(clk, pstate);
> +	if (ret) {
> +		nvkm_error(subdev, "error setting pstate %d: %d\n",
> +			   pstate, ret);
>   	}
Why did you get rid of the if?

>   
>   	wake_up_all(&clk->wait);
>   	nvkm_notify_get(&clk->pwrsrc_ntfy);
>   }
>   
> -static int
> -nvkm_pstate_calc(struct nvkm_clk *clk, bool wait)
> +int
> +nvkm_clk_update(struct nvkm_clk *clk, bool wait)
>   {
> +	if (!clk)
> +		return -EINVAL;
> +
> +	if (!clk->allow_reclock)
> +		return -ENODEV;

This is also not part of the rename. Why do you meed this suddenly?
> +
>   	atomic_set(&clk->waiting, 1);
>   	schedule_work(&clk->work);
>   	if (wait)
> @@ -531,7 +539,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>   	if (ret >= 0) {
>   		if (ret -= 2, pwr) clk->ustate_ac = ret;
>   		else		   clk->ustate_dc = ret;
> -		return nvkm_pstate_calc(clk, true);
> +		return nvkm_clk_update(clk, true);
>   	}
>   	return ret;
>   }
> @@ -543,7 +551,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
>   	if ( rel) clk->astate += rel;
>   	clk->astate = min(clk->astate, clk->state_nr - 1);
>   	clk->astate = max(clk->astate, 0);
> -	return nvkm_pstate_calc(clk, wait);
> +	return nvkm_clk_update(clk, wait);
>   }
>   
>   int
> @@ -553,7 +561,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, int req, int rel)
>   	if ( rel) clk->tstate += rel;
>   	clk->tstate = min(clk->tstate, 0);
>   	clk->tstate = max(clk->tstate, -(clk->state_nr - 1));
> -	return nvkm_pstate_calc(clk, true);
> +	return nvkm_clk_update(clk, true);
>   }
>   
>   int
> @@ -563,7 +571,7 @@ nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
>   	if ( rel) clk->dstate += rel;
>   	clk->dstate = min(clk->dstate, clk->state_nr - 1);
>   	clk->dstate = max(clk->dstate, 0);
> -	return nvkm_pstate_calc(clk, true);
> +	return nvkm_clk_update(clk, true);
>   }
>   
>   static int
> @@ -571,7 +579,7 @@ nvkm_clk_pwrsrc(struct nvkm_notify *notify)
>   {
>   	struct nvkm_clk *clk =
>   		container_of(notify, typeof(*clk), pwrsrc_ntfy);
> -	nvkm_pstate_calc(clk, false);
> +	nvkm_clk_update(clk, false);
>   	return NVKM_NOTIFY_DROP;
>   }
>   
> @@ -626,7 +634,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>   	clk->tstate = 0;
>   	clk->dstate = 0;
>   	clk->pstate = -1;
> -	nvkm_pstate_calc(clk, true);
> +	nvkm_clk_update(clk, true);
>   	return 0;
>   }
>   
> @@ -685,7 +693,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>   	clk->ustate_dc = -1;
>   	clk->allow_reclock = allow_reclock;
>   
> -	INIT_WORK(&clk->work, nvkm_pstate_work);
> +	INIT_WORK(&clk->work, nvkm_clk_update_work);
>   	init_waitqueue_head(&clk->wait);
>   	atomic_set(&clk->waiting, 0);
>   

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau