[PATCH] ASoC: rt5651: use msleep for large delays

[PATCH] ASoC: rt5651: use msleep for large delays

[Nicholas Mc Guire]

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(-)

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;
 
-- 
2.1.4

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Joe Perches]

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/ ?

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Nicholas Mc Guire]

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 

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Nicholas Mc Guire]

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

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Nicholas Mc Guire]

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

Re: [PATCH] ASoC: rt5651: use msleep for large delays

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]