From: Lukasz Luba <lukasz.luba@arm.com>
To: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
dri-devel@lists.freedesktop.org, amit.kucheria@verdurent.com,
airlied@linux.ie, daniel.lezcano@linaro.org,
steven.price@arm.com, alyssa.rosenzweig@collabora.com,
rui.zhang@intel.com, orjan.eide@arm.com
Subject: Re: [PATCH 4/5] thermal: devfreq_cooling: remove old power model and use EM
Date: Thu, 22 Oct 2020 12:26:40 +0100 [thread overview]
Message-ID: <4929dcfa-9ab0-52fa-8426-50cda52eb8dc@arm.com> (raw)
In-Reply-To: <20201007151225.GB15063@arm.com>
On 10/7/20 4:12 PM, Ionela Voinescu wrote:
> Hi Lukasz,
>
> On Monday 21 Sep 2020 at 13:20:06 (+0100), Lukasz Luba wrote:
> [..]
>> /**
>> - * freq_get_state() - get the cooling state corresponding to a frequency
>> + * freq_get_state() - get the performance index corresponding to a frequency
>
> If we change the meaning of the return value, I think the function needs
> a name change as well.
>
> Also, we do treat this as a cooling state when we do validation and
> compare it to THERMAL_CSTATE_INVALID, but it's not actually a cooling
> state (it's max_state - state). It does create confusion if we name
> "state" both a performance index and a cooling state.
>
> Given that the only user is devfreq_cooling_get_requested_power(),
> might be good to collapse freq_get_state() in that function and rename
> the "state" variable in there to "em_perf_idx".
I will have a look into this.
>
>> * @dfc: Pointer to devfreq cooling device
>> - * @freq: frequency in Hz
>> + * @freq: frequency in kHz
>> *
>> - * Return: the cooling state associated with the @freq, or
>> + * Return: the performance index associated with the @freq, or
>> * THERMAL_CSTATE_INVALID if it wasn't found.
>> */
>> static unsigned long
>> @@ -128,8 +130,8 @@ freq_get_state(struct devfreq_cooling_device *dfc, unsigned long freq)
>> {
>> int i;
>>
>> - for (i = 0; i < dfc->freq_table_size; i++) {
>> - if (dfc->freq_table[i] == freq)
>> + for (i = 0; i <= dfc->max_state; i++) {
>> + if (dfc->em->table[i].frequency == freq)
>> return i;
>> }
>>
>> @@ -164,71 +166,15 @@ static unsigned long get_voltage(struct devfreq *df, unsigned long freq)
>> return voltage;
>> }
>>
>> -/**
>> - * get_static_power() - calculate the static power
>> - * @dfc: Pointer to devfreq cooling device
>> - * @freq: Frequency in Hz
>> - *
>> - * Calculate the static power in milliwatts using the supplied
>> - * get_static_power(). The current voltage is calculated using the
>> - * OPP library. If no get_static_power() was supplied, assume the
>> - * static power is negligible.
>> - */
>> -static unsigned long
>> -get_static_power(struct devfreq_cooling_device *dfc, unsigned long freq)
>> +static void dfc_em_get_requested_power(struct em_perf_domain *em,
>> + struct devfreq_dev_status *status,
>> + u32 *power, int em_perf_idx)
>
> Is there a reason for not directly returning the power value in this
> function? Also, this only does a few arithmetic operations and it's only
> called in one place. Is it worth to have this in a separate function?
Good question, maybe I will just put this code where it's called.
>
> [..]
>> @@ -345,11 +279,8 @@ static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
>> struct devfreq_cooling_device *dfc = cdev->devdata;
>> struct devfreq *df = dfc->devfreq;
>> struct devfreq_dev_status status;
>> - unsigned long busy_time;
>> + u32 est_power = power;
>
> Nit: You could use power directly and remove est_power as well.
>
>> unsigned long freq;
>> - s32 dyn_power;
>> - u32 static_power;
>> - s32 est_power;
>> int i;
>>
>> mutex_lock(&df->lock);
>> @@ -358,31 +289,26 @@ static int devfreq_cooling_power2state(struct thermal_cooling_device *cdev,
>>
>> freq = status.current_frequency;
>>
>> - if (dfc->power_ops->get_real_power) {
>> + if (dfc->power_ops && dfc->power_ops->get_real_power) {
>> /* Scale for resource utilization */
>> est_power = power * dfc->res_util;
>> est_power /= SCALE_ERROR_MITIGATION;
>> } else {
>> - static_power = get_static_power(dfc, freq);
>> -
>> - dyn_power = power - static_power;
>> - dyn_power = dyn_power > 0 ? dyn_power : 0;
>> -
>> - /* Scale dynamic power for utilization */
>> - busy_time = status.busy_time ?: 1;
>> - est_power = (dyn_power * status.total_time) / busy_time;
>> + _normalize_load(&status);
>> + est_power *= status.total_time;
>> + est_power /= status.busy_time;
>> }
>>
>> /*
>> * Find the first cooling state that is within the power
>> - * budget for dynamic power.
>> + * budget. The EM power table is sorted ascending.
>> */
>> - for (i = 0; i < dfc->freq_table_size - 1; i++)
>> - if (est_power >= dfc->power_table[i])
>> + for (i = dfc->max_state; i > 0; i--)
>> + if (est_power >= dfc->em->table[i].power)
>> break;
>>
>> - *state = i;
>> - dfc->capped_state = i;
>> + *state = dfc->max_state - i;
>> + dfc->capped_state = *state;
>> trace_thermal_power_devfreq_limit(cdev, freq, *state, power);
>> return 0;
>> }
> [..]
>> /**
>> @@ -503,7 +381,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>> struct thermal_cooling_device *cdev;
>> struct devfreq_cooling_device *dfc;
>> char dev_name[THERMAL_NAME_LENGTH];
>> - int err;
>> + int err, num_opps;
>>
>> dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
>> if (!dfc)
>> @@ -511,28 +389,45 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>>
>> dfc->devfreq = df;
>>
>> - if (dfc_power) {
>> - dfc->power_ops = dfc_power;
>> -
>> + dfc->em = em_pd_get(df->dev.parent);
>> + if (dfc->em) {
>> devfreq_cooling_ops.get_requested_power =
>> devfreq_cooling_get_requested_power;
>> devfreq_cooling_ops.state2power = devfreq_cooling_state2power;
>> devfreq_cooling_ops.power2state = devfreq_cooling_power2state;
>> +
>> + dfc->power_ops = dfc_power;
>> +
>> + num_opps = em_pd_nr_perf_states(dfc->em);
>> + } else {
>> + /* Backward compatibility for drivers which do not use IPA */
>> + dev_dbg(df->dev.parent, "missing EM for cooling device\n");
>> +
>> + num_opps = dev_pm_opp_get_opp_count(df->dev.parent);
>> +
>> + err = devfreq_cooling_gen_tables(dfc, num_opps);
>> + if (err)
>> + goto free_dfc;
>> }
>>
>> - err = devfreq_cooling_gen_tables(dfc);
>> - if (err)
>> + if (num_opps <= 0) {
>> + err = -EINVAL;
>> goto free_dfc;
>> + }
>> +
>> + /* max_state is an index, not a counter */
>
> Nit: Might be more clear to replace "index" with cooling state. Then
> knowledge about cooling states would make this more clear.
Similar comment is in cpufreq_cooling.c. The 'index' here means the last
valid index in the array.
Thank you for the review comments for all patches.
Regards,
Lukasz
>
> Regards,
> Ionela.
>
next prev parent reply other threads:[~2020-10-22 11:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-21 12:20 [PATCH 0/5] Thermal devfreq cooling improvements with Energy Model Lukasz Luba
2020-09-21 12:20 ` [PATCH 1/5] thermal: devfreq_cooling: change tracing function and arguments Lukasz Luba
2020-09-21 12:20 ` [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status Lukasz Luba
2020-10-07 16:11 ` Ionela Voinescu
2020-10-22 10:55 ` Lukasz Luba
2020-12-01 10:36 ` Ionela Voinescu
2020-12-01 12:19 ` Lukasz Luba
2020-12-01 14:55 ` Ionela Voinescu
2020-10-14 14:34 ` Daniel Lezcano
2020-10-22 11:45 ` Lukasz Luba
2020-09-21 12:20 ` [PATCH 3/5] thermal: devfreq_cooling: add new registration functions with Energy Model Lukasz Luba
2020-10-07 12:07 ` Ionela Voinescu
2020-10-22 11:17 ` Lukasz Luba
2020-12-01 14:05 ` Ionela Voinescu
2020-12-01 14:37 ` Lukasz Luba
2020-12-01 15:02 ` Ionela Voinescu
2020-09-21 12:20 ` [PATCH 4/5] thermal: devfreq_cooling: remove old power model and use EM Lukasz Luba
2020-10-07 15:12 ` Ionela Voinescu
2020-10-22 11:26 ` Lukasz Luba [this message]
2020-09-21 12:20 ` [PATCH 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model Lukasz Luba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4929dcfa-9ab0-52fa-8426-50cda52eb8dc@arm.com \
--to=lukasz.luba@arm.com \
--cc=airlied@linux.ie \
--cc=alyssa.rosenzweig@collabora.com \
--cc=amit.kucheria@verdurent.com \
--cc=daniel.lezcano@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=ionela.voinescu@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=orjan.eide@arm.com \
--cc=rui.zhang@intel.com \
--cc=steven.price@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).