Re: [Nouveau] [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it
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: remove parameter name
>>
>> Signed-off-by: Karol Herbst
>> ---
>> drm/nouveau/include/nvkm/subdev/clk.h | 1 +
>> drm/nouveau/nvkm/subdev/clk/base.c| 26 --
>> 2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
>> b/drm/nouveau/include/nvkm/subdev/clk.h
>> index e5275f74..ce3bbcfe 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -123,6 +123,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 *, u8 temperature);
>> +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 e4c8d310..ecff3ff3 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -296,7 +296,7 @@ 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;
>> @@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
>> 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;
>> +
>> atomic_set(&clk->waiting, 1);
>> schedule_work(&clk->work);
>> if (wait)
>> @@ -524,7 +530,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;
>> }
>> @@ -536,7 +542,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
>> @@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
>> if (clk->temp == temp)
>> return 0;
>> clk->temp = temp;
>> - return nvkm_pstate_calc(clk, false);
>> + return nvkm_clk_update(clk, false);
>> }
>>
>> int
>> @@ -555,7 +561,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
>> @@ -563,7 +569,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;
>
> Same here as below, shouldn’t there be some error checking on what
> `nvkm_clk_update()` returned, as it can fail?
>
it doesn't matter here, because it shouldn't change the return value
of that function.
>> }
>>
>> @@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>> clk->dstate = 0;
>> clk->pstate = -1;
>> clk->temp = 90; /* reasonable default value */
>> - nvkm_pstate_calc(clk, true);
>> + nvkm_clk_update(clk, true);
>> return 0;
>
> `nvkm_pstate_calc()` did not return any error code, but `nvkm_clk_update()`
> now
> can, so shouldn’t the function return the return value of `nvkm_clk_update()`
> instead? Or at least do some error checking on what `nvkm_clk_update()`
> returned?
I don't want nvkm_clk_init to fail just because we don't allow
reclocking (which is basically the only reason it may return !0 here)
>
>> }
>>
>> @@ -675,7 +681,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->wo
Re: [Nouveau] [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it
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
> ---
> drm/nouveau/include/nvkm/subdev/clk.h | 1 +
> drm/nouveau/nvkm/subdev/clk/base.c| 26 --
> 2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
> b/drm/nouveau/include/nvkm/subdev/clk.h
> index e5275f74..ce3bbcfe 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -123,6 +123,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 *, u8 temperature);
> +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 e4c8d310..ecff3ff3 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -296,7 +296,7 @@ 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;
> @@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
> 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;
> +
> atomic_set(&clk->waiting, 1);
> schedule_work(&clk->work);
> if (wait)
> @@ -524,7 +530,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;
> }
> @@ -536,7 +542,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
> @@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
> if (clk->temp == temp)
> return 0;
> clk->temp = temp;
> - return nvkm_pstate_calc(clk, false);
> + return nvkm_clk_update(clk, false);
> }
>
> int
> @@ -555,7 +561,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
> @@ -563,7 +569,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;
Same here as below, shouldn’t there be some error checking on what
`nvkm_clk_update()` returned, as it can fail?
> }
>
> @@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
> clk->dstate = 0;
> clk->pstate = -1;
> clk->temp = 90; /* reasonable default value */
> - nvkm_pstate_calc(clk, true);
> + nvkm_clk_update(clk, true);
> return 0;
`nvkm_pstate_calc()` did not return any error code, but `nvkm_clk_update()` now
can, so shouldn’t the function return the return value of `nvkm_clk_update()`
instead? Or at least do some error checking on what `nvkm_clk_update()`
returned?
> }
>
> @@ -675,7 +681,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);
>
> --
> 2.14.1
>
> ___
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
signature.asc
Description: PGP signature
___
Nouveau mailing lis
[Nouveau] [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it
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
---
drm/nouveau/include/nvkm/subdev/clk.h | 1 +
drm/nouveau/nvkm/subdev/clk/base.c| 26 --
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drm/nouveau/include/nvkm/subdev/clk.h
b/drm/nouveau/include/nvkm/subdev/clk.h
index e5275f74..ce3bbcfe 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -123,6 +123,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 *, u8 temperature);
+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 e4c8d310..ecff3ff3 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -296,7 +296,7 @@ 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;
@@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
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;
+
atomic_set(&clk->waiting, 1);
schedule_work(&clk->work);
if (wait)
@@ -524,7 +530,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;
}
@@ -536,7 +542,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
@@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
if (clk->temp == temp)
return 0;
clk->temp = temp;
- return nvkm_pstate_calc(clk, false);
+ return nvkm_clk_update(clk, false);
}
int
@@ -555,7 +561,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
@@ -563,7 +569,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;
}
@@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
clk->dstate = 0;
clk->pstate = -1;
clk->temp = 90; /* reasonable default value */
- nvkm_pstate_calc(clk, true);
+ nvkm_clk_update(clk, true);
return 0;
}
@@ -675,7 +681,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);
--
2.14.1
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
