Re: [Nouveau] [RFC PATCH 15/29] bios: add thermal policies table

2017-10-08 Thread Karol Herbst
On Sun, Oct 8, 2017 at 8:40 PM, Pierre Moreau wrote: > Looking good. It would be nice to have some defines/enums for the different > modes. Some comments about t0, t1 and t2 would be nice. I saw you are using t0 > in patch 16, but I have no idea why use t0 rather than t1 or t2. > I think I will r

Re: [Nouveau] [RFC PATCH 16/29] clk: parse thermal policies for throttling thresholds

2017-10-08 Thread Karol Herbst
On Sun, Oct 8, 2017 at 8:36 PM, Pierre Moreau wrote: > On 2017-09-15 — 17:11, Karol Herbst wrote: >> v2: use min_t >> >> Signed-off-by: Karol Herbst >> --- >> drm/nouveau/include/nvkm/subdev/clk.h | 2 ++ >> drm/nouveau/nvkm/subdev/clk/base.c| 42 >> +++ >>

Re: [Nouveau] [RFC PATCH 17/29] clk: thermal throttling

2017-10-08 Thread Pierre Moreau
Reviewed-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > v2: make message about relaxed throttling an info > rework reporting about current clk state > > Signed-off-by: Karol Herbst > --- > drm/nouveau/include/nvkm/subdev/clk.h | 1 + > drm/nouveau/nvkm/subdev/clk/base.c

Re: [Nouveau] [RFC PATCH 15/29] bios: add thermal policies table

2017-10-08 Thread Pierre Moreau
Looking good. It would be nice to have some defines/enums for the different modes. Some comments about t0, t1 and t2 would be nice. I saw you are using t0 in patch 16, but I have no idea why use t0 rather than t1 or t2. Otherwise, Acked-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote

Re: [Nouveau] [RFC PATCH 16/29] clk: parse thermal policies for throttling thresholds

2017-10-08 Thread Pierre Moreau
On 2017-09-15 — 17:11, Karol Herbst wrote: > v2: use min_t > > Signed-off-by: Karol Herbst > --- > drm/nouveau/include/nvkm/subdev/clk.h | 2 ++ > drm/nouveau/nvkm/subdev/clk/base.c| 42 > +++ > 2 files changed, 44 insertions(+) > > diff --git a/drm/nouveau

Re: [Nouveau] [RFC PATCH 14/29] therm: Trigger reclock in temperature daemon

2017-10-08 Thread Pierre Moreau
Reviewed-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > Depending on the temperature, cstates might become unreachable or the maped > voltage of a cstate changes. We want to adjust to that. > > Signed-off-by: Karol Herbst > Reviewed-by: Martin Peres > --- > drm/nouveau/nvkm/su

Re: [Nouveau] [RFC PATCH 09/29] clk: Set clocks to pre suspend state after suspend

2017-10-08 Thread Pierre Moreau
Reviewed-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > The idea is to clear out the saved state, because after a resume we can't > know what the GPU is clocked to. The reclock is triggered by the call to > nvkm_clk_update later in nvkm_clk_init. > > v2: convert to C style commen

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

2017-10-08 Thread Karol Herbst
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 ha

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

2017-10-08 Thread Pierre Moreau
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

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

Re: [Nouveau] [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it

2017-10-08 Thread Karol Herbst
On Sun, Oct 8, 2017 at 12:41 PM, Pierre Moreau wrote: > On 2017-09-15 — 17:11, Karol Herbst wrote: >> This function will be used to update the current clock state. >> >> This will happen for various reasons: >> * Temperature changes >> * User changes clocking state >> * Load changes >> >> v2

Re: [Nouveau] [RFC PATCH 07/29] clk: Hold information about the current cstate status

2017-10-08 Thread Pierre Moreau
Acked-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > Later we will have situations where the expected and the current state > isn't the same. > > Signed-off-by: Karol Herbst > Reviewed-by: Martin Peres > --- > drm/nouveau/include/nvkm/subdev/clk.h | 2 ++ > drm/nouveau/nvkm/s

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 > som

Re: [Nouveau] [RFC PATCH 05/29] clk: Remove dstate

2017-10-08 Thread Pierre Moreau
This patch could be applied before patch 04, that way patch 04 does not need to update a function (`nvkm_clk_dstate()`) that will be removed in the next patch. Reviewed-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > We won't need it now, because we will adjust the clocks dependin

Re: [Nouveau] [RFC PATCH 03/29] subdev/volt/gk104: return error when read fails

2017-10-08 Thread Karol Herbst
On Sun, Oct 8, 2017 at 12:29 PM, Pierre Moreau wrote: > On 2017-09-15 — 17:11, Karol Herbst wrote: >> While my gpu was powered off, hwmon returned 0.6V as the current voltage. >> If nvkm_rd32 fails for any reason, return the error. >> >> With that sensors will display a "N/A" instead of 0.6V. > >

Re: [Nouveau] [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it

2017-10-08 Thread Pierre Moreau
On 2017-09-15 — 17:11, Karol Herbst wrote: > This function will be used to update the current clock state. > > This will happen for various reasons: > * Temperature changes > * User changes clocking state > * Load changes > > v2: remove parameter name > > Signed-off-by: Karol Herbst > ---

Re: [Nouveau] [RFC PATCH 01/29] therm: split return code and value in nvkm_get_temp

2017-10-08 Thread Karol Herbst
On Sun, Oct 8, 2017 at 12:16 PM, Pierre Moreau wrote: > As you changed the return value of `temp_get()` to solely be the error code, > or > absence of an error, I would change all those tests that checked whether the > returned value was strictly less, or greater than, 0 to now only compare > aga

Re: [Nouveau] [RFC PATCH 03/29] subdev/volt/gk104: return error when read fails

2017-10-08 Thread Pierre Moreau
On 2017-09-15 — 17:11, Karol Herbst wrote: > While my gpu was powered off, hwmon returned 0.6V as the current voltage. > If nvkm_rd32 fails for any reason, return the error. > > With that sensors will display a "N/A" instead of 0.6V. Small nitpick, add a comma between “that” and “sensors”. Otherw

Re: [Nouveau] [RFC PATCH 02/29] hwmon: properly check for errors

2017-10-08 Thread Pierre Moreau
Reviewed-by: Pierre Moreau On 2017-09-15 — 17:11, Karol Herbst wrote: > Otherwise hwmon interprets error codes as real values. > > Signed-off-by: Karol Herbst > --- > drm/nouveau/nouveau_hwmon.c | 33 ++--- > 1 file changed, 26 insertions(+), 7 deletions(-) > > dif

Re: [Nouveau] [RFC PATCH 01/29] therm: split return code and value in nvkm_get_temp

2017-10-08 Thread Pierre Moreau
As you changed the return value of `temp_get()` to solely be the error code, or absence of an error, I would change all those tests that checked whether the returned value was strictly less, or greater than, 0 to now only compare against 0 (no error). For example, if (therm && therm->attr_get &&