The Power values coming from the Energy Model are already in uW.
The PowerCap and DTPM framework operate on uW, thus all places should
just use the values from EM. Fix the code which left and still does
the unneeded conversion.
Fixes: ae6ccaa65038 (PM: EM: convert power field to micro-Watts precision and align drivers)
Cc: <[email protected]> # v5.19+
Signed-off-by: Lukasz Luba <[email protected]>
---
Hi Daniel,
I have found an issue due to the uW in the EM. My apologies for that.
I have check those with the Rockpi dev board with your DTPM module there.
BTW, if you like to check the DTPM_devfreq there, you can apply that
patch. It should create EM for your GPU there and setup DTPM GPU:
https://lore.kernel.org/all/[email protected]/
Regards,
Lukasz
drivers/powercap/dtpm_cpu.c | 6 +-----
drivers/powercap/dtpm_devfreq.c | 11 +++--------
2 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 2ff7717530bf..8a2f18fa3faf 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -24,7 +24,6 @@
#include <linux/of.h>
#include <linux/pm_qos.h>
#include <linux/slab.h>
-#include <linux/units.h>
struct dtpm_cpu {
struct dtpm dtpm;
@@ -104,8 +103,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
if (pd->table[i].frequency < freq)
continue;
- return scale_pd_power_uw(pd_mask, pd->table[i].power *
- MICROWATT_PER_MILLIWATT);
+ return scale_pd_power_uw(pd_mask, pd->table[i].power);
}
return 0;
@@ -122,11 +120,9 @@ static int update_pd_power_uw(struct dtpm *dtpm)
nr_cpus = cpumask_weight(&cpus);
dtpm->power_min = em->table[0].power;
- dtpm->power_min *= MICROWATT_PER_MILLIWATT;
dtpm->power_min *= nr_cpus;
dtpm->power_max = em->table[em->nr_perf_states - 1].power;
- dtpm->power_max *= MICROWATT_PER_MILLIWATT;
dtpm->power_max *= nr_cpus;
return 0;
diff --git a/drivers/powercap/dtpm_devfreq.c b/drivers/powercap/dtpm_devfreq.c
index 91276761a31d..612c3b59dd5b 100644
--- a/drivers/powercap/dtpm_devfreq.c
+++ b/drivers/powercap/dtpm_devfreq.c
@@ -39,10 +39,8 @@ static int update_pd_power_uw(struct dtpm *dtpm)
struct em_perf_domain *pd = em_pd_get(dev);
dtpm->power_min = pd->table[0].power;
- dtpm->power_min *= MICROWATT_PER_MILLIWATT;
dtpm->power_max = pd->table[pd->nr_perf_states - 1].power;
- dtpm->power_max *= MICROWATT_PER_MILLIWATT;
return 0;
}
@@ -54,13 +52,10 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
struct device *dev = devfreq->dev.parent;
struct em_perf_domain *pd = em_pd_get(dev);
unsigned long freq;
- u64 power;
int i;
for (i = 0; i < pd->nr_perf_states; i++) {
-
- power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
- if (power > power_limit)
+ if (pd->table[i].power > power_limit)
break;
}
@@ -68,7 +63,7 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
dev_pm_qos_update_request(&dtpm_devfreq->qos_req, freq);
- power_limit = pd->table[i - 1].power * MICROWATT_PER_MILLIWATT;
+ power_limit = pd->table[i - 1].power;
return power_limit;
}
@@ -110,7 +105,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
if (pd->table[i].frequency < freq)
continue;
- power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
+ power = pd->table[i].power;
power *= status.busy_time;
power >>= 10;
--
2.25.1
On Mon, Nov 27, 2023 at 10:27 AM Lukasz Luba <[email protected]> wrote:
>
> The Power values coming from the Energy Model are already in uW.
> The PowerCap and DTPM framework operate on uW, thus all places should
> just use the values from EM. Fix the code which left and still does
> the unneeded conversion.
>
> Fixes: ae6ccaa65038 (PM: EM: convert power field to micro-Watts precision and align drivers)
> Cc: <[email protected]> # v5.19+
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> Hi Daniel,
>
> I have found an issue due to the uW in the EM. My apologies for that.
No need to apologize, you are making the code better.
> I have check those with the Rockpi dev board with your DTPM module there.
> BTW, if you like to check the DTPM_devfreq there, you can apply that
> patch. It should create EM for your GPU there and setup DTPM GPU:
> https://lore.kernel.org/all/[email protected]/
>
> Regards,
> Lukasz
>
>
> drivers/powercap/dtpm_cpu.c | 6 +-----
> drivers/powercap/dtpm_devfreq.c | 11 +++--------
> 2 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 2ff7717530bf..8a2f18fa3faf 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -24,7 +24,6 @@
> #include <linux/of.h>
> #include <linux/pm_qos.h>
> #include <linux/slab.h>
> -#include <linux/units.h>
>
> struct dtpm_cpu {
> struct dtpm dtpm;
> @@ -104,8 +103,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
> if (pd->table[i].frequency < freq)
> continue;
>
> - return scale_pd_power_uw(pd_mask, pd->table[i].power *
> - MICROWATT_PER_MILLIWATT);
> + return scale_pd_power_uw(pd_mask, pd->table[i].power);
> }
>
> return 0;
> @@ -122,11 +120,9 @@ static int update_pd_power_uw(struct dtpm *dtpm)
> nr_cpus = cpumask_weight(&cpus);
>
> dtpm->power_min = em->table[0].power;
> - dtpm->power_min *= MICROWATT_PER_MILLIWATT;
> dtpm->power_min *= nr_cpus;
>
> dtpm->power_max = em->table[em->nr_perf_states - 1].power;
> - dtpm->power_max *= MICROWATT_PER_MILLIWATT;
> dtpm->power_max *= nr_cpus;
>
> return 0;
> diff --git a/drivers/powercap/dtpm_devfreq.c b/drivers/powercap/dtpm_devfreq.c
> index 91276761a31d..612c3b59dd5b 100644
> --- a/drivers/powercap/dtpm_devfreq.c
> +++ b/drivers/powercap/dtpm_devfreq.c
> @@ -39,10 +39,8 @@ static int update_pd_power_uw(struct dtpm *dtpm)
> struct em_perf_domain *pd = em_pd_get(dev);
>
> dtpm->power_min = pd->table[0].power;
> - dtpm->power_min *= MICROWATT_PER_MILLIWATT;
>
> dtpm->power_max = pd->table[pd->nr_perf_states - 1].power;
> - dtpm->power_max *= MICROWATT_PER_MILLIWATT;
>
> return 0;
> }
> @@ -54,13 +52,10 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
> struct device *dev = devfreq->dev.parent;
> struct em_perf_domain *pd = em_pd_get(dev);
> unsigned long freq;
> - u64 power;
> int i;
>
> for (i = 0; i < pd->nr_perf_states; i++) {
> -
> - power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
> - if (power > power_limit)
> + if (pd->table[i].power > power_limit)
> break;
> }
>
> @@ -68,7 +63,7 @@ static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
>
> dev_pm_qos_update_request(&dtpm_devfreq->qos_req, freq);
>
> - power_limit = pd->table[i - 1].power * MICROWATT_PER_MILLIWATT;
> + power_limit = pd->table[i - 1].power;
>
> return power_limit;
> }
> @@ -110,7 +105,7 @@ static u64 get_pd_power_uw(struct dtpm *dtpm)
> if (pd->table[i].frequency < freq)
> continue;
>
> - power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
> + power = pd->table[i].power;
> power *= status.busy_time;
> power >>= 10;
>
> --
Applied as 6.7-rc material, thanks!
On 11/28/23 14:17, Rafael J. Wysocki wrote:
> On Mon, Nov 27, 2023 at 10:27 AM Lukasz Luba <[email protected]> wrote:
>>
>> The Power values coming from the Energy Model are already in uW.
>> The PowerCap and DTPM framework operate on uW, thus all places should
>> just use the values from EM. Fix the code which left and still does
>> the unneeded conversion.
>>
>> Fixes: ae6ccaa65038 (PM: EM: convert power field to micro-Watts precision and align drivers)
>> Cc: <[email protected]> # v5.19+
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> Hi Daniel,
>>
>> I have found an issue due to the uW in the EM. My apologies for that.
>
> No need to apologize, you are making the code better.
I apologize because I introduced this uW change in the EM fwk :)
although I missed those DTPM bits which are now fixed.
Daniel told me last week that I can easily test the DTPM using
the rockpi code (because it was a pain for me in some hacks
trying to test DTPM).
>
>> I have check those with the Rockpi dev board with your DTPM module there.
>> BTW, if you like to check the DTPM_devfreq there, you can apply that
>> patch. It should create EM for your GPU there and setup DTPM GPU:
>> https://lore.kernel.org/all/[email protected]/
>>
[snip]
>
> Applied as 6.7-rc material, thanks!
thanks Rafael!