September 2014 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCHv4 1/9] Clean WM8971 through checkpatch
by Xavier Hsu 24 Sep '14

24 Sep '14

This patch improves WM8971. We clean the file through checkpatch. Signed-off-by: Xavier Hsu <xavier.hsu(a)linaro.org> Signed-off-by: Andy Green <andy.green(a)linaro.org> --- Any comments about improving the patch are welcome. Thanks. sound/soc/codecs/wm8971.c | 108 +++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 52 deletions(-) diff --git a/sound/soc/codecs/wm8971.c b/sound/soc/codecs/wm8971.c index 0499cd4..c0f7e9b 100644 --- a/sound/soc/codecs/wm8971.c +++ b/sound/soc/codecs/wm8971.c @@ -31,7 +31,7 @@ #define WM8971_REG_COUNT 43 -static struct workqueue_struct *wm8971_workq = NULL; +static struct workqueue_struct *wm8971_workq; /* codec private data */ struct wm8971_priv { @@ -92,25 +92,28 @@ static const struct reg_default wm8971_reg_defaults[] = { #define wm8971_reset(c) snd_soc_write(c, WM8971_RESET, 0) /* WM8971 Controls */ -static const char *wm8971_bass[] = { "Linear Control", "Adaptive Boost" }; -static const char *wm8971_bass_filter[] = { "130Hz @ 48kHz", - "200Hz @ 48kHz" }; -static const char *wm8971_treble[] = { "8kHz", "4kHz" }; -static const char *wm8971_alc_func[] = { "Off", "Right", "Left", "Stereo" }; -static const char *wm8971_ng_type[] = { "Constant PGA Gain", - "Mute ADC Output" }; -static const char *wm8971_deemp[] = { "None", "32kHz", "44.1kHz", "48kHz" }; -static const char *wm8971_mono_mux[] = {"Stereo", "Mono (Left)", - "Mono (Right)", "Digital Mono"}; -static const char *wm8971_dac_phase[] = { "Non Inverted", "Inverted" }; -static const char *wm8971_lline_mux[] = {"Line", "NC", "NC", "PGA", - "Differential"}; -static const char *wm8971_rline_mux[] = {"Line", "Mic", "NC", "PGA", - "Differential"}; -static const char *wm8971_lpga_sel[] = {"Line", "NC", "NC", "Differential"}; -static const char *wm8971_rpga_sel[] = {"Line", "Mic", "NC", "Differential"}; -static const char *wm8971_adcpol[] = {"Normal", "L Invert", "R Invert", - "L + R Invert"}; +static const char const *wm8971_bass[] = {"Linear Control", "Adaptive Boost"}; +static const char const *wm8971_bass_filter[] = {"130Hz @ 48kHz", + "200Hz @ 48kHz"}; +static const char const *wm8971_treble[] = {"8kHz", "4kHz"}; +static const char const *wm8971_alc_func[] = {"Off", "Right", + "Left", "Stereo"}; +static const char const *wm8971_ng_type[] = {"Constant PGA Gain", + "Mute ADC Output"}; +static const char const *wm8971_deemp[] = {"None", "32kHz", "44.1kHz", "48kHz"}; +static const char const *wm8971_mono_mux[] = {"Stereo", "Mono (Left)", + "Mono (Right)", "Digital Mono"}; +static const char const *wm8971_dac_phase[] = {"Non Inverted", "Inverted"}; +static const char const *wm8971_lline_mux[] = {"Line", "NC", "NC", + "PGA", "Differential"}; +static const char const *wm8971_rline_mux[] = {"Line", "Mic", "NC", + "PGA", "Differential"}; +static const char const *wm8971_lpga_sel[] = {"Line", "NC", "NC", + "Differential"}; +static const char const *wm8971_rpga_sel[] = {"Line", "Mic", "NC", + "Differential"}; +static const char const *wm8971_adcpol[] = {"Normal", "L Invert", + "R Invert", "L + R Invert"}; static const struct soc_enum wm8971_enum[] = { SOC_ENUM_SINGLE(WM8971_BASS, 7, 2, wm8971_bass), /* 0 */ @@ -136,24 +139,24 @@ static const struct snd_kcontrol_new wm8971_snd_controls[] = { SOC_DOUBLE_R("Capture Switch", WM8971_LINVOL, WM8971_RINVOL, 7, 1, 1), SOC_DOUBLE_R("Headphone Playback ZC Switch", WM8971_LOUT1V, - WM8971_ROUT1V, 7, 1, 0), + WM8971_ROUT1V, 7, 1, 0), SOC_DOUBLE_R("Speaker Playback ZC Switch", WM8971_LOUT2V, - WM8971_ROUT2V, 7, 1, 0), + WM8971_ROUT2V, 7, 1, 0), SOC_SINGLE("Mono Playback ZC Switch", WM8971_MOUTV, 7, 1, 0), SOC_DOUBLE_R("PCM Volume", WM8971_LDAC, WM8971_RDAC, 0, 255, 0), SOC_DOUBLE_R("Bypass Left Playback Volume", WM8971_LOUTM1, - WM8971_LOUTM2, 4, 7, 1), + WM8971_LOUTM2, 4, 7, 1), SOC_DOUBLE_R("Bypass Right Playback Volume", WM8971_ROUTM1, - WM8971_ROUTM2, 4, 7, 1), + WM8971_ROUTM2, 4, 7, 1), SOC_DOUBLE_R("Bypass Mono Playback Volume", WM8971_MOUTM1, - WM8971_MOUTM2, 4, 7, 1), + WM8971_MOUTM2, 4, 7, 1), SOC_DOUBLE_R("Headphone Playback Volume", WM8971_LOUT1V, - WM8971_ROUT1V, 0, 127, 0), + WM8971_ROUT1V, 0, 127, 0), SOC_DOUBLE_R("Speaker Playback Volume", WM8971_LOUT2V, - WM8971_ROUT2V, 0, 127, 0), + WM8971_ROUT2V, 0, 127, 0), SOC_ENUM("Bass Boost", wm8971_enum[0]), SOC_ENUM("Bass Filter", wm8971_enum[1]), @@ -238,14 +241,14 @@ SOC_DAPM_ENUM("Route", wm8971_enum[13]); static const struct snd_soc_dapm_widget wm8971_dapm_widgets[] = { SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0, - &wm8971_left_mixer_controls[0], - ARRAY_SIZE(wm8971_left_mixer_controls)), + &wm8971_left_mixer_controls[0], + ARRAY_SIZE(wm8971_left_mixer_controls)), SND_SOC_DAPM_MIXER("Right Mixer", SND_SOC_NOPM, 0, 0, - &wm8971_right_mixer_controls[0], - ARRAY_SIZE(wm8971_right_mixer_controls)), + &wm8971_right_mixer_controls[0], + ARRAY_SIZE(wm8971_right_mixer_controls)), SND_SOC_DAPM_MIXER("Mono Mixer", WM8971_PWR2, 2, 0, - &wm8971_mono_mixer_controls[0], - ARRAY_SIZE(wm8971_mono_mixer_controls)), + &wm8971_mono_mixer_controls[0], + ARRAY_SIZE(wm8971_mono_mixer_controls)), SND_SOC_DAPM_PGA("Right Out 2", WM8971_PWR2, 3, 0, NULL, 0), SND_SOC_DAPM_PGA("Left Out 2", WM8971_PWR2, 4, 0, NULL, 0), @@ -260,18 +263,18 @@ static const struct snd_soc_dapm_widget wm8971_dapm_widgets[] = { SND_SOC_DAPM_ADC("Left ADC", "Left Capture", WM8971_PWR1, 3, 0), SND_SOC_DAPM_MUX("Left PGA Mux", WM8971_PWR1, 5, 0, - &wm8971_left_pga_controls), + &wm8971_left_pga_controls), SND_SOC_DAPM_MUX("Right PGA Mux", WM8971_PWR1, 4, 0, - &wm8971_right_pga_controls), + &wm8971_right_pga_controls), SND_SOC_DAPM_MUX("Left Line Mux", SND_SOC_NOPM, 0, 0, - &wm8971_left_line_controls), + &wm8971_left_line_controls), SND_SOC_DAPM_MUX("Right Line Mux", SND_SOC_NOPM, 0, 0, - &wm8971_right_line_controls), + &wm8971_right_line_controls), SND_SOC_DAPM_MUX("Left ADC Mux", SND_SOC_NOPM, 0, 0, - &wm8971_monomux_controls), + &wm8971_monomux_controls), SND_SOC_DAPM_MUX("Right ADC Mux", SND_SOC_NOPM, 0, 0, - &wm8971_monomux_controls), + &wm8971_monomux_controls), SND_SOC_DAPM_OUTPUT("LOUT1"), SND_SOC_DAPM_OUTPUT("ROUT1"), @@ -431,7 +434,7 @@ static int get_coeff(int mclk, int rate) } static int wm8971_set_dai_sysclk(struct snd_soc_dai *codec_dai, - int clk_id, unsigned int freq, int dir) + int clk_id, unsigned int freq, int dir) { struct snd_soc_codec *codec = codec_dai->codec; struct wm8971_priv *wm8971 = snd_soc_codec_get_drvdata(codec); @@ -449,7 +452,7 @@ static int wm8971_set_dai_sysclk(struct snd_soc_dai *codec_dai, } static int wm8971_set_dai_fmt(struct snd_soc_dai *codec_dai, - unsigned int fmt) + unsigned int fmt) { struct snd_soc_codec *codec = codec_dai->codec; u16 iface = 0; @@ -507,8 +510,8 @@ static int wm8971_set_dai_fmt(struct snd_soc_dai *codec_dai, } static int wm8971_pcm_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_dai *dai) + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) { struct snd_soc_codec *codec = dai->codec; struct wm8971_priv *wm8971 = snd_soc_codec_get_drvdata(codec); @@ -553,7 +556,7 @@ static int wm8971_mute(struct snd_soc_dai *dai, int mute) } static int wm8971_set_bias_level(struct snd_soc_codec *codec, - enum snd_soc_bias_level level) + enum snd_soc_bias_level level) { u16 pwr_reg = snd_soc_read(codec, WM8971_PWR1) & 0xfe3e; @@ -580,11 +583,12 @@ static int wm8971_set_bias_level(struct snd_soc_codec *codec, } #define WM8971_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ - SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_44100 | \ - SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000) + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\ + SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\ + SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000) #define WM8971_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\ - SNDRV_PCM_FMTBIT_S24_LE) + SNDRV_PCM_FMTBIT_S24_LE) static const struct snd_soc_dai_ops wm8971_dai_ops = { .hw_params = wm8971_pcm_hw_params, @@ -616,6 +620,7 @@ static void wm8971_work(struct work_struct *work) container_of(work, struct snd_soc_dapm_context, delayed_work.work); struct snd_soc_codec *codec = dapm->codec; + wm8971_set_bias_level(codec, codec->dapm.bias_level); } @@ -637,7 +642,7 @@ static int wm8971_resume(struct snd_soc_codec *codec) snd_soc_write(codec, WM8971_PWR1, reg | 0x01c0); codec->dapm.bias_level = SND_SOC_BIAS_ON; queue_delayed_work(wm8971_workq, &codec->dapm.delayed_work, - msecs_to_jiffies(1000)); + msecs_to_jiffies(1000)); } return 0; @@ -660,7 +665,7 @@ static int wm8971_probe(struct snd_soc_codec *codec) snd_soc_write(codec, WM8971_PWR1, reg | 0x01c0); codec->dapm.bias_level = SND_SOC_BIAS_STANDBY; queue_delayed_work(wm8971_workq, &codec->dapm.delayed_work, - msecs_to_jiffies(1000)); + msecs_to_jiffies(1000)); /* set the update bits */ snd_soc_update_bits(codec, WM8971_LDAC, 0x0100, 0x0100); @@ -675,7 +680,6 @@ static int wm8971_probe(struct snd_soc_codec *codec) return ret; } - /* power down chip */ static int wm8971_remove(struct snd_soc_codec *codec) { @@ -729,8 +733,8 @@ static int wm8971_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, wm8971); - ret = snd_soc_register_codec(&i2c->dev, - &soc_codec_dev_wm8971, &wm8971_dai, 1); + ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_wm8971, + &wm8971_dai, 1); return ret; } -- 1.7.9.5

2 13

[alsa-devel] [PATCH v4 1/5] ALSA: ctxfi: added reference of snd_card
by Sudip Mukherjee 23 Sep '14

23 Sep '14

added a pointer of snd_card in some of the structures to get a reference of the card from other functions. these references of snd_card will be initialised in the next patch of this series and as of now these snd_card will be used to print the the device information when we convert the pr_* macros to dev_* in a later patch of this series. Signed-off-by: Sudip Mukherjee <sudip(a)vectorindia.org> --- sound/pci/ctxfi/ctamixer.h | 3 +++ sound/pci/ctxfi/ctdaio.h | 2 ++ sound/pci/ctxfi/cthardware.h | 2 ++ sound/pci/ctxfi/ctsrc.h | 3 +++ 4 files changed, 10 insertions(+) diff --git a/sound/pci/ctxfi/ctamixer.h b/sound/pci/ctxfi/ctamixer.h index cc49e5a..ea81b8a 100644 --- a/sound/pci/ctxfi/ctamixer.h +++ b/sound/pci/ctxfi/ctamixer.h @@ -21,6 +21,7 @@ #include "ctresource.h" #include <linux/spinlock.h> +#include <sound/core.h> /* Define the descriptor of a summation node resource */ struct sum { @@ -35,6 +36,7 @@ struct sum_desc { struct sum_mgr { struct rsc_mgr mgr; /* Basic resource manager info */ + struct snd_card *card; /* pointer to this card */ spinlock_t mgr_lock; /* request one sum resource */ @@ -79,6 +81,7 @@ struct amixer_desc { struct amixer_mgr { struct rsc_mgr mgr; /* Basic resource manager info */ + struct snd_card *card; /* pointer to this card */ spinlock_t mgr_lock; /* request one amixer resource */ diff --git a/sound/pci/ctxfi/ctdaio.h b/sound/pci/ctxfi/ctdaio.h index 85ccb6e..19cb285 100644 --- a/sound/pci/ctxfi/ctdaio.h +++ b/sound/pci/ctxfi/ctdaio.h @@ -23,6 +23,7 @@ #include "ctimap.h" #include <linux/spinlock.h> #include <linux/list.h> +#include <sound/core.h> /* Define the descriptor of a daio resource */ enum DAIOTYP { @@ -98,6 +99,7 @@ struct daio_desc { struct daio_mgr { struct rsc_mgr mgr; /* Basic resource manager info */ + struct snd_card *card; /* pointer to this card */ spinlock_t mgr_lock; spinlock_t imap_lock; struct list_head imappers; diff --git a/sound/pci/ctxfi/cthardware.h b/sound/pci/ctxfi/cthardware.h index 5977e9a..940bbd0 100644 --- a/sound/pci/ctxfi/cthardware.h +++ b/sound/pci/ctxfi/cthardware.h @@ -20,6 +20,7 @@ #include <linux/types.h> #include <linux/pci.h> +#include <sound/core.h> enum CHIPTYP { ATC20K1, @@ -184,6 +185,7 @@ struct hw { void *irq_callback_data; struct pci_dev *pci; /* the pci kernel structure of this card */ + struct snd_card *card; /* pointer to this card */ int irq; unsigned long io_base; unsigned long mem_base; diff --git a/sound/pci/ctxfi/ctsrc.h b/sound/pci/ctxfi/ctsrc.h index 259366a..7887cc6 100644 --- a/sound/pci/ctxfi/ctsrc.h +++ b/sound/pci/ctxfi/ctsrc.h @@ -23,6 +23,7 @@ #include "ctimap.h" #include <linux/spinlock.h> #include <linux/list.h> +#include <sound/core.h> #define SRC_STATE_OFF 0x0 #define SRC_STATE_INIT 0x4 @@ -85,6 +86,7 @@ struct src_desc { /* Define src manager object */ struct src_mgr { struct rsc_mgr mgr; /* Basic resource manager info */ + struct snd_card *card; /* pointer to this card */ spinlock_t mgr_lock; /* request src resource */ @@ -123,6 +125,7 @@ struct srcimp_desc { struct srcimp_mgr { struct rsc_mgr mgr; /* Basic resource manager info */ + struct snd_card *card; /* pointer to this card */ spinlock_t mgr_lock; spinlock_t imap_lock; struct list_head imappers; -- 1.8.1.2

2 6

[alsa-devel] [PATCH] ASoC: Fix snd_soc_{new, free}_ac97_codec() locking removal
by Lars-Peter Clausen 23 Sep '14

23 Sep '14

Commit e3f205a72c45 ("ASoC: Remove locking in snd_soc_{new,free}_ac97_codec()") overlooked a unlock on one of the error paths. Fixes: e3f205a72c45 ("ASoC: Remove locking in snd_soc_{new,free}_ac97_codec()") Signed-off-by: Lars-Peter Clausen <lars(a)metafoo.de> --- sound/soc/soc-core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index f7e4ede..5b88fde 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1967,7 +1967,6 @@ int snd_soc_new_ac97_codec(struct snd_soc_codec *codec, if (codec->ac97->bus == NULL) { kfree(codec->ac97); codec->ac97 = NULL; - mutex_unlock(&codec->mutex); return -ENOMEM; } -- 1.8.0

2 1

[alsa-devel] Master Plan on rewinding
by Alexander E. Patrakov 23 Sep '14

23 Sep '14

Hello. (TL;DR: nothing really new except the strawman proposal about threads and the note about interaction of variable sample rate with rewindability) As Takashi Iwai told me that the audio miniconference is for discussion only, and not for presentation of anything, I guess that I need to present the plans and options now. That's why this e-mail. The goal here, besides merely presenting the plan, is to identify points that everyone agrees upon, so that they are not discussed pointlessly at the miniconference. Also, this e-mail serves as a justification for the pending seemingly-destructive work. First, the status quo. If anyone disagrees with the facts below, please complain loudly, before I make any conclusions! 1. PulseAudio does not call snd_pcm_rewindable(), because for some ALSA plugins it crashed. This crash is completely fixed in alsa-lib 1.0.28, but in some cases snd_pcm_rewindable() still returns wrong results. 2. PulseAudio blindly assumes that it can rewind up to hwbuf_frames - (snd_pcm_avail() + rewind_safeguard) frames. The rewind safeguard is needed due to reasons that I don't completely understand, but one of them is imprecise reporting of the hardware pointer, and another one is that the hardware transfers several bytes at a time, and the bytes we need to overwrite may be already cached by the hardware. 3. On the hw plugin, I could demonstrate two other bugs regarding snd_pcm_rewindable(): stale data and bogus negative return values. 4. There are ALSA plugins like "a52" or the "bluetooth" plugin from old version of bluez that return non-zero for snd_pcm_rewindable() and snd_pcm_rewind, but actually don't rewind, and where rewinding is almost impossible to implement (e.g. because this would seemingly involve unsending of already-sent bluetooth packets). See below why "almost", you need to search for "Alternative strawman proposal". 5. PulseAudio contains a workaround for non-rewindability of the a52 plugin that also happens to apply to other ioplug-based plugins. However, this workaround does not match extplug-based plugins (including "dca"), and my patch extending the workaround has been rejected with the argument that this has to be fixed in ALSA. 6. The rate plugin has been made non-rewindable in alsa-lib 1.0.28 because the old implementation was wrong and no simple fix exists. 7. Issues regarding rewindability of PulseAudio internals such as virtual sinks will be discussed separately. And now the topics for discussion. My immediate plan is to fix (3) and, if anyone replies to the rewind safeguard proposal, maybe start fixing (2) from the alsa-lib side. For all proposals below, unless I say otherwise, I intend to write the required alsa-lib code myself if the proposal is accepted. === On the rewind safeguard === The consensus is that rewind_safeguard is a workaround for an ALSA bug. This information should come from snd_pcm_rewindable() from the hw plugin. I.e., on a hw device, it should not be equal to snd_pcm_mmap_hw_avail(). While the method of dealing with stale data and negative returns is obvious to me, I am not completely sure about the safeguard. Where should this value come from? Proposal (credit goes to Raymond Yau): the safeguard should be ideally equal to the granularity of hardware pointer updates. However, we don't know this granularity a priori, and cannot know this because nobody will find out this information for old cards. Since we can't get this, let's instead use the upper bound: the minimum possible period size for the given sample rate, sample format and the number of channels. On my desktop PC, on snd-hda-intel with analog outputs for S16LE stereo, the granularity is 32 bytes (= 8 samples), and I get the pointer granularity of 64 bytes (=16 samples) over HDMI. The minimum period size is 32 samples in both cases. Thus Raymond's method will overestimate the required safeguard on my cards, but it is still better than no safeguard at all, and less than the hard-coded value in PulseAudio. On ymfpci, the pointer granularity is 5 ms, thus the value currently hard-coded in PA is insufficient. If I understand the card's limitations correctly, Raymond's method will yield the spot-on result for this card. For the allegedly existing cards where the pointer granularity is always the same as the period size, Raymond's method will not work. However, I don't know any concrete example of such card (I didn't look at snd-firewire in detail, though). And in any case, my opinion is that this "pointer granularity = period size" limitation on such cards can be treated as a driver bug that needs to be fixed (by someone who knows the driver, not by me). On cards where pointer updates happen only on interrupts, the driver should not configure the card in such a way that one period visible to the userspace corresponds to one interrupt. Instead, it should always configure the card for the minimum possible period size, and report only part of the period interrupts to period_elapsed(). I.e.: userspace asked for 2 periods 64 ms each, but let's configure the card for, say, 64 periods 2 ms each, and use the "extra" interrupts only for updating the pointer. Hopefully, the above heuristic and driver fix will also let PulseAudio get rid of special treatment of the "batch" cards, simply because they (in the "pointer granularity = period size" definition which is currently used in PulseAudio) will no longer exist. If the heuristic (or something else) is accepted, then the question remains how to enable the use of snd_pcm_rewindable in PulseAudio. The problem is compatibility with older ALSA versions that, let me remind you, crash if snd_pcm_rewindable is called. I don't have proposals or opinions here. The obvious options are: compile-time alsa-lib version check with fallback to old code on buggy versions, run-time alsa-lib version check with the same fallback, add a (both compile-time and run-time) hard requirement of a fully-fixed alsa-lib version, or do nothing at all. Possible discussion: alternative heuristics and, as mentioned, migration path in PulseAudio. === On non-rewindability of the rate plugin === I intend to write a rewindable resampler eventually, but don't have time now. I understand that it is an important task, but issues below (and the dayjob which you can change by offering me a new one) have higher priority for me. However, I want everyone to understand the following point now: "The resampler has to be written from scratch for the reasons explained in http://permalink.gmane.org/gmane.linux.alsa.devel/122179 , and similar arguments apply to all other kinds of sound processing code that needs history." For PulseAudio, it is also needed to figure out the desired interaction between variable rate and rewindability. Should rewinding other than "discard everything completely" be allowed at all on variable rate streams when the rewind crosses the sample rate change point? I.e., write 100 samples, change rate, write 50 samples, rewind 100 samples, what should be the resulting rate? Should we special-case small changes vs big ones? Slightly off-topic, but still a discussion point. === On possibly-incomplete rewindability of the file plugin === The file plugin has quite hairy code involving the use of the write buffer. I have not verified its correctness or studied the code in detail. Anyway, it looks like it allows rewinding over that buffer only, even though its slave may allow rewinding further. If true, this would make the apparent rewindability useless, as the applications depend on being able to rewind almost everything, and won't like the apparently-random limitation (random because it depends on some obscure "wbuf" implementation detail). I think that the plugin has to be changed to avoid this limitation if it really exists (which I still need to verify). Namely, I propose keeping a shadow copy of the slave's buffer, and, at the beginning of each API call, querying the slave about its hardware pointer. Only the part of the shadow buffer that corresponds to the distance covered by the slave's hardware pointer since the last call has to be written to the file. The same applies to recording. As the code is much simpler there, it is obvious that the problem exists: if one rewinds in order to look at the past recorded samples again, the plugin will read further samples from the file instead of supplying the already-looked-at samples. The same solution with the slave hardware pointer should be applicable. The possible discussion here amounts to suggesting alternative proposals. === On bogus rewindability of ladspa and extplug plugins === For ioplug, see below, because the situation is different enough. Neither LADSPA nor snd_pcm_extplug_t have an API that allows to tell the corresponding plugin to forget the last N samples. We cannot fix LADSPA, as it is beyond our control. And we cannot really fix extplug, because this just means moving the problem down one level. External libraries (which are depended upon by the extplug-based plugins) are almost universally not rewindable, are beyond our control, and will not be made rewindable, because rewindability is hard and is not needed by anyone except ALSA and PulseAudio. Still, neither .rewindable not .rewind currently return 0. So, I'd argue that the only correct result from snd_pcm_rewindable() is 0 for ladspa and extplug plugins. However, right now, they forward the result from the slave. This needs to be fixed, and I have a patch. With snd_pcm_rewind(), the situation is a bit different. It, obviously, does nothing to tell the LADSPA or extplug plugin that the rewind happened (because it can't). It also performs a rewind on the slave. So, assuming that the rewind succeeds and the DSP algorithm in the plugin uses N samples from the past, we'll end up with the output of the correct duration but with N corrupted samples. Not ideal. But if we don't perform the rewind at all and return 0, we'll end up with regressing PulseAudio: huge latency when applying software volume changes or starting new streams. By setting up the large buffer size, it essentially assumes that any rewind will succeed. So, my hackish proposal is: make the .rewindable callbacks on ladspa and extplug return 0, keep the current implementation of the .rewind callback for compatibility with the current versions of PulseAudio, document this hack. I already have patch for this, will send out if we agree that the proposal is good. An alternative proposal (that I would implement if the proposal above is rejected and someone actively confirms this one - but I don't like it) would be to keep the current versions of the .rewindable and .rewind callbacks and add new versions of them (and new user-visible ALSA API) for the situation when imperfect results are unacceptable. A completely strawman proposal would be to introduce a low-latency thread, see below in the ioplug section. Same for .forwardable and .forward. Any other alternatives that I missed? === On bogus rewindability of some ioplug-based plugins === Ioplug-based plugins currently report rewindability according to the same "appl.ptr - hw.ptr" rule that is used by the hw plugin. However, this is incorrect. In fact, there are two types of ioplug-based plugins. The jack plugin does not even have a .transfer callback, and does all the work of irrevocably submitting samples to the JACK server in a thread. So, it is almost completely rewindable (with that "almost" being equal to JACK buffer size). Proposal: keep this logic for all plugins without the .transfer callback. Expose the JACK buffer size as the minimum period size. Then the same rule proposed in the section about the hw plugin will work for .rewindable. Other plugins do their irrevocable work in the .transfer callback and thus are not really rewindable. Proposal: return 0 from .rewindable and rewind ioplug callbacks for all plugins with a .transfer callback (but see the next section for an amendment). Document that the best practice is to have no .transfer callback. Alternative strawman proposal (speculative): if a .transfer callback exists, call it from an alsa-lib-created thread (not from snd_pcm_writei!) at the last possible moment, using the least possible amount of data (that truly cannot be rewound) in the mmap-style buffer. The new mantra is: .transfer is the thing that moves the hardware pointer. This way, previously non-rewindable plugins could become rewindable. Not sure if this is possible without changing the ABI or possible at all, though - it essentially forces all plugins to declare mmap support. Also not sure what to do with the .pointer callback. For the strawman proposal, the benefit is a possibility to have dmix on top of a52 (because freewheeling will now work), the downside is that all programs (not only those intending to do rewinds) now pay the cost of the background low-latency thread. Can we avoid this (e.g. with a new flag for snd_pcm_open), so that only programs that intend to do rewinds pay the price? Can this new flag apply to the batch hw drivers? Won't that inflate the test matrix for plugin code? For discussion: do we implement the first proposal or try the strawman proposal? Any other suggestions regarding ioplug internals? Anyone else has a problem with two completely different interfaces (with and without .transfer) being exposed under the same ioplug framework? === On the pulse plugin === PulseAudio natively supports rewinding on the wire. However, the ioplug-based plugin doesn't. It pretends that it rewound something, but always passes 0, 0 as the last two parameters to pa_stream_write(). That's bad. It looks possible (but I haven't checked) to figure out the correct arguments (i.e. to detect the rewinds done before the call to snd_pcm_writei) by looking at the application pointer. This way, it looks possible to support rewinds, but does not look possible to get the correct result (and not the result implied by the circular-buffer model) from the snd_pcm_rewindable() function. Also, we apply proposals from the previous section, we'll either end up with a plugin that is truly non-rewindable and doesn't pretend to be rewindable, or (with a strawman proposal) a rewindable plugin that forces low latency on the PulseAudio side and causes a lot of wakeups. Both situations are suboptimal. So, I see no way around adding the .rewind and .rewindable callbacks to snd_pcm_ioplug_callback, and treating the plugins with them as rewindable. I guess .forward and .forwardable should be added, too. For discussion: do we do the above or declare that ioplug is not really a suitable infrastructure for the pulse plugin? What other alternatives can be proposed? === On communication of non-rewindability to the program === PulseAudio attempts to use timer-based scheduling and rewinds. It makes a big hardware buffer in expectation that it will be able to rewind - with the exception of a workaround for ioplug-based plugins. It should not set big latency for non-rewindable plugins, but currently has no idea how to get this bit of information. See http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-April/020457.… for the initial problem statement. If all of the proposals above are implemented, then we'll be at a point where each ALSA plugin is either almost fully rewindable, or not rewindable at all. So, contrary to the end of http://permalink.gmane.org/gmane.linux.alsa.devel/122191 , I think we need just snd_pcm_hw_params_is_seekable() (or rename it if you wish) with an essentially boolean result. For discussion: 1. Decide, finally, on this bit of API, so that I can start working. 2. Decide what to do with old alsa-lib versions in PulseAudio. I.e. transition plan. === On the programmer expectations === (social issue) Some people, including at least Andrew Eikum and Clemens Ladisch, at least once in the past expressed the opinion that amounts to "any plugin that does not allow random access is, as far as the ALSA API is concerned, buggy" (quoting http://permalink.gmane.org/gmane.linux.alsa.devel/122159 ), i.e. they are maybe asking for the impossible. We need to do something about that. Choices: 1. Document clearly that there exist non-rewindable plugins, and that it is not a bug until someone implements a working time machine. 2. Implement a strawman proposal with the background low-latency thread if it is doable (and I am not sure that it is doable). 3. Remove ioplug, extplug and ladspa as unfixable, fix the rate and adpcm plugins (or remove adpcm). Do something about pulse and jack because they rely on the to-be-removed ioplug infrastructure. Then all remaining plugins will indeed be fully rewindable. 4. Something else? Sorry for the negative tone here, I know I am bad at formulating and resolving social issues. === Conclusion === When all the proposals are implemented, we'll have correct implementations of the rewind operation in all plugins (where "return 0" counts as correct), and "return 0" only where it is truly unavoidable. PulseAudio will be able to use all of that, and avoid doing rewinds on non-rewindable plugins. On this positive note, I'd like to finish writing this long e-mail. -- Alexander E. Patrakov

9 51

[alsa-devel] [PATCH] ALSA: hda - add explicit include of err.h
by David Henningsson 23 Sep '14

23 Sep '14

Since every caller of snd_hda_jack_detect_enable_callback needs to use the macros from err.h, it makes sense to include it directly from hda_jack.h. Signed-off-by: David Henningsson <david.henningsson(a)canonical.com> --- sound/pci/hda/hda_jack.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h index b41e0a3..13cb375 100644 --- a/sound/pci/hda/hda_jack.h +++ b/sound/pci/hda/hda_jack.h @@ -12,6 +12,8 @@ #ifndef __SOUND_HDA_JACK_H #define __SOUND_HDA_JACK_H +#include <linux/err.h> + struct auto_pin_cfg; struct hda_jack_tbl; struct hda_jack_callback; -- 1.9.1

2 2

Re: [alsa-devel] [PATCH v2 01/11] mixer : fixed double free
by Renu Tyagi 23 Sep '14

23 Sep '14

Hi Takashi, I will prefer "renu tyagi". I cannot use git send-email from my organization. I will resubmit patches where I missed email id in signoff. Or you can see the attachments as they are fine. If any other issues I will be happy to respond. Thanks -------------------------------------------- Signed-off-by: renu tyagi <renu.tyagi(a)samsung.com> --- src/mixer/mixer.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/src/mixer/mixer.c b/src/mixer/mixer.c index 56e023d..eee9875 100644 --- a/src/mixer/mixer.c +++ b/src/mixer/mixer.c @@ -204,10 +204,8 @@ int snd_mixer_attach(snd_mixer_t *mixer, const char *name) if (err < 0) return err; err = snd_mixer_attach_hctl(mixer, hctl); - if (err < 0) { - snd_hctl_close(hctl); + if (err < 0) return err; - } return 0; } -- 1.7.1

2 1

Re: [alsa-devel] [PATCH v2 04/11] rawmidi : close handle h
by Renu Tyagi 23 Sep '14

23 Sep '14

Signed-off-by: renu tyagi <renu.tyagi(a)samsung.com> --- src/rawmidi/rawmidi.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c index b835b47..ac699b4 100644 --- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -256,8 +256,11 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp snd_config_delete(type_conf); if (err >= 0) err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode); - if (err < 0) + if (err < 0) { + if (h) + snd_dlclose(h); return err; + } if (inputp) { (*inputp)->dl_handle = h; h = NULL; snd_rawmidi_params_default(*inputp, &params); -- 1.7.1

1 0

Re: [alsa-devel] [PATCH v2 03/11] sbase : fixed missing free
by Renu Tyagi 23 Sep '14

23 Sep '14

Signed-off-by: renu tyagi <renu.tyagi(a)samsung.com> --- modules/mixer/simple/sbase.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/modules/mixer/simple/sbase.c b/modules/mixer/simple/sbase.c index 97feee8..bb2f59d 100644 --- a/modules/mixer/simple/sbase.c +++ b/modules/mixer/simple/sbase.c @@ -377,6 +377,7 @@ static int simple_event_add1(snd_mixer_class_t *class, if (ctype != SND_CTL_ELEM_TYPE_BOOLEAN) { __invalid_type: snd_mixer_selem_id_free(id); + free(hsimple); return -EINVAL; } break; -- 1.7.1

1 0

[alsa-devel] [PATCH v2 02/11] pcm_file : fixed missing free
by Renu Tyagi 23 Sep '14

23 Sep '14

Signed-off-by: renu tyagi <renu.tyagi(a)samsung.com> --- src/pcm/pcm_file.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index a0b8bf4..f6d222f 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -758,6 +758,7 @@ int snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, ifd = open(ifname, O_RDONLY); /* TODO: mind blocking mode */ if (ifd < 0) { SYSERR("open %s for reading failed", ifname); + free(file->fname); free(file); return -errno; } @@ -772,6 +773,7 @@ int snd_pcm_file_open(snd_pcm_t **pcmp, const char *name, err = snd_pcm_new(&pcm, SND_PCM_TYPE_FILE, name, slave->stream, slave->mode); if (err < 0) { free(file->fname); + free(file->ifname); free(file); return err; } -- 1.7.1

1 0

Re: [alsa-devel] [PATCH v6 05/10] ASoC: Intel: mrfld: add DSP core controls
by Mark Brown 23 Sep '14

23 Sep '14

On Wed, Sep 17, 2014 at 04:25:57PM +0530, Subhransu S. Prusty wrote: > On Tue, Sep 16, 2014 at 12:30:53PM -0700, Mark Brown wrote: > > This is returning with the lock still held AFAICT. I'm a bit surprised > > that we don't need to interact with the hardware if we've disabled > > everything, shouldn't this have some effect on the hardware? > > Also the coding style thing with the comments again. > Will fix the locking and comment. > Regarding interaction with the driver, the slot map is cached and sent in > sst_set_be_modules event. This is sent only when that particular BE is > active, otherwise driver will happily cache these values. > This is the reason why we don't see trigger to DSP when usermode fiddles > around. > In our model only when a particular FE/BE/Mixer/Pipe is active we forward > the settings and parameters, rest we keep the values in driver and forward > when DAPM enables them. > I think we can add this explanation here at top of this file to help. This doesn't really answer my concern - what happens if we're already active and making a change? > Power ops in SST takes care of the PM handling. > Following comment is already added in the code which explains. > "Send the command only if this call is the first enable or last disable" > Let us know if it is not clear enough. No, that comment is orthogonal to the interaction with the DSP which is what is confusing.

2 6