LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Mike Brady <mikebrady@eircom.net>,
	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
Date: Wed, 19 Sep 2018 20:39:46 +0200
Message-ID: <s5hworhs559.wl-tiwai@suse.de> (raw)
In-Reply-To: <8866e22a-6cd7-d32d-92e5-9a4e60206d2f@i2se.com>

On Wed, 19 Sep 2018 14:41:28 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> [add Phil and Mike]
> 
> Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
> > On Wed, 19 Sep 2018 11:42:22 +0200,
> > Stefan Wahren wrote:
> >> Hi Takashi,
> >>
> >> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
> >>> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
> >>> minimal period size is also 10 msec.
> >>>
> >>> This patch adds the corresponding hw constraint.
> >>>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> index 9659c25b9f9d..6d89db6e14e4 100644
> >>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
> >>>  				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> >>>  				   16);
> >>>  
> >>> +	/* position update is in 10ms order */
> >>> +	snd_pcm_hw_constraint_minmax(runtime,
> >>> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
> >>> +				     10 * 1000, UINT_MAX);
> >>> +
> >>>  	chip->alsa_stream[idx] = alsa_stream;
> >>>  
> >>>  	chip->opened |= (1 << idx);
> >> in the Foundation Kernel (Downstream) there is a patch to interpolate
> >> the audio delay. So my questions is, does your patch above makes the
> >> following patch obsolete?
> > Through a quick glance, no, my patch is orthogonal to this.
> >
> > My patch adds a PCM hw constraint so that the period size won't go
> > below 10ms, while the downstream patch provides the additional delay
> > value that is calculated from the system clock.
> 
> thanks for your explanation. So your patch must be reverted with
> implementation of interpolate audio delay.

No, no.
Both can be applied as is.  They have *nothing to do* with each
other.


> >> [PATCH] bcm2835: interpolate audio delay
> >>
> >> It appears the GPU only sends us a message all 10ms to update
> >> the playback progress. Other than this, the playback position
> >> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
> >> Userspace will see jitter up to 10ms in the audio position.
> >>
> >> Make this a bit nicer for userspace by interpolating the
> >> position using the CPU clock.
> >>
> >> I'm not sure if setting snd_pcm_runtime.delay is the right
> >> approach for this. Or if there is maybe an already existing
> >> mechanism for position interpolation in the ALSA core.
> > That's OK, as long as the computation is accurate enough (at least not
> > exceed the actual position) and is light-weight.
> >
> >> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
> >> at least one situation snd_pcm_runtime.delay is used, so I have
> >> to worry less in which place I have to update this field, or
> >> how it interacts with the rest of ALSA.
> > Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
> > patch.  It has nothing to do with the runtime->delay calculation.
> > (And, this "one situation" is likely called PulseAudio :)
> >
> >> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
> >> One problem is that it requires sending a videocore message, and
> >> waiting for a reply, which could make the implementation much
> >> harder due to locking and synchronization requirements.
> > This can be now easy with my patch series.  By switching to non-atomic
> > operation, we can issue the vc04 command inside the pointer callback,
> > too.
> 
> I think we should try to implement this later.
> 
> @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.


Takashi

  parent 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 [this message]
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
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=s5hworhs559.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