Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1347692 > unrolled thread

[PATCH 5/6] cpufreq: Support for fast frequency switching

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/6] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-02 03:30 +0100
    Re: [PATCH 5/6] cpufreq: Support for fast frequency switching Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-03 07:10 +0100
      Re: [PATCH 5/6] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-04 03:20 +0100
    Re: [PATCH 5/6] cpufreq: Support for fast frequency switching Peter Zijlstra <peterz@infradead.org> - 2016-03-03 12:20 +0100
      Re: [PATCH 5/6] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-03 20:40 +0100
    Re: [PATCH 5/6] cpufreq: Support for fast frequency switching Peter Zijlstra <peterz@infradead.org> - 2016-03-03 12:20 +0100
      Re: [PATCH 5/6] cpufreq: Support for fast frequency switching "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-03 22:00 +0100
        Re: [PATCH 5/6] cpufreq: Support for fast frequency switching Peter Zijlstra <peterz@infradead.org> - 2016-03-03 22:20 +0100

#1347692 — [PATCH 5/6] cpufreq: Support for fast frequency switching

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify the ACPI cpufreq driver to provide a method for switching
CPU frequencies from interrupt context and update the cpufreq core
to support that method if available.

Introduce a new cpufreq driver callback, ->fast_switch, to be
invoked for frequency switching from interrupt context via a
new helper function, cpufreq_driver_fast_switch().  Add a new
policy flag, fast_switch_possible, to be set if fast frequency
switching can be used for the given policy.

Implement the ->fast_switch callback in the ACPI cpufreq driver
and make it set fast_switch_possible during policy initialization
as appropriate.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

The most important change from the previous version is that the
->fast_switch() callback takes an additional "relation" argument
and now the governor can use it to choose a selection method.

---
 drivers/cpufreq/acpi-cpufreq.c |   53 +++++++++++++++++++++++++++++++++++++++++
 drivers/cpufreq/cpufreq.c      |   33 +++++++++++++++++++++++++
 include/linux/cpufreq.h        |    6 ++++
 3 files changed, 92 insertions(+)

Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
+++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
@@ -458,6 +458,55 @@ static int acpi_cpufreq_target(struct cp
 	return result;
 }
 
+unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
+				      unsigned int target_freq,
+				      unsigned int relation)
+{
+	struct acpi_cpufreq_data *data = policy->driver_data;
+	struct acpi_processor_performance *perf;
+	struct cpufreq_frequency_table *entry, *found;
+	unsigned int next_perf_state, next_freq, freq;
+
+	/*
+	 * Find the closest frequency above target_freq or equal to it.
+	 *
+	 * The table is sorted in the reverse order with respect to the
+	 * frequency and all of the entries are valid (see the initialization).
+	 */
+	entry = data->freq_table;
+	do {
+		entry++;
+		freq = entry->frequency;
+	} while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
+	found = entry - 1;
+	/*
+	 * Use the one found or the previous one, depending on the relation.
+	 * CPUFREQ_RELATION_H is not taken into account here, but it is not
+	 * expected to be passed to this function anyway.
+	 */
+	next_freq = found->frequency;
+	if (freq == CPUFREQ_TABLE_END || relation != CPUFREQ_RELATION_C ||
+	    target_freq - freq >= next_freq - target_freq) {
+		next_perf_state = found->driver_data;
+	} else {
+		next_freq = freq;
+		next_perf_state = entry->driver_data;
+	}
+
+	perf = to_perf_data(data);
+	if (perf->state == next_perf_state) {
+		if (unlikely(data->resume))
+			data->resume = 0;
+		else
+			return next_freq;
+	}
+
+	data->cpu_freq_write(&perf->control_register,
+			     perf->states[next_perf_state].control);
+	perf->state = next_perf_state;
+	return next_freq;
+}
+
 static unsigned long
 acpi_cpufreq_guess_freq(struct acpi_cpufreq_data *data, unsigned int cpu)
 {
@@ -740,6 +789,9 @@ static int acpi_cpufreq_cpu_init(struct
 		goto err_unreg;
 	}
 
+	policy->fast_switch_possible = !acpi_pstate_strict &&
+		!(policy_is_shared(policy) && policy->shared_type != CPUFREQ_SHARED_TYPE_ANY);
+
 	data->freq_table = kzalloc(sizeof(*data->freq_table) *
 		    (perf->state_count+1), GFP_KERNEL);
 	if (!data->freq_table) {
@@ -874,6 +926,7 @@ static struct freq_attr *acpi_cpufreq_at
 static struct cpufreq_driver acpi_cpufreq_driver = {
 	.verify		= cpufreq_generic_frequency_table_verify,
 	.target_index	= acpi_cpufreq_target,
+	.fast_switch	= acpi_cpufreq_fast_switch,
 	.bios_limit	= acpi_processor_get_bios_limit,
 	.init		= acpi_cpufreq_cpu_init,
 	.exit		= acpi_cpufreq_cpu_exit,
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1772,6 +1772,39 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
  *                              GOVERNORS                            *
  *********************************************************************/
 
+/**
+ * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
+ * @policy: cpufreq policy to switch the frequency for.
+ * @target_freq: New frequency to set (may be approximate).
+ * @relation: Relation to use for frequency selection.
+ *
+ * Carry out a fast frequency switch from interrupt context.
+ *
+ * This function must not be called if policy->fast_switch_possible is unset.
+ *
+ * Governors calling this function must guarantee that it will never be invoked
+ * twice in parallel for the same policy and that it will never be called in
+ * parallel with either ->target() or ->target_index() for the same policy.
+ *
+ * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
+ * callback, the hardware configuration must be preserved.
+ */
+void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+				unsigned int target_freq, unsigned int relation)
+{
+	unsigned int freq;
+
+	if (target_freq == policy->cur)
+		return;
+
+	freq = cpufreq_driver->fast_switch(policy, target_freq, relation);
+	if (freq != CPUFREQ_ENTRY_INVALID) {
+		policy->cur = freq;
+		trace_cpu_frequency(freq, smp_processor_id());
+	}
+}
+EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
+
 /* Must set freqs->new to intermediate frequency */
 static int __target_intermediate(struct cpufreq_policy *policy,
 				 struct cpufreq_freqs *freqs, int index)
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -81,6 +81,7 @@ struct cpufreq_policy {
 	struct cpufreq_governor	*governor; /* see below */
 	void			*governor_data;
 	char			last_governor[CPUFREQ_NAME_LEN]; /* last governor used */
+	bool			fast_switch_possible;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
@@ -270,6 +271,9 @@ struct cpufreq_driver {
 				  unsigned int relation);	/* Deprecated */
 	int		(*target_index)(struct cpufreq_policy *policy,
 					unsigned int index);
+	unsigned int	(*fast_switch)(struct cpufreq_policy *policy,
+				       unsigned int target_freq,
+				       unsigned int relation);
 	/*
 	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
 	 * unset.
@@ -484,6 +488,8 @@ struct cpufreq_governor {
 };
 
 /* Pass a target to the cpufreq driver */
+void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
+				unsigned int target_freq, unsigned int relation);
 int cpufreq_driver_target(struct cpufreq_policy *policy,
 				 unsigned int target_freq,
 				 unsigned int relation);

[toc] | [next] | [standalone]

#1348805

On 02-03-16, 03:12, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -1772,6 +1772,39 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>   *                              GOVERNORS                            *
>   *********************************************************************/
>  
> +/**
> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
> + * @policy: cpufreq policy to switch the frequency for.
> + * @target_freq: New frequency to set (may be approximate).
> + * @relation: Relation to use for frequency selection.
> + *
> + * Carry out a fast frequency switch from interrupt context.
> + *
> + * This function must not be called if policy->fast_switch_possible is unset.
> + *
> + * Governors calling this function must guarantee that it will never be invoked
> + * twice in parallel for the same policy and that it will never be called in
> + * parallel with either ->target() or ->target_index() for the same policy.
> + *
> + * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
> + * callback, the hardware configuration must be preserved.
> + */
> +void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> +				unsigned int target_freq, unsigned int relation)
> +{
> +	unsigned int freq;
> +
> +	if (target_freq == policy->cur)

Maybe an unlikely() here ?

> +		return;
> +
> +	freq = cpufreq_driver->fast_switch(policy, target_freq, relation);
> +	if (freq != CPUFREQ_ENTRY_INVALID) {
> +		policy->cur = freq;

Hmm.. What will happen to the code relying on the cpufreq-notifiers
now ?

> +		trace_cpu_frequency(freq, smp_processor_id());
> +	}
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1349784

On Thu, Mar 3, 2016 at 7:00 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-03-16, 03:12, Rafael J. Wysocki wrote:
>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>> @@ -1772,6 +1772,39 @@ EXPORT_SYMBOL(cpufreq_unregister_notifie
>>   *                              GOVERNORS                            *
>>   *********************************************************************/
>>
>> +/**
>> + * cpufreq_driver_fast_switch - Carry out a fast CPU frequency switch.
>> + * @policy: cpufreq policy to switch the frequency for.
>> + * @target_freq: New frequency to set (may be approximate).
>> + * @relation: Relation to use for frequency selection.
>> + *
>> + * Carry out a fast frequency switch from interrupt context.
>> + *
>> + * This function must not be called if policy->fast_switch_possible is unset.
>> + *
>> + * Governors calling this function must guarantee that it will never be invoked
>> + * twice in parallel for the same policy and that it will never be called in
>> + * parallel with either ->target() or ->target_index() for the same policy.
>> + *
>> + * If CPUFREQ_ENTRY_INVALID is returned by the driver's ->fast_switch()
>> + * callback, the hardware configuration must be preserved.
>> + */
>> +void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>> +                             unsigned int target_freq, unsigned int relation)
>> +{
>> +     unsigned int freq;
>> +
>> +     if (target_freq == policy->cur)
>
> Maybe an unlikely() here ?
>
>> +             return;
>> +
>> +     freq = cpufreq_driver->fast_switch(policy, target_freq, relation);
>> +     if (freq != CPUFREQ_ENTRY_INVALID) {
>> +             policy->cur = freq;
>
> Hmm.. What will happen to the code relying on the cpufreq-notifiers
> now ?

It will have a problem.

For that code it's like the CPU changing the frequency and not telling
it (which is not unusual for that matter).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1348997

On Wed, Mar 02, 2016 at 03:12:33AM +0100, Rafael J. Wysocki wrote:
> +void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> +				unsigned int target_freq, unsigned int relation)
> +{
> +	unsigned int freq;
> +
> +	if (target_freq == policy->cur)
> +		return;

But what if relation is different from last time? ;-)

[toc] | [prev] | [next] | [standalone]

#1349526

On Thu, Mar 3, 2016 at 12:18 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 02, 2016 at 03:12:33AM +0100, Rafael J. Wysocki wrote:
>> +void cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>> +                             unsigned int target_freq, unsigned int relation)
>> +{
>> +     unsigned int freq;
>> +
>> +     if (target_freq == policy->cur)
>> +             return;
>
> But what if relation is different from last time? ;-)

Doh

Never mind, I'll drop this check (that said this mistake is present
elsewhere too IIRC).

[toc] | [prev] | [next] | [standalone]

#1349000

On Wed, Mar 02, 2016 at 03:12:33AM +0100, Rafael J. Wysocki wrote:
> The most important change from the previous version is that the
> ->fast_switch() callback takes an additional "relation" argument
> and now the governor can use it to choose a selection method.

> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> +				      unsigned int target_freq,
> +				      unsigned int relation)

Would it make sense to replace the {target_freq, relation} pair with
something like the CPPC {min_freq, max_freq} pair?

Then you could use the closest frequency to max provided it is larger
than min.

This communicates more actual information in the same number of
parameters and would thereby allow for a more flexible (better)
frequency selection.

[toc] | [prev] | [next] | [standalone]

#1349570

On Thu, Mar 3, 2016 at 12:16 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 02, 2016 at 03:12:33AM +0100, Rafael J. Wysocki wrote:
>> The most important change from the previous version is that the
>> ->fast_switch() callback takes an additional "relation" argument
>> and now the governor can use it to choose a selection method.
>
>> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
>> +                                   unsigned int target_freq,
>> +                                   unsigned int relation)
>
> Would it make sense to replace the {target_freq, relation} pair with
> something like the CPPC {min_freq, max_freq} pair?

Yes, it would in general, but since I use __cpufreq_driver_target() in
the "slow driver" case, that would need to be reworked too for
consistency.  So I'd prefer to do that later.

> Then you could use the closest frequency to max provided it is larger
> than min.
>
> This communicates more actual information in the same number of
> parameters and would thereby allow for a more flexible (better)
> frequency selection.

Agreed.

[toc] | [prev] | [next] | [standalone]

#1349588

On Thu, Mar 03, 2016 at 09:56:40PM +0100, Rafael J. Wysocki wrote:
> On Thu, Mar 3, 2016 at 12:16 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Mar 02, 2016 at 03:12:33AM +0100, Rafael J. Wysocki wrote:
> >> The most important change from the previous version is that the
> >> ->fast_switch() callback takes an additional "relation" argument
> >> and now the governor can use it to choose a selection method.
> >
> >> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> >> +                                   unsigned int target_freq,
> >> +                                   unsigned int relation)
> >
> > Would it make sense to replace the {target_freq, relation} pair with
> > something like the CPPC {min_freq, max_freq} pair?
> 
> Yes, it would in general, but since I use __cpufreq_driver_target() in
> the "slow driver" case, that would need to be reworked too for
> consistency.  So I'd prefer to do that later.

OK, fair enough.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web