| Message ID | 20180703234705.227473-4-mka@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Matthias, On 2018년 07월 04일 08:46, Matthias Kaehlcke wrote: > Several governors use the user space limits df->min/max_freq to adjust > the target frequency. This is not necessary, since update_devfreq() > already takes care of this. Instead the governor can request the available > min/max frequency by setting the target frequency to DEVFREQ_MIN/MAX_FREQ > and let update_devfreq() take care of any adjustments. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > Changes in v5: > - none > > Changes in v4: > - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag > > Changes in v3: > - none > > Changes in v2: > - squashed "PM / devfreq: Remove redundant frequency adjustment from governors" > and "PM / devfreq: governors: Return device frequency limits instead of user > limits" > - updated subject and commit message > - use DEVFREQ_MIN/MAX_FREQ instead of df->scaling_min/max_freq > --- > drivers/devfreq/governor.h | 3 +++ > drivers/devfreq/governor_performance.c | 5 +---- > drivers/devfreq/governor_powersave.c | 2 +- > drivers/devfreq/governor_simpleondemand.c | 12 +++--------- > drivers/devfreq/governor_userspace.c | 16 ++++------------ > 5 files changed, 12 insertions(+), 26 deletions(-) Actually, I preferred to use 'df->scaling_min/max_freq' instead of DEVFREQ_MIN/MAX_FREQ. But, DEVFREQ_MIN/MAX_FREQ is other way. So, Looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> [snip]
Hi Chanwoo, this patch and "PM / devfreq: Fix handling of min/max_freq == 0" address issues not directly related with the throttler. It seems it could still take a while for the throttler to move forward, do you want me to spin out these two patches so that they can get merged independently from the rest of the series? Thanks Matthias On Tue, Jul 03, 2018 at 04:46:56PM -0700, Matthias Kaehlcke wrote: > Several governors use the user space limits df->min/max_freq to adjust > the target frequency. This is not necessary, since update_devfreq() > already takes care of this. Instead the governor can request the available > min/max frequency by setting the target frequency to DEVFREQ_MIN/MAX_FREQ > and let update_devfreq() take care of any adjustments. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > Changes in v5: > - none > > Changes in v4: > - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag > > Changes in v3: > - none > > Changes in v2: > - squashed "PM / devfreq: Remove redundant frequency adjustment from governors" > and "PM / devfreq: governors: Return device frequency limits instead of user > limits" > - updated subject and commit message > - use DEVFREQ_MIN/MAX_FREQ instead of df->scaling_min/max_freq > --- > drivers/devfreq/governor.h | 3 +++ > drivers/devfreq/governor_performance.c | 5 +---- > drivers/devfreq/governor_powersave.c | 2 +- > drivers/devfreq/governor_simpleondemand.c | 12 +++--------- > drivers/devfreq/governor_userspace.c | 16 ++++------------ > 5 files changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index cfc50a61a90d..b81700244ce3 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -25,6 +25,9 @@ > #define DEVFREQ_GOV_SUSPEND 0x4 > #define DEVFREQ_GOV_RESUME 0x5 > > +#define DEVFREQ_MIN_FREQ 0 > +#define DEVFREQ_MAX_FREQ ULONG_MAX > + > /** > * struct devfreq_governor - Devfreq policy governor > * @node: list node - contains registered devfreq governors > diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c > index 4d23ecfbd948..ded429fd51be 100644 > --- a/drivers/devfreq/governor_performance.c > +++ b/drivers/devfreq/governor_performance.c > @@ -20,10 +20,7 @@ static int devfreq_performance_func(struct devfreq *df, > * target callback should be able to get floor value as > * said in devfreq.h > */ > - if (!df->max_freq) > - *freq = UINT_MAX; > - else > - *freq = df->max_freq; > + *freq = DEVFREQ_MAX_FREQ; > return 0; > } > > diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c > index 0c42f23249ef..9e8897f5ac42 100644 > --- a/drivers/devfreq/governor_powersave.c > +++ b/drivers/devfreq/governor_powersave.c > @@ -20,7 +20,7 @@ static int devfreq_powersave_func(struct devfreq *df, > * target callback should be able to get ceiling value as > * said in devfreq.h > */ > - *freq = df->min_freq; > + *freq = DEVFREQ_MIN_FREQ; > return 0; > } > > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c > index 28e0f2de7100..c0417f0e081e 100644 > --- a/drivers/devfreq/governor_simpleondemand.c > +++ b/drivers/devfreq/governor_simpleondemand.c > @@ -27,7 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > struct devfreq_simple_ondemand_data *data = df->data; > - unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; > > err = devfreq_update_stats(df); > if (err) > @@ -47,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > > /* Assume MAX if it is going to be divided by zero */ > if (stat->total_time == 0) { > - *freq = max; > + *freq = DEVFREQ_MAX_FREQ; > return 0; > } > > @@ -60,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > /* Set MAX if it's busy enough */ > if (stat->busy_time * 100 > > stat->total_time * dfso_upthreshold) { > - *freq = max; > + *freq = DEVFREQ_MAX_FREQ; > return 0; > } > > /* Set MAX if we do not know the initial frequency */ > if (stat->current_frequency == 0) { > - *freq = max; > + *freq = DEVFREQ_MAX_FREQ; > return 0; > } > > @@ -85,11 +84,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); > *freq = (unsigned long) b; > > - if (df->min_freq && *freq < df->min_freq) > - *freq = df->min_freq; > - if (df->max_freq && *freq > df->max_freq) > - *freq = df->max_freq; > - > return 0; > } > > diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c > index 080607c3f34d..378d84c011df 100644 > --- a/drivers/devfreq/governor_userspace.c > +++ b/drivers/devfreq/governor_userspace.c > @@ -26,19 +26,11 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) > { > struct userspace_data *data = df->data; > > - if (data->valid) { > - unsigned long adjusted_freq = data->user_frequency; > - > - if (df->max_freq && adjusted_freq > df->max_freq) > - adjusted_freq = df->max_freq; > - > - if (df->min_freq && adjusted_freq < df->min_freq) > - adjusted_freq = df->min_freq; > - > - *freq = adjusted_freq; > - } else { > + if (data->valid) > + *freq = data->user_frequency; > + else > *freq = df->previous_freq; /* No user freq specified yet */ > - } > + > return 0; > } >
Hi Matthias, On 2018년 08월 03일 08:36, Matthias Kaehlcke wrote: > Hi Chanwoo, > > this patch and "PM / devfreq: Fix handling of min/max_freq == 0" > address issues not directly related with the throttler. It seems it > could still take a while for the throttler to move forward, do you > want me to spin out these two patches so that they can get merged > independently from the rest of the series? How about resend the devfreq patches(patch1/2/3/4/6) which don't depend on throttler core with my reviewed tag? Maybe, it is easy to merge them through Myungjoo. Regards, Chanwoo Choi > > Thanks > > Matthias > > On Tue, Jul 03, 2018 at 04:46:56PM -0700, Matthias Kaehlcke wrote: >> Several governors use the user space limits df->min/max_freq to adjust >> the target frequency. This is not necessary, since update_devfreq() >> already takes care of this. Instead the governor can request the available >> min/max frequency by setting the target frequency to DEVFREQ_MIN/MAX_FREQ >> and let update_devfreq() take care of any adjustments. >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >> Reviewed-by: Brian Norris <briannorris@chromium.org> >> --- >> Changes in v5: >> - none >> >> Changes in v4: >> - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag >> >> Changes in v3: >> - none >> >> Changes in v2: >> - squashed "PM / devfreq: Remove redundant frequency adjustment from governors" >> and "PM / devfreq: governors: Return device frequency limits instead of user >> limits" >> - updated subject and commit message >> - use DEVFREQ_MIN/MAX_FREQ instead of df->scaling_min/max_freq >> --- >> drivers/devfreq/governor.h | 3 +++ >> drivers/devfreq/governor_performance.c | 5 +---- >> drivers/devfreq/governor_powersave.c | 2 +- >> drivers/devfreq/governor_simpleondemand.c | 12 +++--------- >> drivers/devfreq/governor_userspace.c | 16 ++++------------ >> 5 files changed, 12 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h >> index cfc50a61a90d..b81700244ce3 100644 >> --- a/drivers/devfreq/governor.h >> +++ b/drivers/devfreq/governor.h >> @@ -25,6 +25,9 @@ >> #define DEVFREQ_GOV_SUSPEND 0x4 >> #define DEVFREQ_GOV_RESUME 0x5 >> >> +#define DEVFREQ_MIN_FREQ 0 >> +#define DEVFREQ_MAX_FREQ ULONG_MAX >> + >> /** >> * struct devfreq_governor - Devfreq policy governor >> * @node: list node - contains registered devfreq governors >> diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c >> index 4d23ecfbd948..ded429fd51be 100644 >> --- a/drivers/devfreq/governor_performance.c >> +++ b/drivers/devfreq/governor_performance.c >> @@ -20,10 +20,7 @@ static int devfreq_performance_func(struct devfreq *df, >> * target callback should be able to get floor value as >> * said in devfreq.h >> */ >> - if (!df->max_freq) >> - *freq = UINT_MAX; >> - else >> - *freq = df->max_freq; >> + *freq = DEVFREQ_MAX_FREQ; >> return 0; >> } >> >> diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c >> index 0c42f23249ef..9e8897f5ac42 100644 >> --- a/drivers/devfreq/governor_powersave.c >> +++ b/drivers/devfreq/governor_powersave.c >> @@ -20,7 +20,7 @@ static int devfreq_powersave_func(struct devfreq *df, >> * target callback should be able to get ceiling value as >> * said in devfreq.h >> */ >> - *freq = df->min_freq; >> + *freq = DEVFREQ_MIN_FREQ; >> return 0; >> } >> >> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c >> index 28e0f2de7100..c0417f0e081e 100644 >> --- a/drivers/devfreq/governor_simpleondemand.c >> +++ b/drivers/devfreq/governor_simpleondemand.c >> @@ -27,7 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, >> unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; >> unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; >> struct devfreq_simple_ondemand_data *data = df->data; >> - unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; >> >> err = devfreq_update_stats(df); >> if (err) >> @@ -47,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, >> >> /* Assume MAX if it is going to be divided by zero */ >> if (stat->total_time == 0) { >> - *freq = max; >> + *freq = DEVFREQ_MAX_FREQ; >> return 0; >> } >> >> @@ -60,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, >> /* Set MAX if it's busy enough */ >> if (stat->busy_time * 100 > >> stat->total_time * dfso_upthreshold) { >> - *freq = max; >> + *freq = DEVFREQ_MAX_FREQ; >> return 0; >> } >> >> /* Set MAX if we do not know the initial frequency */ >> if (stat->current_frequency == 0) { >> - *freq = max; >> + *freq = DEVFREQ_MAX_FREQ; >> return 0; >> } >> >> @@ -85,11 +84,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, >> b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); >> *freq = (unsigned long) b; >> >> - if (df->min_freq && *freq < df->min_freq) >> - *freq = df->min_freq; >> - if (df->max_freq && *freq > df->max_freq) >> - *freq = df->max_freq; >> - >> return 0; >> } >> >> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c >> index 080607c3f34d..378d84c011df 100644 >> --- a/drivers/devfreq/governor_userspace.c >> +++ b/drivers/devfreq/governor_userspace.c >> @@ -26,19 +26,11 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) >> { >> struct userspace_data *data = df->data; >> >> - if (data->valid) { >> - unsigned long adjusted_freq = data->user_frequency; >> - >> - if (df->max_freq && adjusted_freq > df->max_freq) >> - adjusted_freq = df->max_freq; >> - >> - if (df->min_freq && adjusted_freq < df->min_freq) >> - adjusted_freq = df->min_freq; >> - >> - *freq = adjusted_freq; >> - } else { >> + if (data->valid) >> + *freq = data->user_frequency; >> + else >> *freq = df->previous_freq; /* No user freq specified yet */ >> - } >> + >> return 0; >> } >> > >
On Fri, Aug 03, 2018 at 09:03:30AM +0900, Chanwoo Choi wrote: > Hi Matthias, > > On 2018년 08월 03일 08:36, Matthias Kaehlcke wrote: > > Hi Chanwoo, > > > > this patch and "PM / devfreq: Fix handling of min/max_freq == 0" > > address issues not directly related with the throttler. It seems it > > could still take a while for the throttler to move forward, do you > > want me to spin out these two patches so that they can get merged > > independently from the rest of the series? > > How about resend the devfreq patches(patch1/2/3/4/6) which don't depend on > throttler core with my reviewed tag? Maybe, it is easy to merge them through Myungjoo. Sure, I can do this if you think it is reasonable to merge all these patches without the throttler. These are the patches we are talking about and my interpretation of their status: [01] PM / devfreq: Init user limits from OPP limits, not viceversa landed in Rafaels tree [02] PM / devfreq: Fix handling of min/max_freq == 0 independent fix, can land [03] PM / devfreq: Don't adjust to user limits in governors independent improvement, can land [04] PM / devfreq: Add struct devfreq_policy edge case, can land if devfreq maintainers think that factoring out some fields to the policy struct is an improvement independently of the throttler [05] PM / devfreq: Add support for policy notifiers under heavy discussion ;-), can't land [06] PM / devfreq: Make update_devfreq() public has no user without the throttler, not sure if it should be merged without it. up to devfreq maintainers. Please let me know what you think Thanks Matthias
Hi Matthias, On 2018년 08월 03일 09:24, Matthias Kaehlcke wrote: > On Fri, Aug 03, 2018 at 09:03:30AM +0900, Chanwoo Choi wrote: >> Hi Matthias, >> >> On 2018년 08월 03일 08:36, Matthias Kaehlcke wrote: >>> Hi Chanwoo, >>> >>> this patch and "PM / devfreq: Fix handling of min/max_freq == 0" >>> address issues not directly related with the throttler. It seems it >>> could still take a while for the throttler to move forward, do you >>> want me to spin out these two patches so that they can get merged >>> independently from the rest of the series? >> >> How about resend the devfreq patches(patch1/2/3/4/6) which don't depend on >> throttler core with my reviewed tag? Maybe, it is easy to merge them through Myungjoo. > > Sure, I can do this if you think it is reasonable to merge all these > patches without the throttler. IMO, patch1/2/3/6 looks good. I replied with my reviewed-tag for them. patch4 defines the 'struct devfreq_policy' and then patch5 send notification with 'struct devfreq_policy' on original patch. But, when we discussed it on patch5, new devfreq notification send 'struct devfreq_freq_limits' better than 'struct devfreq_policy'. So, patch4 would be required with more discussion. If myungjoo agree the current patch4, I'm okay. > > These are the patches we are talking about and my interpretation of > their status: > > [01] PM / devfreq: Init user limits from OPP limits, not viceversa > landed in Rafaels tree > > [02] PM / devfreq: Fix handling of min/max_freq == 0 > independent fix, can land > > [03] PM / devfreq: Don't adjust to user limits in governors > independent improvement, can land > > [04] PM / devfreq: Add struct devfreq_policy > edge case, can land if devfreq maintainers think that factoring out > some fields to the policy struct is an improvement independently of > the throttler > > [05] PM / devfreq: Add support for policy notifiers > under heavy discussion ;-), can't land > > [06] PM / devfreq: Make update_devfreq() public > has no user without the throttler, not sure if it should be merged > without it. up to devfreq maintainers. > > Please let me know what you think
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index cfc50a61a90d..b81700244ce3 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -25,6 +25,9 @@ #define DEVFREQ_GOV_SUSPEND 0x4 #define DEVFREQ_GOV_RESUME 0x5 +#define DEVFREQ_MIN_FREQ 0 +#define DEVFREQ_MAX_FREQ ULONG_MAX + /** * struct devfreq_governor - Devfreq policy governor * @node: list node - contains registered devfreq governors diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c index 4d23ecfbd948..ded429fd51be 100644 --- a/drivers/devfreq/governor_performance.c +++ b/drivers/devfreq/governor_performance.c @@ -20,10 +20,7 @@ static int devfreq_performance_func(struct devfreq *df, * target callback should be able to get floor value as * said in devfreq.h */ - if (!df->max_freq) - *freq = UINT_MAX; - else - *freq = df->max_freq; + *freq = DEVFREQ_MAX_FREQ; return 0; } diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c index 0c42f23249ef..9e8897f5ac42 100644 --- a/drivers/devfreq/governor_powersave.c +++ b/drivers/devfreq/governor_powersave.c @@ -20,7 +20,7 @@ static int devfreq_powersave_func(struct devfreq *df, * target callback should be able to get ceiling value as * said in devfreq.h */ - *freq = df->min_freq; + *freq = DEVFREQ_MIN_FREQ; return 0; } diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c index 28e0f2de7100..c0417f0e081e 100644 --- a/drivers/devfreq/governor_simpleondemand.c +++ b/drivers/devfreq/governor_simpleondemand.c @@ -27,7 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; struct devfreq_simple_ondemand_data *data = df->data; - unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; err = devfreq_update_stats(df); if (err) @@ -47,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, /* Assume MAX if it is going to be divided by zero */ if (stat->total_time == 0) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } @@ -60,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, /* Set MAX if it's busy enough */ if (stat->busy_time * 100 > stat->total_time * dfso_upthreshold) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } /* Set MAX if we do not know the initial frequency */ if (stat->current_frequency == 0) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } @@ -85,11 +84,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); *freq = (unsigned long) b; - if (df->min_freq && *freq < df->min_freq) - *freq = df->min_freq; - if (df->max_freq && *freq > df->max_freq) - *freq = df->max_freq; - return 0; } diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c index 080607c3f34d..378d84c011df 100644 --- a/drivers/devfreq/governor_userspace.c +++ b/drivers/devfreq/governor_userspace.c @@ -26,19 +26,11 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) { struct userspace_data *data = df->data; - if (data->valid) { - unsigned long adjusted_freq = data->user_frequency; - - if (df->max_freq && adjusted_freq > df->max_freq) - adjusted_freq = df->max_freq; - - if (df->min_freq && adjusted_freq < df->min_freq) - adjusted_freq = df->min_freq; - - *freq = adjusted_freq; - } else { + if (data->valid) + *freq = data->user_frequency; + else *freq = df->previous_freq; /* No user freq specified yet */ - } + return 0; }