* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table [not found] <CADGdYn6ehM36k1O3O6mXnHRJBy42N5iACnbOB+bwjPx+RAqQTw@mail.gmail.com> @ 2013-02-07 1:56 ` amit daniel kachhap 2013-02-07 2:53 ` Zhang Rui 0 siblings, 1 reply; 7+ messages in thread From: amit daniel kachhap @ 2013-02-07 1:56 UTC (permalink / raw) To: Zhang Rui; +Cc: linux-pm, Gu1, Hongbo Zhang, amit kachhap Hi Rui, I tested this code and after 2 changes it started working. Also calling memory allocation for getting cpufreq table is quite expensive and might also fail. Storing the frequency again in a new table can be avoided and only required frequency or level can be returned. Thanks, Amit Daniel On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote: > CPU cooling table is a cleaned cpufreq_frequency_table that > 1. each entry represents a cooling state, > aka, each entry has a different frequency. > 2. does not have invalid entry. > 3. does not have duplicate entry. > 4. the frequencies of all the entries are in descending order. > > we should use this table inside thermal layer and thermal drivers. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 455c77a..08f12c7 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) > return !cpufreq_get_policy(&policy, cpu); > } > > +/* > + * get the cleaned cpufreq_frequency_table that > + * 1) does not have invalid entries > + * 2) does not have duplicate entries > + * 3) the frequency of the entries are in descending order > + */ > +static struct cpufreq_frequency_table * > +get_cpu_cooling_table(unsigned int cpu) > +{ > + int i = 0; > + int level; > + struct cpufreq_frequency_table *old, *new; > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > + int descend = -1; > + > + old = cpufreq_frequency_get_table(cpu); > + if (!old) > + return ERR_PTR(-EINVAL); > + > + while (old[i].frequency != CPUFREQ_TABLE_END) > + i++; > + > + i++; /* one more entry for CPUFREQ_TABLE_END */ > + > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); > + if (!new) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { > + /* ignore invalid entry */ > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) > + continue; > + > + /* ignore duplicate entry */ > + if (freq == old[i].frequency) > + continue; > + > + /* found an valid entry */ > + new[level].frequency = old[i].frequency; > + > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) > + descend = !!(freq > old[i].frequency); > + > + /* freq always equals the last valid frequency */ > + freq = new[level].frequency; > + > + level++; > + } > + new[level].frequency = CPUFREQ_ENTRY_INVALID; > + > + /* convert to decending if in ascending order */ > + if (!descend) { > + for(i = 0; i < (level - i - 1); i++) { > + int j = level - i - 1; > + freq = new[i].frequency; > + new[i].frequency = new[j].frequency; > + new[j].frequency = freq; > + } > + } > + > + return new; > +} > + > /** > * get_cpu_frequency - get the absolute value of frequency from level. > * @cpu: cpu for which frequency is fetched. > -- > 1.7.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-07 1:56 ` [PATCH 2/5] Thermal: Introduce cpu cooling table amit daniel kachhap @ 2013-02-07 2:53 ` Zhang Rui 2013-02-07 23:18 ` amit kachhap 0 siblings, 1 reply; 7+ messages in thread From: Zhang Rui @ 2013-02-07 2:53 UTC (permalink / raw) To: amit daniel kachhap; +Cc: linux-pm, Gu1, Hongbo Zhang, amit kachhap On Wed, 2013-02-06 at 17:56 -0800, amit daniel kachhap wrote: > Hi Rui, > > I tested this code and after 2 changes it started working. > which two changes? > Also calling memory allocation for getting cpufreq table is quite > expensive and might also fail. > Storing the frequency again in a new > table can be avoided you mean it is not worth creating a cpu cooling table? do you mean this memory allocation is quite expensive for SOC? Sorry I do not have too much experience on SOC. > and only required frequency or level can be > returned. > Sorry, I do not understand. But it is true that we do not have to allocate a new table to get the level/frequency. I just want to make the cooling state <-> frequency transition clear and consistent for cpufreq cooling. I can work out another version without memory allocation, if that's what you want. thanks, rui > Thanks, > Amit Daniel > > On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote: > > CPU cooling table is a cleaned cpufreq_frequency_table that > > 1. each entry represents a cooling state, > > aka, each entry has a different frequency. > > 2. does not have invalid entry. > > 3. does not have duplicate entry. > > 4. the frequencies of all the entries are in descending order. > > > > we should use this table inside thermal layer and thermal drivers. > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 63 insertions(+) > > > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > > index 455c77a..08f12c7 100644 > > --- a/drivers/thermal/cpu_cooling.c > > +++ b/drivers/thermal/cpu_cooling.c > > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) > > return !cpufreq_get_policy(&policy, cpu); > > } > > > > +/* > > + * get the cleaned cpufreq_frequency_table that > > + * 1) does not have invalid entries > > + * 2) does not have duplicate entries > > + * 3) the frequency of the entries are in descending order > > + */ > > +static struct cpufreq_frequency_table * > > +get_cpu_cooling_table(unsigned int cpu) > > +{ > > + int i = 0; > > + int level; > > + struct cpufreq_frequency_table *old, *new; > > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > > + int descend = -1; > > + > > + old = cpufreq_frequency_get_table(cpu); > > + if (!old) > > + return ERR_PTR(-EINVAL); > > + > > + while (old[i].frequency != CPUFREQ_TABLE_END) > > + i++; > > + > > + i++; /* one more entry for CPUFREQ_TABLE_END */ > > + > > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); > > + if (!new) > > + return ERR_PTR(-ENOMEM); > > + > > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { > > + /* ignore invalid entry */ > > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) > > + continue; > > + > > + /* ignore duplicate entry */ > > + if (freq == old[i].frequency) > > + continue; > > + > > + /* found an valid entry */ > > + new[level].frequency = old[i].frequency; > > + > > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) > > + descend = !!(freq > old[i].frequency); > > + > > + /* freq always equals the last valid frequency */ > > + freq = new[level].frequency; > > + > > + level++; > > + } > > + new[level].frequency = CPUFREQ_ENTRY_INVALID; > > + > > + /* convert to decending if in ascending order */ > > + if (!descend) { > > + for(i = 0; i < (level - i - 1); i++) { > > + int j = level - i - 1; > > + freq = new[i].frequency; > > + new[i].frequency = new[j].frequency; > > + new[j].frequency = freq; > > + } > > + } > > + > > + return new; > > +} > > + > > /** > > * get_cpu_frequency - get the absolute value of frequency from level. > > * @cpu: cpu for which frequency is fetched. > > -- > > 1.7.9.5 > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-07 2:53 ` Zhang Rui @ 2013-02-07 23:18 ` amit kachhap 0 siblings, 0 replies; 7+ messages in thread From: amit kachhap @ 2013-02-07 23:18 UTC (permalink / raw) To: Zhang Rui; +Cc: linux-pm, Gu1, Hongbo Zhang On Wed, Feb 6, 2013 at 6:53 PM, Zhang Rui <rui.zhang@intel.com> wrote: > On Wed, 2013-02-06 at 17:56 -0800, amit daniel kachhap wrote: >> Hi Rui, >> >> I tested this code and after 2 changes it started working. Oh sorry I didn't explained it . Actually I replied the problems to their corresponding patches. >> > which two changes? > >> Also calling memory allocation for getting cpufreq table is quite >> expensive and might also fail. >> Storing the frequency again in a new >> table can be avoided > > you mean it is not worth creating a cpu cooling table? > > do you mean this memory allocation is quite expensive for SOC? > Sorry I do not have too much experience on SOC. > >> and only required frequency or level can be >> returned. >> > Sorry, I do not understand. > But it is true that we do not have to allocate a new table to get the > level/frequency. I meant the same that it is unnecessary to allocate the cpufreq table and then free them. Thanks. Amit Daniel > I just want to make the cooling state <-> frequency transition clear and > consistent for cpufreq cooling. > I can work out another version without memory allocation, if that's what > you want. > > thanks, > rui >> Thanks, >> Amit Daniel >> >> On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote: >> > CPU cooling table is a cleaned cpufreq_frequency_table that >> > 1. each entry represents a cooling state, >> > aka, each entry has a different frequency. >> > 2. does not have invalid entry. >> > 3. does not have duplicate entry. >> > 4. the frequencies of all the entries are in descending order. >> > >> > we should use this table inside thermal layer and thermal drivers. >> > >> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> > --- >> > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 63 insertions(+) >> > >> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> > index 455c77a..08f12c7 100644 >> > --- a/drivers/thermal/cpu_cooling.c >> > +++ b/drivers/thermal/cpu_cooling.c >> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) >> > return !cpufreq_get_policy(&policy, cpu); >> > } >> > >> > +/* >> > + * get the cleaned cpufreq_frequency_table that >> > + * 1) does not have invalid entries >> > + * 2) does not have duplicate entries >> > + * 3) the frequency of the entries are in descending order >> > + */ >> > +static struct cpufreq_frequency_table * >> > +get_cpu_cooling_table(unsigned int cpu) >> > +{ >> > + int i = 0; >> > + int level; >> > + struct cpufreq_frequency_table *old, *new; >> > + unsigned int freq = CPUFREQ_ENTRY_INVALID; >> > + int descend = -1; >> > + >> > + old = cpufreq_frequency_get_table(cpu); >> > + if (!old) >> > + return ERR_PTR(-EINVAL); >> > + >> > + while (old[i].frequency != CPUFREQ_TABLE_END) >> > + i++; >> > + >> > + i++; /* one more entry for CPUFREQ_TABLE_END */ >> > + >> > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); >> > + if (!new) >> > + return ERR_PTR(-ENOMEM); >> > + >> > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { >> > + /* ignore invalid entry */ >> > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) >> > + continue; >> > + >> > + /* ignore duplicate entry */ >> > + if (freq == old[i].frequency) >> > + continue; >> > + >> > + /* found an valid entry */ >> > + new[level].frequency = old[i].frequency; >> > + >> > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) >> > + descend = !!(freq > old[i].frequency); >> > + >> > + /* freq always equals the last valid frequency */ >> > + freq = new[level].frequency; >> > + >> > + level++; >> > + } >> > + new[level].frequency = CPUFREQ_ENTRY_INVALID; >> > + >> > + /* convert to decending if in ascending order */ >> > + if (!descend) { >> > + for(i = 0; i < (level - i - 1); i++) { >> > + int j = level - i - 1; >> > + freq = new[i].frequency; >> > + new[i].frequency = new[j].frequency; >> > + new[j].frequency = freq; >> > + } >> > + } >> > + >> > + return new; >> > +} >> > + >> > /** >> > * get_cpu_frequency - get the absolute value of frequency from level. >> > * @cpu: cpu for which frequency is fetched. >> > -- >> > 1.7.9.5 >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/5] Thermal: cpu cooling fix @ 2013-02-06 4:30 Zhang Rui 2013-02-06 4:30 ` [PATCH 2/5] Thermal: Introduce cpu cooling table Zhang Rui 0 siblings, 1 reply; 7+ messages in thread From: Zhang Rui @ 2013-02-06 4:30 UTC (permalink / raw) To: linux-pm; +Cc: amit.daniel, gu1, hongbo.zhang, Zhang Rui the per_cpu cpufreq_frequency_table contains all the frequency entries which are reported by platform code. But this table is not clean enough because it may contain invalid, duplicate entries, and it may be in either descending or asceding order. converting between cpu frequency and cooling state using this table in cpufreq cooling code is painful as each cooling state of cpu cooling device must have a valid and unique frequency. thus cpu cooling table is introduced in this patch set. CPU cooling table is a table that 1. each entry represents a cpu cooling state, aka, a different cpu frequency. 2. no invalid entry in this table 3. no duplicate entry in this table 4. the frequency of all the entries are in descending order, aka, table[0] equals maximum frequency and cooling state 0. And this is the table we are using inside cpufreq cooling generic code. Besides, all the cpufreq cooling users, e.g. exynos, should touch neither per_cpu cpufreq_frequency_table nor the cpu cooling table. it should invoke cpufreq cooling API to do cooling_state/frequency transition. I just run build test here. It would be great that some of you can test it on exynos. any comments? thanks, rui ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-06 4:30 [PATCH 0/5] Thermal: cpu cooling fix Zhang Rui @ 2013-02-06 4:30 ` Zhang Rui 2013-02-07 9:36 ` Hongbo Zhang 0 siblings, 1 reply; 7+ messages in thread From: Zhang Rui @ 2013-02-06 4:30 UTC (permalink / raw) To: linux-pm; +Cc: amit.daniel, gu1, hongbo.zhang, Zhang Rui CPU cooling table is a cleaned cpufreq_frequency_table that 1. each entry represents a cooling state, aka, each entry has a different frequency. 2. does not have invalid entry. 3. does not have duplicate entry. 4. the frequencies of all the entries are in descending order. we should use this table inside thermal layer and thermal drivers. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 455c77a..08f12c7 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) return !cpufreq_get_policy(&policy, cpu); } +/* + * get the cleaned cpufreq_frequency_table that + * 1) does not have invalid entries + * 2) does not have duplicate entries + * 3) the frequency of the entries are in descending order + */ +static struct cpufreq_frequency_table * +get_cpu_cooling_table(unsigned int cpu) +{ + int i = 0; + int level; + struct cpufreq_frequency_table *old, *new; + unsigned int freq = CPUFREQ_ENTRY_INVALID; + int descend = -1; + + old = cpufreq_frequency_get_table(cpu); + if (!old) + return ERR_PTR(-EINVAL); + + while (old[i].frequency != CPUFREQ_TABLE_END) + i++; + + i++; /* one more entry for CPUFREQ_TABLE_END */ + + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); + if (!new) + return ERR_PTR(-ENOMEM); + + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { + /* ignore invalid entry */ + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) + continue; + + /* ignore duplicate entry */ + if (freq == old[i].frequency) + continue; + + /* found an valid entry */ + new[level].frequency = old[i].frequency; + + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) + descend = !!(freq > old[i].frequency); + + /* freq always equals the last valid frequency */ + freq = new[level].frequency; + + level++; + } + new[level].frequency = CPUFREQ_ENTRY_INVALID; + + /* convert to decending if in ascending order */ + if (!descend) { + for(i = 0; i < (level - i - 1); i++) { + int j = level - i - 1; + freq = new[i].frequency; + new[i].frequency = new[j].frequency; + new[j].frequency = freq; + } + } + + return new; +} + /** * get_cpu_frequency - get the absolute value of frequency from level. * @cpu: cpu for which frequency is fetched. -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-06 4:30 ` [PATCH 2/5] Thermal: Introduce cpu cooling table Zhang Rui @ 2013-02-07 9:36 ` Hongbo Zhang 2013-02-08 3:30 ` Zhang Rui 0 siblings, 1 reply; 7+ messages in thread From: Hongbo Zhang @ 2013-02-07 9:36 UTC (permalink / raw) To: Zhang Rui; +Cc: linux-pm, amit.daniel, gu1 On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote: > CPU cooling table is a cleaned cpufreq_frequency_table that > 1. each entry represents a cooling state, > aka, each entry has a different frequency. > 2. does not have invalid entry. > 3. does not have duplicate entry. > 4. the frequencies of all the entries are in descending order. > This is a good idea, but we should consider more details, one tough issue is invalid frequencies: 1. some freq is invalid while freq table is creating, but after that the invalid freq becomes valid, we will miss cooling states. 2. vice versa, all the freq is valid while freq table is creating, but become invalid later, thus some cooling state isn't valid either, should we consider such case? In fact, these should be considered even if this patch isn't sent out. (We are assuming that all the CPU cores run at same freq and should be adjust freq simultaneously, if no affinity between CPU cores, it will be more complicated -- this seems not proper to be discussed in this patch) > we should use this table inside thermal layer and thermal drivers. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 455c77a..08f12c7 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) > return !cpufreq_get_policy(&policy, cpu); > } > > +/* > + * get the cleaned cpufreq_frequency_table that > + * 1) does not have invalid entries > + * 2) does not have duplicate entries > + * 3) the frequency of the entries are in descending order > + */ > +static struct cpufreq_frequency_table * > +get_cpu_cooling_table(unsigned int cpu) > +{ > + int i = 0; > + int level; > + struct cpufreq_frequency_table *old, *new; > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > + int descend = -1; > + > + old = cpufreq_frequency_get_table(cpu); > + if (!old) > + return ERR_PTR(-EINVAL); > + > + while (old[i].frequency != CPUFREQ_TABLE_END) > + i++; > + > + i++; /* one more entry for CPUFREQ_TABLE_END */ > + > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); > + if (!new) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { > + /* ignore invalid entry */ > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) > + continue; > + > + /* ignore duplicate entry */ > + if (freq == old[i].frequency) > + continue; > + > + /* found an valid entry */ > + new[level].frequency = old[i].frequency; > + > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) > + descend = !!(freq > old[i].frequency); > + > + /* freq always equals the last valid frequency */ > + freq = new[level].frequency; > + > + level++; > + } > + new[level].frequency = CPUFREQ_ENTRY_INVALID; > + > + /* convert to decending if in ascending order */ > + if (!descend) { > + for(i = 0; i < (level - i - 1); i++) { > + int j = level - i - 1; > + freq = new[i].frequency; > + new[i].frequency = new[j].frequency; > + new[j].frequency = freq; > + } > + } > + > + return new; > +} > + > /** > * get_cpu_frequency - get the absolute value of frequency from level. > * @cpu: cpu for which frequency is fetched. > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-07 9:36 ` Hongbo Zhang @ 2013-02-08 3:30 ` Zhang Rui 2013-02-08 13:09 ` Hongbo Zhang 0 siblings, 1 reply; 7+ messages in thread From: Zhang Rui @ 2013-02-08 3:30 UTC (permalink / raw) To: Hongbo Zhang; +Cc: linux-pm, amit.daniel, gu1 On Thu, 2013-02-07 at 17:36 +0800, Hongbo Zhang wrote: > On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote: > > CPU cooling table is a cleaned cpufreq_frequency_table that > > 1. each entry represents a cooling state, > > aka, each entry has a different frequency. > > 2. does not have invalid entry. > > 3. does not have duplicate entry. > > 4. the frequencies of all the entries are in descending order. > > > This is a good idea, but we should consider more details, one tough > issue is invalid frequencies: > 1. some freq is invalid while freq table is creating, but after that > the invalid freq becomes valid, we will miss cooling states. when will the invalid freq becomes valid? for ACPI platforms, the number of valid cpu frequency should not change at runtime, but BIOS will indicate that only several of them can be used dynamically. I'm not sure if this may behave differently on other platforms. if it is true, the max_state and cur_state may change from time to time. and we should cache current frequency rather than cur_state in cpu cooling code. > 2. vice versa, all the freq is valid while freq table is creating, but > become invalid later, thus some cooling state isn't valid either, > should we consider such case? > In fact, these should be considered even if this patch isn't sent out. > > (We are assuming that all the CPU cores run at same freq and should be > adjust freq simultaneously, if no affinity between CPU cores, it will > be more complicated -- this seems not proper to be discussed in this > patch) > Agreed. But I'll try to fix the duplicate/invalid entries and ascending order issue in this patch set. thanks, rui > > we should use this table inside thermal layer and thermal drivers. > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 63 insertions(+) > > > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > > index 455c77a..08f12c7 100644 > > --- a/drivers/thermal/cpu_cooling.c > > +++ b/drivers/thermal/cpu_cooling.c > > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) > > return !cpufreq_get_policy(&policy, cpu); > > } > > > > +/* > > + * get the cleaned cpufreq_frequency_table that > > + * 1) does not have invalid entries > > + * 2) does not have duplicate entries > > + * 3) the frequency of the entries are in descending order > > + */ > > +static struct cpufreq_frequency_table * > > +get_cpu_cooling_table(unsigned int cpu) > > +{ > > + int i = 0; > > + int level; > > + struct cpufreq_frequency_table *old, *new; > > + unsigned int freq = CPUFREQ_ENTRY_INVALID; > > + int descend = -1; > > + > > + old = cpufreq_frequency_get_table(cpu); > > + if (!old) > > + return ERR_PTR(-EINVAL); > > + > > + while (old[i].frequency != CPUFREQ_TABLE_END) > > + i++; > > + > > + i++; /* one more entry for CPUFREQ_TABLE_END */ > > + > > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); > > + if (!new) > > + return ERR_PTR(-ENOMEM); > > + > > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { > > + /* ignore invalid entry */ > > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) > > + continue; > > + > > + /* ignore duplicate entry */ > > + if (freq == old[i].frequency) > > + continue; > > + > > + /* found an valid entry */ > > + new[level].frequency = old[i].frequency; > > + > > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) > > + descend = !!(freq > old[i].frequency); > > + > > + /* freq always equals the last valid frequency */ > > + freq = new[level].frequency; > > + > > + level++; > > + } > > + new[level].frequency = CPUFREQ_ENTRY_INVALID; > > + > > + /* convert to decending if in ascending order */ > > + if (!descend) { > > + for(i = 0; i < (level - i - 1); i++) { > > + int j = level - i - 1; > > + freq = new[i].frequency; > > + new[i].frequency = new[j].frequency; > > + new[j].frequency = freq; > > + } > > + } > > + > > + return new; > > +} > > + > > /** > > * get_cpu_frequency - get the absolute value of frequency from level. > > * @cpu: cpu for which frequency is fetched. > > -- > > 1.7.9.5 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/5] Thermal: Introduce cpu cooling table 2013-02-08 3:30 ` Zhang Rui @ 2013-02-08 13:09 ` Hongbo Zhang 0 siblings, 0 replies; 7+ messages in thread From: Hongbo Zhang @ 2013-02-08 13:09 UTC (permalink / raw) To: Zhang Rui; +Cc: linux-pm, amit.daniel, gu1 On 8 February 2013 11:30, Zhang Rui <rui.zhang@intel.com> wrote: > On Thu, 2013-02-07 at 17:36 +0800, Hongbo Zhang wrote: >> On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote: >> > CPU cooling table is a cleaned cpufreq_frequency_table that >> > 1. each entry represents a cooling state, >> > aka, each entry has a different frequency. >> > 2. does not have invalid entry. >> > 3. does not have duplicate entry. >> > 4. the frequencies of all the entries are in descending order. >> > >> This is a good idea, but we should consider more details, one tough >> issue is invalid frequencies: >> 1. some freq is invalid while freq table is creating, but after that >> the invalid freq becomes valid, we will miss cooling states. > > when will the invalid freq becomes valid? > for ACPI platforms, the number of valid cpu frequency should not change > at runtime, but BIOS will indicate that only several of them can be used > dynamically. > I'm not sure if this may behave differently on other platforms. > if it is true, the max_state and cur_state may change from time to time. > and we should cache current frequency rather than cur_state in cpu > cooling code. > Honestly, I am not expert of cpufreq, just know that we can set scaling_max_freq and scaling_min_freq under /sys/devices/system/cpu/cpu*/cpufreq/, so the cpufreq scaling range can be changed. Correct me if I am wrong. >> 2. vice versa, all the freq is valid while freq table is creating, but >> become invalid later, thus some cooling state isn't valid either, >> should we consider such case? > >> In fact, these should be considered even if this patch isn't sent out. >> >> (We are assuming that all the CPU cores run at same freq and should be >> adjust freq simultaneously, if no affinity between CPU cores, it will >> be more complicated -- this seems not proper to be discussed in this >> patch) >> > Agreed. > But I'll try to fix the duplicate/invalid entries and ascending order > issue in this patch set. > > thanks, > rui > >> > we should use this table inside thermal layer and thermal drivers. >> > >> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> > --- >> > drivers/thermal/cpu_cooling.c | 63 +++++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 63 insertions(+) >> > >> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> > index 455c77a..08f12c7 100644 >> > --- a/drivers/thermal/cpu_cooling.c >> > +++ b/drivers/thermal/cpu_cooling.c >> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu) >> > return !cpufreq_get_policy(&policy, cpu); >> > } >> > >> > +/* >> > + * get the cleaned cpufreq_frequency_table that >> > + * 1) does not have invalid entries >> > + * 2) does not have duplicate entries >> > + * 3) the frequency of the entries are in descending order >> > + */ >> > +static struct cpufreq_frequency_table * >> > +get_cpu_cooling_table(unsigned int cpu) >> > +{ >> > + int i = 0; >> > + int level; >> > + struct cpufreq_frequency_table *old, *new; >> > + unsigned int freq = CPUFREQ_ENTRY_INVALID; >> > + int descend = -1; >> > + >> > + old = cpufreq_frequency_get_table(cpu); >> > + if (!old) >> > + return ERR_PTR(-EINVAL); >> > + >> > + while (old[i].frequency != CPUFREQ_TABLE_END) >> > + i++; >> > + >> > + i++; /* one more entry for CPUFREQ_TABLE_END */ >> > + >> > + new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL); >> > + if (!new) >> > + return ERR_PTR(-ENOMEM); >> > + >> > + for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) { >> > + /* ignore invalid entry */ >> > + if (old[i].frequency == CPUFREQ_ENTRY_INVALID) >> > + continue; >> > + >> > + /* ignore duplicate entry */ >> > + if (freq == old[i].frequency) >> > + continue; >> > + >> > + /* found an valid entry */ >> > + new[level].frequency = old[i].frequency; >> > + >> > + if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) >> > + descend = !!(freq > old[i].frequency); >> > + >> > + /* freq always equals the last valid frequency */ >> > + freq = new[level].frequency; >> > + >> > + level++; >> > + } >> > + new[level].frequency = CPUFREQ_ENTRY_INVALID; >> > + >> > + /* convert to decending if in ascending order */ >> > + if (!descend) { >> > + for(i = 0; i < (level - i - 1); i++) { >> > + int j = level - i - 1; >> > + freq = new[i].frequency; >> > + new[i].frequency = new[j].frequency; >> > + new[j].frequency = freq; >> > + } >> > + } >> > + >> > + return new; >> > +} >> > + >> > /** >> > * get_cpu_frequency - get the absolute value of frequency from level. >> > * @cpu: cpu for which frequency is fetched. >> > -- >> > 1.7.9.5 >> > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-02-08 13:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CADGdYn6ehM36k1O3O6mXnHRJBy42N5iACnbOB+bwjPx+RAqQTw@mail.gmail.com>
2013-02-07 1:56 ` [PATCH 2/5] Thermal: Introduce cpu cooling table amit daniel kachhap
2013-02-07 2:53 ` Zhang Rui
2013-02-07 23:18 ` amit kachhap
2013-02-06 4:30 [PATCH 0/5] Thermal: cpu cooling fix Zhang Rui
2013-02-06 4:30 ` [PATCH 2/5] Thermal: Introduce cpu cooling table Zhang Rui
2013-02-07 9:36 ` Hongbo Zhang
2013-02-08 3:30 ` Zhang Rui
2013-02-08 13:09 ` Hongbo Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.