diff mbox series

[V3,2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

Message ID 1543221866-19671-2-git-send-email-daniel.lezcano@linaro.org
State New
Headers show
Series [V3,1/2] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE | expand

Commit Message

Daniel Lezcano Nov. 26, 2018, 8:44 a.m. UTC 
In the case of asymmetric SoC with the same micro-architecture, we
have a group of CPUs with smaller OPPs than the other group. One
example is the 96boards dragonboard 820c. There is no dmips/MHz
difference between both groups, so no need to specify the values in
the DT. Unfortunately, without these defined, there is no scaling
capacity computation triggered, so we need to write
'capacity-dmips-mhz' for each CPU with the same value in order to
force the scaled capacity computation.

In order to fix this situation, allocate 'raw_capacity' so the pointer
is set and the init_cpu_capacity_callback() function can be called.

This was tested on db820c:
 - specified values in the DT (correct results)
 - partial values defined in the DT (error + fallback to defaults)
 - no specified values in the DT (correct results)

correct results are:
  cat /sys/devices/system/cpu/cpu*/cpu_capacity
   758
   758
  1024
  1024

  ... respectively for CPU0, CPU1, CPU2 and CPU3.

That reflects the capacity for the max frequencies 1593600 and 2150400.

Cc: Chris Redpath <chris.redpath@linaro.org>
Cc: Quentin Perret <quentin.perret@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
Cc: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 drivers/base/arch_topology.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Viresh Kumar Nov. 26, 2018, 8:45 a.m. UTC | #1 
On 26-11-18, 09:44, Daniel Lezcano wrote:
> In the case of asymmetric SoC with the same micro-architecture, we

> have a group of CPUs with smaller OPPs than the other group. One

> example is the 96boards dragonboard 820c. There is no dmips/MHz

> difference between both groups, so no need to specify the values in

> the DT. Unfortunately, without these defined, there is no scaling

> capacity computation triggered, so we need to write

> 'capacity-dmips-mhz' for each CPU with the same value in order to

> force the scaled capacity computation.

> 

> In order to fix this situation, allocate 'raw_capacity' so the pointer

> is set and the init_cpu_capacity_callback() function can be called.

> 

> This was tested on db820c:

>  - specified values in the DT (correct results)

>  - partial values defined in the DT (error + fallback to defaults)

>  - no specified values in the DT (correct results)

> 

> correct results are:

>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>    758

>    758

>   1024

>   1024

> 

>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

> 

> That reflects the capacity for the max frequencies 1593600 and 2150400.

> 

> Cc: Chris Redpath <chris.redpath@linaro.org>

> Cc: Quentin Perret <quentin.perret@linaro.org>

> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: Amit Kucheria <amit.kucheria@linaro.org>

> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

> Cc: Niklas Cassel <niklas.cassel@linaro.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>  drivers/base/arch_topology.c | 13 ++++++++++++-

>  1 file changed, 12 insertions(+), 1 deletion(-)


Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>


-- 
viresh
Quentin Perret Nov. 26, 2018, 9:52 a.m. UTC | #2 
On Monday 26 Nov 2018 at 09:44:21 (+0100), Daniel Lezcano wrote:
> In the case of asymmetric SoC with the same micro-architecture, we

> have a group of CPUs with smaller OPPs than the other group. One

> example is the 96boards dragonboard 820c. There is no dmips/MHz

> difference between both groups, so no need to specify the values in

> the DT. Unfortunately, without these defined, there is no scaling

> capacity computation triggered, so we need to write

> 'capacity-dmips-mhz' for each CPU with the same value in order to

> force the scaled capacity computation.

> 

> In order to fix this situation, allocate 'raw_capacity' so the pointer

> is set and the init_cpu_capacity_callback() function can be called.

> 

> This was tested on db820c:

>  - specified values in the DT (correct results)

>  - partial values defined in the DT (error + fallback to defaults)

>  - no specified values in the DT (correct results)

> 

> correct results are:

>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>    758

>    758

>   1024

>   1024

> 

>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

> 

> That reflects the capacity for the max frequencies 1593600 and 2150400.

> 

> Cc: Chris Redpath <chris.redpath@linaro.org>

> Cc: Quentin Perret <quentin.perret@linaro.org>

> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: Amit Kucheria <amit.kucheria@linaro.org>

> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

> Cc: Niklas Cassel <niklas.cassel@linaro.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>  drivers/base/arch_topology.c | 13 ++++++++++++-

>  1 file changed, 12 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

> index fd5325b..e0c5b60 100644

> --- a/drivers/base/arch_topology.c

> +++ b/drivers/base/arch_topology.c

> @@ -243,9 +243,20 @@ static int __init register_cpufreq_notifier(void)

>  	 * until we have the necessary code to parse the cpu capacity, so

>  	 * skip registering cpufreq notifier.

>  	 */

> -	if (!acpi_disabled || !raw_capacity)

> +	if (!acpi_disabled)

>  		return -EINVAL;

>  

> +	if (!raw_capacity) {

> +

> +		pr_info("cpu_capacity: No capacity defined in DT, set default "

> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);

> +

> +		raw_capacity = kmalloc_array(num_possible_cpus(),

> +					     sizeof(*raw_capacity), GFP_KERNEL);

> +		if (!raw_capacity)

> +			return -ENOMEM;

> +	}

> +

>  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {

>  		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");

>  		return -ENOMEM;

> -- 

> 2.7.4


With this, if the DT is partially filled, we will still do the frequency
scaling thing now right ?

I'm not sure if this is the expected behaviour. If the DT is partially
filled, we probably want to have 1024 of capacity for all CPUs to match
the doc.

Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
top of topology_parse_cpu_capacity() ?

Thanks,
Quentin
Daniel Lezcano Nov. 26, 2018, 10:08 a.m. UTC | #3 
On 26/11/2018 10:52, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 09:44:21 (+0100), Daniel Lezcano wrote:

>> In the case of asymmetric SoC with the same micro-architecture, we

>> have a group of CPUs with smaller OPPs than the other group. One

>> example is the 96boards dragonboard 820c. There is no dmips/MHz

>> difference between both groups, so no need to specify the values in

>> the DT. Unfortunately, without these defined, there is no scaling

>> capacity computation triggered, so we need to write

>> 'capacity-dmips-mhz' for each CPU with the same value in order to

>> force the scaled capacity computation.

>>

>> In order to fix this situation, allocate 'raw_capacity' so the pointer

>> is set and the init_cpu_capacity_callback() function can be called.

>>

>> This was tested on db820c:

>>  - specified values in the DT (correct results)

>>  - partial values defined in the DT (error + fallback to defaults)

>>  - no specified values in the DT (correct results)

>>

>> correct results are:

>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity

>>    758

>>    758

>>   1024

>>   1024

>>

>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.

>>

>> That reflects the capacity for the max frequencies 1593600 and 2150400.

>>

>> Cc: Chris Redpath <chris.redpath@linaro.org>

>> Cc: Quentin Perret <quentin.perret@linaro.org>

>> Cc: Viresh Kumar <viresh.kumar@linaro.org>

>> Cc: Amit Kucheria <amit.kucheria@linaro.org>

>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>

>> Cc: Niklas Cassel <niklas.cassel@linaro.org>

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>> ---

>>  drivers/base/arch_topology.c | 13 ++++++++++++-

>>  1 file changed, 12 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c

>> index fd5325b..e0c5b60 100644

>> --- a/drivers/base/arch_topology.c

>> +++ b/drivers/base/arch_topology.c

>> @@ -243,9 +243,20 @@ static int __init register_cpufreq_notifier(void)

>>  	 * until we have the necessary code to parse the cpu capacity, so

>>  	 * skip registering cpufreq notifier.

>>  	 */

>> -	if (!acpi_disabled || !raw_capacity)

>> +	if (!acpi_disabled)

>>  		return -EINVAL;

>>  

>> +	if (!raw_capacity) {

>> +

>> +		pr_info("cpu_capacity: No capacity defined in DT, set default "

>> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);

>> +

>> +		raw_capacity = kmalloc_array(num_possible_cpus(),

>> +					     sizeof(*raw_capacity), GFP_KERNEL);

>> +		if (!raw_capacity)

>> +			return -ENOMEM;

>> +	}

>> +

>>  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {

>>  		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");

>>  		return -ENOMEM;

>> -- 

>> 2.7.4

> 

> With this, if the DT is partially filled, we will still do the frequency

> scaling thing now right ?


Right, if the DT is partially filled. We end up with the error, the
raw_capacity is free and set to NULL.

register_cpufreq_notifier() will allocate it and the capacity is computed.

> I'm not sure if this is the expected behaviour. If the DT is partially

> filled, we probably want to have 1024 of capacity for all CPUs to match

> the doc.


Yes if they have the same number of OPP which is the case of 99% of the
boards (excluding the big Little). Otherwise setting all CPU with a
capacity of 1024 but having different OPP (like qcom gold-silver arch)
does not make sense and the patch fix this.

> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the

> top of topology_parse_cpu_capacity() ?


I prefer to update the documentation, it makes more sense than adding
more cumbersome tests in the current code.



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Viresh Kumar Nov. 26, 2018, 10:19 a.m. UTC | #4 
On 26-11-18, 11:08, Daniel Lezcano wrote:
> On 26/11/2018 10:52, Quentin Perret wrote:

> > Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the

> > top of topology_parse_cpu_capacity() ?

> 

> I prefer to update the documentation, it makes more sense than adding

> more cumbersome tests in the current code.


+1

Throwing an error and ignoring DT number completely for the capacity
are good enough in my opinion as well.

And who cares for the platforms that can't even fill the DT properly :)

-- 
viresh
Quentin Perret Nov. 26, 2018, 11:09 a.m. UTC | #5 
On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:
> On 26-11-18, 11:08, Daniel Lezcano wrote:

> > On 26/11/2018 10:52, Quentin Perret wrote:

> > > Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the

> > > top of topology_parse_cpu_capacity() ?

> > 

> > I prefer to update the documentation, it makes more sense than adding

> > more cumbersome tests in the current code.

> 

> +1

> 

> Throwing an error and ignoring DT number completely for the capacity

> are good enough in my opinion as well.

> 

> And who cares for the platforms that can't even fill the DT properly :)


Right, I think we all agree the case with a partially filled DT is
broken. I don't actually care too much about the behaviour in this case,
but it needs to be consistent with the doc.

So, as long as you fix the doc, that change is fine by me :-)

Thanks,
Quentin
Daniel Lezcano Nov. 26, 2018, 11:36 a.m. UTC | #6 
On 26/11/2018 12:09, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:

>> On 26-11-18, 11:08, Daniel Lezcano wrote:

>>> On 26/11/2018 10:52, Quentin Perret wrote:

>>>> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the

>>>> top of topology_parse_cpu_capacity() ?

>>>

>>> I prefer to update the documentation, it makes more sense than adding

>>> more cumbersome tests in the current code.

>>

>> +1

>>

>> Throwing an error and ignoring DT number completely for the capacity

>> are good enough in my opinion as well.

>>

>> And who cares for the platforms that can't even fill the DT properly :)

> 

> Right, I think we all agree the case with a partially filled DT is

> broken. I don't actually care too much about the behaviour in this case,

> but it needs to be consistent with the doc.

> 

> So, as long as you fix the doc, that change is fine by me :-)


Ok what about the following change ?

"

If capacity-dmips-mhz is not specified or if the parsing fails, the
default capacity value will be computed against the highest frequency,
it will result most of the time on the same capacity value. However on
some platform with different OPP set but the same micro-architecture,
the capacity will be scaled down for CPUs having lower frequencies.

"



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Quentin Perret Nov. 26, 2018, 12:15 p.m. UTC | #7 
On Monday 26 Nov 2018 at 12:36:31 (+0100), Daniel Lezcano wrote:
> On 26/11/2018 12:09, Quentin Perret wrote:

> > On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:

> >> On 26-11-18, 11:08, Daniel Lezcano wrote:

> >>> On 26/11/2018 10:52, Quentin Perret wrote:

> >>>> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the

> >>>> top of topology_parse_cpu_capacity() ?

> >>>

> >>> I prefer to update the documentation, it makes more sense than adding

> >>> more cumbersome tests in the current code.

> >>

> >> +1

> >>

> >> Throwing an error and ignoring DT number completely for the capacity

> >> are good enough in my opinion as well.

> >>

> >> And who cares for the platforms that can't even fill the DT properly :)

> > 

> > Right, I think we all agree the case with a partially filled DT is

> > broken. I don't actually care too much about the behaviour in this case,

> > but it needs to be consistent with the doc.

> > 

> > So, as long as you fix the doc, that change is fine by me :-)

> 

> Ok what about the following change ?

> 

> "

> 

> If capacity-dmips-mhz is not specified or if the parsing fails, the

> default capacity value will be computed against the highest frequency,

> it will result most of the time on the same capacity value.


That "most of the time" sounds a bit odd no ? Maybe mention explicitly
the case you're referring to (that is when all CPUs have the same max
freq) ?

> However on

> some platform with different OPP set but the same micro-architecture,

> the capacity will be scaled down for CPUs having lower frequencies.

> 

> "


Other than that LGTM.

Thanks,
Quentin
diff mbox series

Patch

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index fd5325b..e0c5b60 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -243,9 +243,20 @@  static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || !raw_capacity)
+	if (!acpi_disabled)
 		return -EINVAL;
 
+	if (!raw_capacity) {
+
+		pr_info("cpu_capacity: No capacity defined in DT, set default "
+		       "values to %ld\n", SCHED_CAPACITY_SCALE);
+
+		raw_capacity = kmalloc_array(num_possible_cpus(),
+					     sizeof(*raw_capacity), GFP_KERNEL);
+		if (!raw_capacity)
+			return -ENOMEM;
+	}
+
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
 		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
 		return -ENOMEM;