November 2012 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] Bug in alsa-lib causes high CPU with rate plugin
by Trent Piepho 10 Nov '12

10 Nov '12

There is a bug in the way alsa-lib sleeps when using a plugin that can cause very high CPU usage for PCM playback with using rate conversion. I've attached a simple test program. It simply uses blocking mode and calls snd_pcm_writei() with a buffer of zeros one thousand times and then exits. One would think the CPU usage would be very low. And indeed when the write sized passed to snd_pcm_writei is equal to the period size it is quite low (i5 with HDA): lucid@Dashboard:~$ /usr/bin/time ./a.out > /dev/null period size 352, write size 352 done, wrote 1000 buffers of 352 frames 0.10user 0.15system 0:09.01elapsed 2%CPU (0avgtext+0avgdata 9360maxresident)k But, with one small change to have snd_pcm_writei() write buffers one frame larger than the period size, one gets this: period size 352, write size 353 done, wrote 1000 buffers of 353 frames 1.92user 2.00system 0:09.04elapsed 43%CPU (0avgtext+0avgdata 9360maxresident)k Simply increasing the write size by one caused the CPU usage to jump to 43% from 2%! Because more software writes in chunks equal to the period size it doesn't see this problem. That's why it wasn't found and fixed long ago. I've figured out why this is happening. The problem is caused by how alsa-lib decides to sleep when waiting for available buffer space and the way the rate plugin can only process complete periods at a time. Suppose we have the rate plugin with hw as its slave. Both have a ring buffer that is 3+ periods in size. Allowing for the rate conversion, the buffers and periods are the same size for both. avail_min is set to the normal value of one period for both. Suppose there are two empty periods in the buffer and we call snd_pcm_writei() to write 1.5 periods. That fits in the empty space in the rate plugin's buffer and so gets written immediately. The rate plugin's buffer now has 0.5 empty periods of space. The rate plugin will resample just one period of the newly written data, because it can only process complete periods at a time, and write that to the hw buffer. So the hw buffer now has one free period. This means the rate plugin has less free space in its buffer than the hw has, because the rate plugin has half a period of data it can't yet resample and pass to the slave. Now we write another one period of data with another snd_pcm_writei() call. This gets to snd_pcm_write_areas() in pcm.c and triggers this code (edited for brevity): if ((state == SND_PCM_STATE_RUNNING && (snd_pcm_uframes_t)avail < pcm->avail_min && size > (snd_pcm_uframes_t)avail)) { err = snd_pcm_wait(pcm, -1); goto _again; } avail is the available space in the rate plugin's buffer, which is half a period. pcm->avail_min is also for the rate plugin. size is the amount remaining in the writei() call, which is one period in this example. As we can see, the conditions of the if() statement are true. avail of 0.5 periods is less than avail_min of 1 period and the size of the write (1 period) is greater than avail. So we call snd_pcm_wait() and then go back to the top when it returns. avail will get updated to the current value and this code runs again. Eventually snd_pcm_wait() will call poll() and try to sleep in the kernel while waiting for more room to be available. But remember the hw buffer had one full period free! That's more than tghe half period that's free in the rate buffer and, and this is the key here, it is *more than avail_min*. So poll() will not sleep at all! That's where the 43% CPU comes from. alsa-lib is in a tight loop inside snd_pcm_write_areas() calling snd_pcm_wait() over and over again. snd_pcm_wait() doesn't sleep, because it's based on the status of the hw buffer and that buffer does have free space. snd_pcm_write_areas() would write to the hw buffer and not try to sleep. But it's not looking at the hw buffer when it decides to sleep or write, it's looking at the rate buffer. And the rate buffer's fill level says to sleep, not write. And if snd_pcm_wait() was looking at the rate buffer, it would sleep. We have the rate buffer that's current fill level says, "sleep first, then write" and the HW buffer that says the opposite, "write first, then sleep". The code that writes looks at the former and decides to sleep but the code that sleeps looks at the latter and decides it's not time sleep. A trivial patch that stops this behavior is below, which simply forces the rate plugin's avail_min to 1. diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 41089d7..86f57c4 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -1142,6 +1142,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm) return 0; } rate->start_pending = 0; + pcm->avail_min = 1; return snd_pcm_start(rate->gen.slave); } I think it would be better to have the rate plugin decrease avail_min by how much unresampled data it has yet to pass to the slave. This preserves the idea of avail_min if it were set to more than one period. This also fixes the problem. I've attached a real patch to do that. For the test program to show the problem YOU MUST USE THE RATE CONVERT PLUGIN! It seems obvious but people keep missing this. I found the easiest way to do that was to create a custom device in ~/.asoundrc that forced rate converting to 48kHz. Like this: pcm.foo { type plug; slave { pcm "hw:0,0"; rate 48000; } }

2 3

[alsa-devel] [PATCH] ASoC: samsung - Don't setup i2s if already active.
by Dylan Reid 09 Nov '12

09 Nov '12

If the dai is already running when setup is called, skip setup. Setting up again caused set_sysclk to set rclk_srcrate to other->rclk_srcrate, other would often be set to the default of 44.1. Playing a 48k stream then adding a 48k capture stream would lead to both streams set to 44.1 (The value of other->rclk_srcrate). Similarly in shutdown, if either playback or capture is still active, return instead of turning off the clocks. Signed-off-by: Dylan Reid <dgreid(a)chromium.org> --- sound/soc/samsung/i2s.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index 40b00a1..b9935dd 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -637,6 +637,10 @@ static int i2s_startup(struct snd_pcm_substream *substream, struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai; unsigned long flags; + /* Check not already running. */ + if (dai->playback_active || dai->capture_active) + return 0; + spin_lock_irqsave(&lock, flags); i2s->mode |= DAI_OPENED; @@ -661,6 +665,10 @@ static void i2s_shutdown(struct snd_pcm_substream *substream, struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai; unsigned long flags; + /* Check not still running. */ + if (dai->playback_active || dai->capture_active) + return; + spin_lock_irqsave(&lock, flags); i2s->mode &= ~DAI_OPENED; -- 1.7.12.146.g16d26b1

2 2

Re: [alsa-devel] [PATCH 1/6] spi: imx: specify spi base for device tree probe
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:00:20PM +0100, Steffen Trumtrar wrote: > * This breaks compatibility with user space applications between non-DT and DT. > * On platforms that are in between DT and non-DT, it breaks registration of > devices via spi_board_info. > Use the same method as 7e6086d9e54a159a6257c02bb7fc5805c614aad2 does for gpios: > Use alias to identify the spi port, and then specify the base via the port id. > If alias is not defined in DT, allocate the base dynamically. This sounds like a fix userspace and/or complete your DT conversion problem...

1 0

[alsa-devel] [PATCH] ASoC: dapm: Use card_list during DAPM shutdown
by Misael Lopez Cruz 09 Nov '12

09 Nov '12

DAPM shutdown incorrectly uses "list" field of codec struct while iterating over probed components (codec_dev_list). "list" field refers to codecs registered in the system, "card_list" field is used for probed components. Signed-off-by: Misael Lopez Cruz <misael.lopez(a)ti.com> Signed-off-by: Liam Girdwood <lrg(a)ti.com> --- sound/soc/soc-dapm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index d0a4be3..6e35bca 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3745,7 +3745,7 @@ void snd_soc_dapm_shutdown(struct snd_soc_card *card) { struct snd_soc_codec *codec; - list_for_each_entry(codec, &card->codec_dev_list, list) { + list_for_each_entry(codec, &card->codec_dev_list, card_list) { soc_dapm_shutdown_codec(&codec->dapm); if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) snd_soc_dapm_set_bias_level(&codec->dapm, -- 1.7.4.1

3 3

Re: [alsa-devel] [PATCH 1/1] ASOC: core: Double control update err for snd_soc_put_volsw_sx
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 11:53:40AM +0530, y(a)dbdp33.itg.ti.com wrote: > From: Mukund Navada <navada(a)ti.com> > > snd_soc_put_volsw_sx function fails to update second control > if first control is updated by snd_soc_update_bits_locked. Applied, thanks.

1 0

Re: [alsa-devel] [PATCH 6/6] ASoC: fsl: add imx-wm8974 machine driver
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:00:25PM +0100, Steffen Trumtrar wrote: > +static int imx_wm8974_dai_init(struct snd_soc_pcm_runtime *rtd) > +{ > + struct imx_wm8974_data *data = container_of(rtd->card, > + struct imx_wm8974_data, card); > + struct device *dev = rtd->card->dev; > + int ret; > + > + /* the pll stability peaks at N=8 and around 90MHz. > + * This values are best reached with a 12.288MHz or > + * 11.289MHz clock. As the first is closer to N=8 in > + * more situations, chose 12.288MHz as the target clock > + * (ref: datasheet section "Master Clock and Phase Locked Loop")*/ > + ret = snd_soc_dai_set_pll(rtd->codec_dai, 0, 0, data->clk_frequency, > + 12288000); This seems far from ideal - it's starting the PLL at system boot and just leaving it running. This will be power inefficient when the system is not playing audio as the PLL will be consuming power but not used for anything. You could also do the same thing with the MCLK. Note also that a 12.288MHz clock is only suitable for 8kHz based sample rates, for CD audio 44.1kHz based rates like most media files 11.2896MHz should be chosen. The driver may want to switch dynamically at runtime to support the widest possible range of rates. > + data->codec_clk = clk_get(&codec_pdev->dev, NULL); > + if (IS_ERR(data->codec_clk)) { devm_clk_get() - this'd free the clock that's leaked on removal.

1 0

Re: [alsa-devel] [PATCH 4/6] ASoC: wm8974: add SPI as a possible bus master
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:55:04PM +0100, Steffen Trumtrar wrote: > On Fri, Nov 09, 2012 at 02:38:42PM +0000, Mark Brown wrote: > > We should be converting things to regmap if we're going to do this - > > we're trying to phase out the ASoC-specific I/O and this makes the code > > simpler anyway as the core will just be able to get the regmap from the > > struct device without bouncing information around through the private > > data and so on. > I wasn't aware of that driver. That sounds way better than the private data stuff. If you look in mainline there's a bunch of conversions been done recently, it's fairly mechanical so hopefully they'll show you what needs doing.

1 0

Re: [alsa-devel] [PATCH 5/6] ARM i.MX: rename ssi1 clock for imx27
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:00:24PM +0100, Steffen Trumtrar wrote: > Signed-off-by: Steffen Trumtrar <s.trumtrar(a)pengutronix.de> You don't say anything about why you're renaming here... > - clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0"); > + clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "10010000.ssi"); > clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1"); I'm guessing this is for DT... why are we only renaming one of the clocks here and will this not break non-DT users?

1 0

Re: [alsa-devel] [PATCH 3/6] ASoC: wm8974: include MCLKDIV in pll_factors
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:00:22PM +0100, Steffen Trumtrar wrote: > To calculate the integer part of the frequency ratio, the whole output > path has to be considered (post and pre are optional): > Ndiv = (pre * target * 4 * post) / source > In the current implementation only the fixed- and pre-divider is > considered, but the post-divider is omitted. > To calculate Ndiv, this post divider has to be applied before any > calculation happens. Otherwise Ndiv is considered to be to low in the > later stages. This leads to a wrong value in the PLLN register, which > in turn produces a wrong playback speed of the audio signal. This changelog doesn't mention where you're using MCLKDIV here but it does rather sound like the PLL configuration is not being done correctly here. The expectation is that when the PLL is configured the raw output frequency from the PLL is specified. The PLL is then specified as the system clock with that rate and then any division required to make that usable happens afterwards. > + case WM8974_MCLKDIV_1: > + reg = 1; > + break; Please do follow the Linux coding style for kernel code.

1 0

Re: [alsa-devel] [PATCH 4/6] ASoC: wm8974: add SPI as a possible bus master
by Mark Brown 09 Nov '12

09 Nov '12

On Fri, Nov 09, 2012 at 03:00:23PM +0100, Steffen Trumtrar wrote: > The wm8974 can be controlled via i2c or spi. The current driver only supports > i2c. Add SPI as possible bus. > > As the driver has to distinguish the bus type, pass this info via the wm8974_priv. > This reverts c2562a8e3b5f871ad0b73caf98bb7541e8724efc, because the driver now > needs it back. Don't just quote raw commit IDs, *especially* not full ones, as they're completely illegible to humans. Someone reading the changelog needs to be able to understand what this commit is and why the driver needs it back. > - ret = snd_soc_codec_set_cache_io(codec, 7, 9, SND_SOC_I2C); > + ret = snd_soc_codec_set_cache_io(codec, 7, 9, wm8974->control_type); We should be converting things to regmap if we're going to do this - we're trying to phase out the ASoC-specific I/O and this makes the code simpler anyway as the core will just be able to get the regmap from the struct device without bouncing information around through the private data and so on.

1 0