subject:"\[Nouveau\] \[RFC PATCH 06\/29\] clk\: Make pstate a pointer to nvkm

Re: [Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

2017-10-08 Thread Karol Herbst

On Sun, Oct 8, 2017 at 1:47 PM, Pierre Moreau  wrote:
> The patch seems fine but I found it super confusing that sometimes `pstate` is
> a pointer (for example `clk->pstate`), sometimes it is an int (for example
> `args->v0.pstate`).
>

yeah I still plan to clean the names up when I find some time to
finish those patches for real

> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> We will access the current cstate at least every second and this saves us
>> some CPU cycles looking them up every second.
>>
>> v2: Rewording commit message.
>>
>> Signed-off-by: Karol Herbst 
>> Reviewed-by: Martin Peres 
>> ---
>>  drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
>>  drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
>>  drm/nouveau/nvkm/subdev/clk/base.c| 17 -
>>  drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++---
>>  4 files changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h 
>> b/drm/nouveau/include/nvkm/subdev/clk.h
>> index 1340f5b8..ec537e08 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -10,6 +10,8 @@ struct nvkm_pll_vals;
>>  #define NVKM_CLK_CSTATE_BASE-2 /* pstate base */
>>  #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
>>
>> +#define NVKM_CLK_PSTATE_DEFAULT -1
>> +
>>  enum nv_clk_src {
>>   nv_clk_src_crystal,
>>   nv_clk_src_href,
>> @@ -95,7 +97,7 @@ struct nvkm_clk {
>>
>>   struct nvkm_notify pwrsrc_ntfy;
>>   int pwrsrc;
>> - int pstate; /* current */
>> + struct nvkm_pstate *pstate; /* current */
>>   int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>>   int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>>   int astate; /* perfmon adjustment (base) */
>> diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c 
>> b/drm/nouveau/nvkm/engine/device/ctrl.c
>> index b0ece71a..da70626c 100644
>> --- a/drm/nouveau/nvkm/engine/device/ctrl.c
>> +++ b/drm/nouveau/nvkm/engine/device/ctrl.c
>> @@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, 
>> void *data, u32 size)
>>   args->v0.ustate_ac = clk->ustate_ac;
>>   args->v0.ustate_dc = clk->ustate_dc;
>>   args->v0.pwrsrc = clk->pwrsrc;
>> - args->v0.pstate = clk->pstate;
>> + if (clk->pstate)
>> + args->v0.pstate = clk->pstate->pstate;
>> + else
>> + args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
>>   } else {
>>   args->v0.count = 0;
>>   args->v0.ustate_ac = 
>> NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c 
>> b/drm/nouveau/nvkm/subdev/clk/base.c
>> index 07d530ed..0d4d9fdf 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>>   struct nvkm_pstate *pstate;
>>   int ret, idx = 0;
>>
>> + if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
>> + return 0;
>> +
>>   list_for_each_entry(pstate, >states, head) {
>>   if (idx++ == pstatei)
>>   break;
>>   }
>>
>>   nvkm_debug(subdev, "setting performance state %d\n", pstatei);
>> - clk->pstate = pstatei;
>> + clk->pstate = pstate;
>>
>>   nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>>
>> @@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
>>   return;
>>   clk->pwrsrc = power_supply_is_system_supplied();
>>
>> + if (clk->pstate)
>> + pstate = clk->pstate->pstate;
>> + else
>> + pstate = NVKM_CLK_PSTATE_DEFAULT;
>>   nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
>> -clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>> +pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>>  clk->astate, clk->temp);
>>
>>   pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>
> In the if-statement following the previous line, the -1 could be replaced by
> NVKM_CLK_PSTATE_DEFAULT.
>
>> @@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
>>   pstate = (pstate < 0) ? clk->astate : pstate;
>
> Can `pstate` have negative values other than -1?

not yet, but >=0 means a pstate is selected by an id, <0 but a type,
but it might be that we'll never need more than the default one.

>
>>   pstate = min(pstate, clk->state_nr - 1);
>>   } else {
>> - pstate = clk->pstate = -1;
>> + pstate = NVKM_CLK_PSTATE_DEFAULT;
>
> Shouldn’t you also reset `clk->pstate` to NULL? Or maybe it is
> `nvkm_pstate_prog()` which should do it if
> `pstatei == NVKM_CLK_PSTATE_DEFAULT`.
>

no. We can't go back to the default clocks except we run devinit
again.

Re: [Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

2017-10-08 Thread Pierre Moreau

The patch seems fine but I found it super confusing that sometimes `pstate` is
a pointer (for example `clk->pstate`), sometimes it is an int (for example
`args->v0.pstate`).

On 2017-09-15 — 17:11, Karol Herbst wrote:
> We will access the current cstate at least every second and this saves us
> some CPU cycles looking them up every second.
> 
> v2: Rewording commit message.
> 
> Signed-off-by: Karol Herbst 
> Reviewed-by: Martin Peres 
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
>  drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
>  drm/nouveau/nvkm/subdev/clk/base.c| 17 -
>  drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++---
>  4 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h 
> b/drm/nouveau/include/nvkm/subdev/clk.h
> index 1340f5b8..ec537e08 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -10,6 +10,8 @@ struct nvkm_pll_vals;
>  #define NVKM_CLK_CSTATE_BASE-2 /* pstate base */
>  #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
>  
> +#define NVKM_CLK_PSTATE_DEFAULT -1
> +
>  enum nv_clk_src {
>   nv_clk_src_crystal,
>   nv_clk_src_href,
> @@ -95,7 +97,7 @@ struct nvkm_clk {
>  
>   struct nvkm_notify pwrsrc_ntfy;
>   int pwrsrc;
> - int pstate; /* current */
> + struct nvkm_pstate *pstate; /* current */
>   int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>   int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>   int astate; /* perfmon adjustment (base) */
> diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c 
> b/drm/nouveau/nvkm/engine/device/ctrl.c
> index b0ece71a..da70626c 100644
> --- a/drm/nouveau/nvkm/engine/device/ctrl.c
> +++ b/drm/nouveau/nvkm/engine/device/ctrl.c
> @@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, 
> void *data, u32 size)
>   args->v0.ustate_ac = clk->ustate_ac;
>   args->v0.ustate_dc = clk->ustate_dc;
>   args->v0.pwrsrc = clk->pwrsrc;
> - args->v0.pstate = clk->pstate;
> + if (clk->pstate)
> + args->v0.pstate = clk->pstate->pstate;
> + else
> + args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
>   } else {
>   args->v0.count = 0;
>   args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c 
> b/drm/nouveau/nvkm/subdev/clk/base.c
> index 07d530ed..0d4d9fdf 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>   struct nvkm_pstate *pstate;
>   int ret, idx = 0;
>  
> + if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
> + return 0;
> +
>   list_for_each_entry(pstate, >states, head) {
>   if (idx++ == pstatei)
>   break;
>   }
>  
>   nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> - clk->pstate = pstatei;
> + clk->pstate = pstate;
>  
>   nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>  
> @@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
>   return;
>   clk->pwrsrc = power_supply_is_system_supplied();
>  
> + if (clk->pstate)
> + pstate = clk->pstate->pstate;
> + else
> + pstate = NVKM_CLK_PSTATE_DEFAULT;
>   nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
> -clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> +pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>  clk->astate, clk->temp);
>  
>   pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;

In the if-statement following the previous line, the -1 could be replaced by
NVKM_CLK_PSTATE_DEFAULT.

> @@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
>   pstate = (pstate < 0) ? clk->astate : pstate;

Can `pstate` have negative values other than -1?

>   pstate = min(pstate, clk->state_nr - 1);
>   } else {
> - pstate = clk->pstate = -1;
> + pstate = NVKM_CLK_PSTATE_DEFAULT;

Shouldn’t you also reset `clk->pstate` to NULL? Or maybe it is
`nvkm_pstate_prog()` which should do it if
`pstatei == NVKM_CLK_PSTATE_DEFAULT`.

>   }
>  
>   nvkm_trace(subdev, "-> %d\n", pstate);
> - if (pstate != clk->pstate) {
> + if (!clk->pstate || pstate != clk->pstate->pstate) {
>   int ret = nvkm_pstate_prog(clk, pstate);
>   if (ret) {
>   nvkm_error(subdev, "error setting pstate %d: %d\n",
> @@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>   return clk->func->init(clk);
>  
>   clk->astate = clk->state_nr - 1;
> - clk->pstate

[Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

2017-09-15 Thread Karol Herbst

We will access the current cstate at least every second and this saves us
some CPU cycles looking them up every second.

v2: Rewording commit message.

Signed-off-by: Karol Herbst 
Reviewed-by: Martin Peres 
---
 drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
 drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
 drm/nouveau/nvkm/subdev/clk/base.c| 17 -
 drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++---
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h 
b/drm/nouveau/include/nvkm/subdev/clk.h
index 1340f5b8..ec537e08 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -10,6 +10,8 @@ struct nvkm_pll_vals;
 #define NVKM_CLK_CSTATE_BASE-2 /* pstate base */
 #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
 
+#define NVKM_CLK_PSTATE_DEFAULT -1
+
 enum nv_clk_src {
nv_clk_src_crystal,
nv_clk_src_href,
@@ -95,7 +97,7 @@ struct nvkm_clk {
 
struct nvkm_notify pwrsrc_ntfy;
int pwrsrc;
-   int pstate; /* current */
+   struct nvkm_pstate *pstate; /* current */
int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
int astate; /* perfmon adjustment (base) */
diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c 
b/drm/nouveau/nvkm/engine/device/ctrl.c
index b0ece71a..da70626c 100644
--- a/drm/nouveau/nvkm/engine/device/ctrl.c
+++ b/drm/nouveau/nvkm/engine/device/ctrl.c
@@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, 
void *data, u32 size)
args->v0.ustate_ac = clk->ustate_ac;
args->v0.ustate_dc = clk->ustate_dc;
args->v0.pwrsrc = clk->pwrsrc;
-   args->v0.pstate = clk->pstate;
+   if (clk->pstate)
+   args->v0.pstate = clk->pstate->pstate;
+   else
+   args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
} else {
args->v0.count = 0;
args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c 
b/drm/nouveau/nvkm/subdev/clk/base.c
index 07d530ed..0d4d9fdf 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
struct nvkm_pstate *pstate;
int ret, idx = 0;
 
+   if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
+   return 0;
+
list_for_each_entry(pstate, >states, head) {
if (idx++ == pstatei)
break;
}
 
nvkm_debug(subdev, "setting performance state %d\n", pstatei);
-   clk->pstate = pstatei;
+   clk->pstate = pstate;
 
nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
 
@@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
return;
clk->pwrsrc = power_supply_is_system_supplied();
 
+   if (clk->pstate)
+   pstate = clk->pstate->pstate;
+   else
+   pstate = NVKM_CLK_PSTATE_DEFAULT;
nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
-  clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
+  pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
   clk->astate, clk->temp);
 
pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
@@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
pstate = (pstate < 0) ? clk->astate : pstate;
pstate = min(pstate, clk->state_nr - 1);
} else {
-   pstate = clk->pstate = -1;
+   pstate = NVKM_CLK_PSTATE_DEFAULT;
}
 
nvkm_trace(subdev, "-> %d\n", pstate);
-   if (pstate != clk->pstate) {
+   if (!clk->pstate || pstate != clk->pstate->pstate) {
int ret = nvkm_pstate_prog(clk, pstate);
if (ret) {
nvkm_error(subdev, "error setting pstate %d: %d\n",
@@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
return clk->func->init(clk);
 
clk->astate = clk->state_nr - 1;
-   clk->pstate = -1;
+   clk->pstate = NULL;
clk->temp = 90; /* reasonable default value */
nvkm_clk_update(clk, true);
return 0;
diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c 
b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
index 05e81855..de579726 100644
--- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
@@ -55,24 +55,22 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
return nvkm_clk_astate(clk, *state, 0, false);
 }
 
-static void
-gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
-{
-   struct nvkm_clk *clk =

Re: [Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

Re: [Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

[Nouveau] [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

3 matches

Site Navigation

Mail list logo

Footer information