lkml.org 
[lkml]   [2018]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
On 2018-08-02 02:56, MyungJoo Ham wrote:
>> Many CPU architectures have caches that can scale independent of the
>> CPUs.
>> Frequency scaling of the caches is necessary to make sure the cache is
>> not
>> a performance bottleneck that leads to poor performance and power. The
>> same
>> idea applies for RAM/DDR.
>>
>> To achieve this, this patch adds a generic devfreq governor that takes
>> the
>> current frequency of each CPU frequency domain and then adjusts the
>> frequency of the cache (or any devfreq device) based on the frequency
>> of
>> the CPUs. It listens to CPU frequency transition notifiers to keep
>> itself
>> up to date on the current CPU frequency.
>>
>> To decide the frequency of the device, the governor does one of the
>> following:
>
> This exactly has the same purpose with "passive" governor except for
> the
> single part: passive governor depends on another devfreq driver, not
> cpufreq driver.
>
> If both governors have many features in common, can you merge them into
> one?
> Passive governor also has "get_target_freq", which allows driver
> authors
> to define the mapping.

Thanks for the review and pointing me to the passive governor. I agree
that at a high level they are both doing the same. I can certainly stuff
this CPU freq to dev freq mapping into passive governor, but I think
it'll just make one huge set of code that's harder to understand and
maintain because it trying to do different things under the hood.

There are also a bunch of complexities and optimizations that come with
the cpufreq-map governor that don't fit with the passive governor.

1. It's not one CPU who's frequency we have to listen to. There are
multiple CPUs/policies we have to aggregate across.
2. We have to deal with big vs little having different needs/mappings.
3. Since it's always just CPUfreq, I can optimize the handling in the
transition notifiers. If I have 4 different devices that are scaled
based on CPU freq, I still use only 1 transition notifier. It becomes a
bit harder to do with the passive governor.
4. It requires that the device explicitly support the passive governor
and pick a mapping function. With cpufreq-map governor, the device
drivers don't need to make any changes. Whoever is making a device/board
can choose what devices to scale up base on CPU freq based on their
board and their needs. Even an end user can say, scale the GPU based on
my CPU based on interpolation if they choose to.
5. If a device has some other use for the private data, it can't work
with passive governor, but can work with cpufreq-map governor.
6. I also want to improve cpufreq-map governor to handle hotplug
correctly in later patches (needs more discussion) and that'll add more
complexity.

I think for these reasons we shouldn't combine these two governors. Let
me know what you think.

> Probably, you will need to add one more notifier call to get events
> from
> cpufreq instead of devfreq.
>
>>
>> * Uses a CPU frequency to device frequency mapping table
>> - Either one mapping table used for all CPU freq policies (typically
>> used
>> for system with homogeneous cores/clusters that have the same
>> OPPs).
>> - One mapping table per CPU freq policy (typically used for ASMP
>> systems
>> with heterogeneous CPUs with different OPPs)
>>
>> OR
>>
>> * Scales the device frequency in proportion to the CPU frequency. So,
>> if
>> the CPUs are running at their max frequency, the device runs at its
>> max
>> frequency. If the CPUs are running at their min frequency, the
>> device
>> runs at its min frequency. And interpolated for frequencies in
>> between.
>
> If you want to satisfy these two cases to the "modified" passive
> governors,
> you may need to supply two "preloaded" get_target_freq functions for
> struct devfreq_passive_data. If that's impossible, requiring a bit more
> modifications, you may need to write alternative routines in
> update_devfreq_passive().
>

Thanks for the pointers. I understood what you mean here.

>> diff --git a/drivers/devfreq/governor_cpufreq_map.c
>> b/drivers/devfreq/governor_cpufreq_map.c
>> new file mode 100644
>> index 0000000..084a3ff
>> --- /dev/null
>> +++ b/drivers/devfreq/governor_cpufreq_map.c
>> @@ -0,0 +1,583 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights
>> reserved.
>> + */
>
> Is this boilerplate fine? ( using // )

I was just following what I thought was the new standard. checkpatch
even complains about not having this.

Thanks,
Saravana

\
 
 \ /
  Last update: 2018-08-02 23:00    [W:0.136 / U:2.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site