LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Mike Brady <mikebrady@eircom.net>
Cc: Takashi Iwai <tiwai@suse.de>,
	Stefan Wahren <stefan.wahren@i2se.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Eric Anholt <eric@anholt.net>,
	linux-rpi-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Phil Elwell <phil@raspberrypi.org>
Subject: Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]
Date: Tue, 09 Oct 2018 17:32:08 +0200
Message-ID: <s5h1s8zyw3r.wl-tiwai@suse.de> (raw)
In-Reply-To: <425F6E5F-782E-4288-ABF0-180504FD7B01@eircom.net>

On Tue, 09 Oct 2018 17:28:36 +0200,
Mike Brady wrote:
> 
> Hi Takashi.
> 
> > On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Tue, 09 Oct 2018 15:18:15 +0200,
> > Mike Brady wrote:
> >> 
> >>>> @Mike: Do you want to write a patch series which upstream "interpolate
> >>>> audio delay" and addresses Takashi's comments?
> >>>> 
> >>>> I would help you, in case you have questions about setup a Raspberry Pi
> >>>> with Mainline kernel or patch submission.
> >>> 
> >>> Well, the question is who really wants this.  The value given by that
> >>> patch is nothing but some estimation and might be even incorrect.
> >>> 
> >>> PulseAudio won't need it any longer when you set the BATCH flag.
> >>> Then it'll switch from tsched mode to the old mode, and the delay
> >>> value would be almost irrelevant.
> >> 
> >> Well, two answers. First, Shairport Sync
> >> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
> >> packet of audio frames is about to be added to the output queue (at
> >> approximately 7.9 millisecond intervals), the delay is checked to
> >> try to maintain sync to within a few milliseconds. The BCM2835 audio
> >> device is the only one I have yet come across with so much
> >> jitter. Whatever other drivers do, the delay they report doesn’t
> >> suffer from anything like this level of jitter.
> > 
> > OK, if there is another application using that delay value, it's worth
> > to consider providing a fine-grained value.
> > 
> >> The second answer is that the veracity of the ALSA documentation
> >> depends on it — any application using the ALSA system for
> >> synchronisation will rely on this being an accurate reflection of
> >> the situation. AFAIK there is really no workaround it if the
> >> application is confined to “safe” ALSA
> >> (http://0pointer.de/blog/projects/guide-to-sound-apis).
> > 
> >> On LMKL.org, Takashi wrote:
> >> 
> >>> Date	Wed, 19 Sep 2018 11:52:33 +0200
> >>> From	Takashi Iwai <>
> >>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
> >> 
> >>> [snip]
> >> 
> >>> That's OK, as long as the computation is accurate enough (at least not
> >>> exceed the actual position) and is light-weight.
> >> 
> >>> [snip]
> >> 
> >> The overhead is small -- an extra ktime_get() every time a GPU message
> >> is sent -- and another call and a few calculations whenever the delay
> >> is sought from userland.
> >> 
> >> At 48,000 frames per second, i.e. approximately 20 microseconds per
> >> frame, it would take a clock inaccuracy of roughly
> >> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
> >> result in an inaccurate estimate. 
> >> Crystal or resonator-based clocks typically have an inaccuracy of
> >> 10s to 100s of parts per million.
> >> 
> >> Finally, to see the effect of the absence and presence of this
> >> interpolation, please have a look at this:
> >> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
> >> where a downstream version of this fix was being discussed.
> > 
> > I'm not opposing to the usage of delay value.  The attribute is
> > provided exactly for such a purpose.  It's a good thing (tm).
> > 
> > The potential problem is, however, rather the implementation: it's
> > using a system timer for interpolation, which is known to drift from
> > the actual clocks.  Though, one may say that in such a use case, we
> > may ignore the drift since the interpolation is so narrow.
> 
> Yes, that was my thought. I guess another thing in its favour is that this audio device will always
> be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
> accurate clock.
> 
> > But another question is whether it should be implemented in each
> > driver level.  The time-stamping is basically a PCM core
> > functionality, and nothing specific to the hardware, especially when
> > it's referring to the system timer.
> 
> That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
> the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.
> 
> > e.g. you can think in a different way, too: we may put a timestamp at
> > each hwptr update, and pass it as-is, instead of updating the
> > timestamp at each position query.  This will effectively gives the
> > accurate position-timestamp pair, and user-space may interpolate as it
> > likes, too.
> 
> That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.
> 
> > In anyway, if *this* kind of feature needs to be merged, it's
> > definitely to be discussed with the upstream.  So, if you're going to
> > merge that sort of path, please keep Cc to alsa-devel ML.
> 
> In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)

I find it's not too bad, at least as a starting point.
It doesn't break anything dramatically from the existing API POV.

We may brush up things (or extend the API) later, and may move it up
to the core code.  But, again, as a kick off, it's good to start with
a local minimal change.


thanks,

Takashi

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-04 15:58 [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes Takashi Iwai
2018-09-04 15:58 ` [PATCH 01/29] staging: bcm2835-audio: Clean up mutex locks Takashi Iwai
2018-09-04 15:58 ` [PATCH 02/29] staging: bcm2835-audio: Remove redundant spdif stream ctls Takashi Iwai
2018-09-04 15:58 ` [PATCH 03/29] staging: bcm2835-audio: Clean up include files in bcm2835-ctl.c Takashi Iwai
2018-09-08 13:25   ` Stefan Wahren
2018-09-08 16:21     ` Takashi Iwai
2018-09-04 15:58 ` [PATCH 04/29] staging: bcm2835-audio: Remove redundant substream mask checks Takashi Iwai
2018-09-04 15:58 ` [PATCH 05/29] staging: bcm2835-audio: Fix mute controls, volume handling cleanup Takashi Iwai
2018-09-04 15:58 ` [PATCH 06/29] staging: bcm2835-audio: Remove redundant function calls Takashi Iwai
2018-09-04 15:58 ` [PATCH 07/29] staging: bcm2835-audio: Remove superfluous open flag Takashi Iwai
2018-09-04 15:58 ` [PATCH 08/29] staging: bcm2835-audio: Drop useless running flag and check Takashi Iwai
2018-09-04 15:58 ` [PATCH 09/29] staging: bcm2835-audio: Fix incorrect draining handling Takashi Iwai
2018-09-04 15:58 ` [PATCH 10/29] staging: bcm2835-audio: Kill unused spinlock Takashi Iwai
2018-09-04 15:58 ` [PATCH 11/29] staging: bcm2835-audio: Use PCM runtime values instead Takashi Iwai
2018-09-04 15:58 ` [PATCH 12/29] staging: bcm2835-audio: Drop unnecessary pcm indirect setup Takashi Iwai
2018-09-04 15:58 ` [PATCH 13/29] staging: bcm2835-audio: Drop useless NULL check Takashi Iwai
2018-09-04 15:58 ` [PATCH 14/29] staging: bcm2835-audio: Propagate parameter setup error Takashi Iwai
2018-09-04 15:58 ` [PATCH 15/29] staging: bcm2835-audio: Drop debug messages in bcm2835-pcm.c Takashi Iwai
2018-09-04 15:58 ` [PATCH 16/29] staging: bcm2835-audio: Drop superfluous mutex lock during prepare Takashi Iwai
2018-09-08 13:40   ` Stefan Wahren
2018-09-08 16:12     ` Takashi Iwai
2018-09-04 15:58 ` [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint Takashi Iwai
2018-09-19  9:42   ` Stefan Wahren
2018-09-19  9:52     ` Takashi Iwai
2018-09-19 12:41       ` Stefan Wahren
2018-09-19 12:47         ` Mike Brady
2018-09-19 18:39         ` Takashi Iwai
2018-10-09 13:18           ` [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...] Mike Brady
2018-10-09 13:44             ` Takashi Iwai
2018-10-09 15:28               ` Mike Brady
2018-10-09 15:32                 ` Takashi Iwai [this message]
2018-10-11 12:53                 ` Mike Brady
2018-10-11 14:07                   ` Stefan Wahren
2018-10-13 15:00                   ` Mike Brady
2018-10-13 15:45                     ` Takashi Iwai
2018-09-04 15:58 ` [PATCH 18/29] staging: bcm2835-audio: Make single vchi handle Takashi Iwai
2018-09-04 15:58 ` [PATCH 19/29] staging: bcm2835-audio: Code refactoring of vchiq accessor codes Takashi Iwai
2018-09-04 15:58 ` [PATCH 20/29] staging: bcm2835-audio: Operate non-atomic PCM ops Takashi Iwai
2018-09-04 15:58 ` [PATCH 21/29] staging: bcm2835-audio: Use card->private_data Takashi Iwai
2018-09-04 15:58 ` [PATCH 22/29] staging: bcm2835-audio: Use standard error print helpers Takashi Iwai
2018-09-04 15:58 ` [PATCH 23/29] staging: bcm2835-audio: Remove unnecessary header file includes Takashi Iwai
2018-09-04 15:58 ` [PATCH 24/29] staging: bcm2835-audio: Move module parameter description Takashi Iwai
2018-09-04 15:58 ` [PATCH 25/29] staging: bcm2835-audio: Use coherent device buffers Takashi Iwai
2018-09-04 15:58 ` [PATCH 26/29] staging: bcm2835-audio: Set SNDRV_PCM_INFO_SYNC_APPLPTR Takashi Iwai
2018-09-04 15:58 ` [PATCH 27/29] staging: bcm2835-audio: Simplify PCM creation helpers Takashi Iwai
2018-09-04 15:58 ` [PATCH 28/29] staging: bcm2835-audio: Simplify kctl " Takashi Iwai
2018-09-04 15:58 ` [PATCH 29/29] staging: bcm2835-audio: Simplify card object management Takashi Iwai
2018-09-08 13:18 ` [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes Stefan Wahren
2018-09-08 16:21   ` Takashi Iwai
2018-09-08 17:00     ` Stefan Wahren
2018-09-08 17:16       ` Takashi Iwai
2018-09-10  9:12     ` Greg Kroah-Hartman
2018-09-10  9:16       ` Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5h1s8zyw3r.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=eric@anholt.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=mikebrady@eircom.net \
    --cc=phil@raspberrypi.org \
    --cc=stefan.wahren@i2se.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git