| Message ID | 1484135376-482-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-01-11 at 12:49 +0100, Nicholas Mc Guire wrote: > ulseep_range() uses hrtimers and provides no advantage over msleep() > for larger delays. Fix up the 75/85ms delays here to use msleep() and > reduce the load on the hrtimer subsystem. If this is useful, why not convert all the large value usleep_range uses in sound/soc/ ?
On Wed, Jan 11, 2017 at 03:55:49AM -0800, Joe Perches wrote: > On Wed, 2017-01-11 at 12:49 +0100, Nicholas Mc Guire wrote: > > ulseep_range() uses hrtimers and provides no advantage over msleep() > > for larger delays. Fix up the 75/85ms delays here to use msleep() and > > reduce the load on the hrtimer subsystem. > > If this is useful, why not convert all the large value > usleep_range uses in sound/soc/ ? > thats what Im currently doing - just that it cant be done fully automatic as it may be justified in some cases - so it needs a case-by-case review. And its not only in sound/soc/ its quite a bit in drivers/ as well. thx! hofrat
On Wed, Jan 11, 2017 at 12:49:36PM +0100, Nicholas Mc Guire wrote: > if (!rt5651->hp_mute) > - usleep_range(80000, 85000); > + msleep(85); If you're doing conversions like this I'd expect us to be picking the lower number rather than the higher number - people are saying "wait for at least X and at most Y" and msleep() is "wait for at least X" so we should be picking X.
On Wed, Jan 11, 2017 at 02:59:26PM +0000, Mark Brown wrote: > On Wed, Jan 11, 2017 at 12:49:36PM +0100, Nicholas Mc Guire wrote: > > > if (!rt5651->hp_mute) > > - usleep_range(80000, 85000); > > + msleep(85); > > If you're doing conversions like this I'd expect us to be picking the > lower number rather than the higher number - people are saying "wait for > at least X and at most Y" and msleep() is "wait for at least X" so we > should be picking X. useleep_range() sets the timer to max and only if there happens to be a timer between min and max uses that - so the mean of runs is generally a bit above max. E.g. usleep_range(*,200) 5000 samples - idle system 100,200 190,200 Min. :188481 Min. :197793 1st Qu.:207062 1st Qu.:207051 Median :207139 Median :207133 Mean :207254 Mean :207244 3rd Qu.:207341 3rd Qu.:207610 Max. :225340 Max. :214885 So to keep the behavior as close to the current code as possible it seemed better to select the upper value. Am I missing something ? thx! hofrat
On Wed, Jan 11, 2017 at 03:06:45PM +0000, Nicholas Mc Guire wrote: > On Wed, Jan 11, 2017 at 02:59:26PM +0000, Mark Brown wrote: > > If you're doing conversions like this I'd expect us to be picking the > > lower number rather than the higher number - people are saying "wait for > > at least X and at most Y" and msleep() is "wait for at least X" so we > > should be picking X. > useleep_range() sets the timer to max and only if there happens to be a > timer between min and max uses that - so the mean of runs is generally > a bit above max. E.g. Yes, but as fairly recently discussed somewhere on the lists (and IIRC actually fixed) approximately no users expect or want that behaviour - it's a really confusing interface given that sleep functions almost always have a "delay up until X" interface and interfaces that can wake things up earlier than the expected delay generally flag that condition. The applications for the "delay for X but it's OK to wake me up this much earlier" are really quite limited. If you look at the conversions that were done to usleep_range() you'll notice that most of them follow this pattern and had their delays extended in the process.
On Wed, Jan 11, 2017 at 06:06:58PM +0000, Mark Brown wrote: > On Wed, Jan 11, 2017 at 03:06:45PM +0000, Nicholas Mc Guire wrote: > > On Wed, Jan 11, 2017 at 02:59:26PM +0000, Mark Brown wrote: > > > > If you're doing conversions like this I'd expect us to be picking the > > > lower number rather than the higher number - people are saying "wait for > > > at least X and at most Y" and msleep() is "wait for at least X" so we > > > should be picking X. > > > useleep_range() sets the timer to max and only if there happens to be a > > timer between min and max uses that - so the mean of runs is generally > > a bit above max. E.g. > > Yes, but as fairly recently discussed somewhere on the lists (and IIRC > actually fixed) approximately no users expect or want that behaviour - > it's a really confusing interface given that sleep functions almost > always have a "delay up until X" interface and interfaces that can wake > things up earlier than the expected delay generally flag that condition. > The applications for the "delay for X but it's OK to wake me up this > much earlier" are really quite limited. If you look at the conversions > that were done to usleep_range() you'll notice that most of them follow > this pattern and had their delays extended in the process. True its an odd behavior - the point just was to change the actual behavior as little from current state as one might expect. Anywa - will fix it up then and resend - in this particular case it really makes little difference - assuming that both the minimum and maximum value were suitable to ensure that the writes had compled or it was actually a failure. thx! hofrat
On Wed, Jan 11, 2017 at 06:59:52PM +0000, Nicholas Mc Guire wrote: > On Wed, Jan 11, 2017 at 06:06:58PM +0000, Mark Brown wrote: > > Yes, but as fairly recently discussed somewhere on the lists (and IIRC > > actually fixed) approximately no users expect or want that behaviour - > True its an odd behavior - the point just was to change the actual behavior > as little from current state as one might expect. Anywa - will fix it up Unfortunately the current state will often reflect a change from the original or at least expected behaviour. > then and resend - in this particular case it really makes little Thanks. > difference - assuming that both the minimum and maximum value were > suitable to ensure that the writes had compled or it was actually a failure. It can create confusion where the delay comes from a datasheet or tech note and people are left wondering why a different value is used by the kernel, and in a lot of the paths with delays in them are pretty sensitive to the overall time to run so there's a strong desire to keep the delays as low as possible.
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index f5d3415..fb592b0 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -802,7 +802,7 @@ static int rt5651_hp_event(struct snd_soc_dapm_widget *w, case SND_SOC_DAPM_PRE_PMD: rt5651->hp_mute = 1; - usleep_range(70000, 75000); + msleep(75); break; default: @@ -822,7 +822,7 @@ static int rt5651_hp_post_event(struct snd_soc_dapm_widget *w, switch (event) { case SND_SOC_DAPM_POST_PMU: if (!rt5651->hp_mute) - usleep_range(80000, 85000); + msleep(85); break;
ulseep_range() uses hrtimers and provides no advantage over msleep() for larger delays. Fix up the 75/85ms delays here to use msleep() and reduce the load on the hrtimer subsystem. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- As these are non-atomic regions and a delay of 75/85 ms implies that there is almost certainty multiple context switches occurring in the sleep time and thus relatively high jitter must be assumed with respect to actual wakeup time implying that there is little point in using high-resolution timers here. Patch was compile tested with: x86_64_defconfig + SND_SOC=m SND_SOC_INTEL_BYTCR_RT5651_MACH=m (implies CONFIG_SND_SOC_RT5651) Patch is aginast 4.10-rc3 (localversion-next is next-20170111) sound/soc/codecs/rt5651.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)