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

[Karol Herbst]

On Sun, Oct 8, 2017 at 12:41 PM, Pierre Moreau <pierre.morrow at free.fr> 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 <karolherbst at gmail.com>
>> ---
>>  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->work, nvkm_clk_update_work);
>>       init_waitqueue_head(&clk->wait);
>>       atomic_set(&clk->waiting, 0);
>>
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau