Re: [Nouveau] [PATCH v4 27/37] clk: make pstate a pointer to nvkm_pstate

[Martin Peres]

On 21/04/16 00:46, karol herbst wrote:

2016-04-20 20:53 GMT+00:00 Martin Peres :

On 18/04/16 22:14, Karol Herbst wrote:

we will access the current set cstate at least every second and this safes
us

saves

some CPU cycles looking them up every second.

Signed-off-by: Karol Herbst 
---
   drm/nouveau/include/nvkm/subdev/clk.h |  2 +-
   drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
   drm/nouveau/nvkm/subdev/clk/base.c| 12 
   drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 23 +++
   4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
b/drm/nouveau/include/nvkm/subdev/clk.h
index db52e65..4fb2c1b 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -91,7 +91,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 039e8a4..cb85266 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 = -1;
 } 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 3867ab7..762dfe2 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -288,7 +288,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
 }
 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);
   @@ -317,15 +317,19 @@ 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 = -1;
 nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d\n",
-  clk->pstate, clk->pwrsrc, clk->ustate_ac,
clk->ustate_dc,
+  pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
clk->astate);
 pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
 if (clk->state_nr && pstate != -1) {
 pstate = (pstate < 0) ? clk->astate : pstate;
 } else {
-   pstate = clk->pstate = -1;
+   pstate = -1;


Isn't "clk->pstate = NULL;" missing here? Why did you change this code
otherwise?


clk->pstate will never be set as long as they are no states


I see. Then, the patch is:
Reviewed-by: Martin Peres 



 }
 nvkm_trace(subdev, "-> %d\n", pstate);
@@ -609,7 +613,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;
 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 f996d90..6f0d290 100644
--- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
@@ -57,24 +57,21 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int
*state)
   }
 static int
-gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
-{
-   struct nvkm_clk *clk = pmu->base.subdev.device->clk;
-
-   *state = clk->pstate;
-   return 0;
-}
-
-static int
   gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
 int *state, int load)
   {
 struct gk20a_pmu_dvfs_data *data = pmu->data;
 struct nvkm_clk *clk = pmu->base.subdev.device->clk;
+   struct nvkm_pstate *pstate = clk->pstate;
 int cur_level, level;
   + if (!pstate) {
+   *state = 0;
+   return 1;
+   }
+
 /* For GK20A, the performance level is directly mapped to pstate
*/
-   level = cur_level = clk->pstate;
+   level = cur_level = clk->pstate->pstate;
 if (load > data->p_load_max) {
 level = min(clk->state_nr - 1, 

Re: [Nouveau] [PATCH v4 27/37] clk: make pstate a pointer to nvkm_pstate

[karol herbst]

2016-04-20 20:53 GMT+00:00 Martin Peres :
> On 18/04/16 22:14, Karol Herbst wrote:
>>
>> we will access the current set cstate at least every second and this safes
>> us
>
> saves
>>
>> some CPU cycles looking them up every second.
>>
>> Signed-off-by: Karol Herbst 
>> ---
>>   drm/nouveau/include/nvkm/subdev/clk.h |  2 +-
>>   drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
>>   drm/nouveau/nvkm/subdev/clk/base.c| 12 
>>   drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 23 +++
>>   4 files changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
>> b/drm/nouveau/include/nvkm/subdev/clk.h
>> index db52e65..4fb2c1b 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -91,7 +91,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 039e8a4..cb85266 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 = -1;
>> } 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 3867ab7..762dfe2 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -288,7 +288,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>> }
>> 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);
>>   @@ -317,15 +317,19 @@ 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 = -1;
>> nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d\n",
>> -  clk->pstate, clk->pwrsrc, clk->ustate_ac,
>> clk->ustate_dc,
>> +  pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>>clk->astate);
>> pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>> if (clk->state_nr && pstate != -1) {
>> pstate = (pstate < 0) ? clk->astate : pstate;
>> } else {
>> -   pstate = clk->pstate = -1;
>> +   pstate = -1;
>
>
> Isn't "clk->pstate = NULL;" missing here? Why did you change this code
> otherwise?
>

clk->pstate will never be set as long as they are no states

>> }
>> nvkm_trace(subdev, "-> %d\n", pstate);
>> @@ -609,7 +613,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;
>> 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 f996d90..6f0d290 100644
>> --- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
>> +++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
>> @@ -57,24 +57,21 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int
>> *state)
>>   }
>> static int
>> -gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
>> -{
>> -   struct nvkm_clk *clk = pmu->base.subdev.device->clk;
>> -
>> -   *state = clk->pstate;
>> -   return 0;
>> -}
>> -
>> -static int
>>   gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
>> int *state, int load)
>>   {
>> struct gk20a_pmu_dvfs_data *data = pmu->data;
>> struct nvkm_clk *clk = pmu->base.subdev.device->clk;
>> +   struct nvkm_pstate *pstate = clk->pstate;
>> int cur_level, level;
>>   + if (!pstate) {
>> +   *state = 0;
>> +   return 1;
>> +   }
>> +
>> /* For GK20A, the performance level is directly mapped to 

Re: [Nouveau] [PATCH v4 27/37] clk: make pstate a pointer to nvkm_pstate

[Martin Peres]

On 18/04/16 22:14, Karol Herbst wrote:

we will access the current set cstate at least every second and this safes us

saves

some CPU cycles looking them up every second.

Signed-off-by: Karol Herbst 
---
  drm/nouveau/include/nvkm/subdev/clk.h |  2 +-
  drm/nouveau/nvkm/engine/device/ctrl.c |  5 -
  drm/nouveau/nvkm/subdev/clk/base.c| 12 
  drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 23 +++
  4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h 
b/drm/nouveau/include/nvkm/subdev/clk.h
index db52e65..4fb2c1b 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -91,7 +91,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 039e8a4..cb85266 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 = -1;
} 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 3867ab7..762dfe2 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -288,7 +288,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
}
  
  	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);
  
@@ -317,15 +317,19 @@ 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 = -1;
nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d\n",
-  clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
+  pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
   clk->astate);
  
  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;

if (clk->state_nr && pstate != -1) {
pstate = (pstate < 0) ? clk->astate : pstate;
} else {
-   pstate = clk->pstate = -1;
+   pstate = -1;


Isn't "clk->pstate = NULL;" missing here? Why did you change this code 
otherwise?


}
  
  	nvkm_trace(subdev, "-> %d\n", pstate);

@@ -609,7 +613,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;
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 f996d90..6f0d290 100644
--- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
@@ -57,24 +57,21 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
  }
  
  static int

-gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
-{
-   struct nvkm_clk *clk = pmu->base.subdev.device->clk;
-
-   *state = clk->pstate;
-   return 0;
-}
-
-static int
  gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
int *state, int load)
  {
struct gk20a_pmu_dvfs_data *data = pmu->data;
struct nvkm_clk *clk = pmu->base.subdev.device->clk;
+   struct nvkm_pstate *pstate = clk->pstate;
int cur_level, level;
  
+	if (!pstate) {

+   *state = 0;
+   return 1;
+   }
+
/* For GK20A, the performance level is directly mapped to pstate */
-   level = cur_level = clk->pstate;
+   level = cur_level = clk->pstate->pstate;
  
  	if (load > data->p_load_max) {

level = min(clk->state_nr - 1, level + (clk->state_nr / 3));
@@ -150,12 +147,6 @@ gk20a_pmu_dvfs_work(struct nvkm_alarm *alarm)
nvkm_trace(subdev, "utilization = %d %%, avg_load = %d %%\n",
   utilization, data->avg_load);
  
-	ret = gk20a_pmu_dvfs_get_cur_state(pmu, );

-   if (ret) {
-