October 2021 - Alsa-devel - mailman.alsa-project.org

[PATCH] ASoC: atmel: add COMPILE_TEST for SND_ATMEL_SOC_PDC
by Randy Dunlap 13 Oct '21

13 Oct '21

Geert pointed out that since sound/soc has the soc_dummy_driver for NO_DMA platforms, it is possible (desirable) to have drivers that depend on HAS_DMA to alternately depend on COMPILE_TEST. This means that SND_ATMEL_SOC_PDC can depend on HAS_DMA || COMPIE_TEST. Fixes: 6c5c659dfe3f ("ASoC: atmel: ATMEL drivers don't need HAS_DMA") Signed-off-by: Randy Dunlap <rdunlap(a)infradead.org> Suggested-by: Geert Uytterhoeven <geert(a)linux-m68k.org> Cc: Liam Girdwood <lgirdwood(a)gmail.com> Cc: Mark Brown <broonie(a)kernel.org> Cc: alsa-devel(a)alsa-project.org Cc: Codrin Ciubotariu <codrin.ciubotariu(a)microchip.com> --- sound/soc/atmel/Kconfig | 1 + 1 file changed, 1 insertion(+) --- linux-next-20211001.orig/sound/soc/atmel/Kconfig +++ linux-next-20211001/sound/soc/atmel/Kconfig @@ -11,6 +11,7 @@ if SND_ATMEL_SOC config SND_ATMEL_SOC_PDC bool + depends on HAS_DMA || COMPILE_TEST config SND_ATMEL_SOC_DMA bool

3 4

[PATCH 1/2] ASoC: dt-bindings: max98520: Add initial bindings
by George Song 13 Oct '21

13 Oct '21

Signed-off-by: George Song <george.song(a)analog.com> --- .../bindings/sound/maxim,max98520.yaml | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/maxim,max98520.yaml diff --git a/Documentation/devicetree/bindings/sound/maxim,max98520.yaml b/Documentation/devicetree/bindings/sound/maxim,max98520.yaml new file mode 100644 index 000000000000..d1f2b5ee775a --- /dev/null +++ b/Documentation/devicetree/bindings/sound/maxim,max98520.yaml @@ -0,0 +1,35 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/maxim,max98520.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim Integrated MAX98520 Speaker Amplifier Driver + +maintainers: + - George Song <George.song(a)analog.com> + +properties: + compatible: + const: maxim,max98520 + + reg: + maxItems: 1 + description: I2C address of the device. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + max98520: amplifier@38 { + compatible = "maxim,max98520"; + reg = <0x38>; + }; + }; -- 2.25.1

1 0

[PATCH v2] alsaloop: added option prateshift for PLAYSHIFT ctl elem used in PLAYSHIFT
by Pavel Hofman 13 Oct '21

13 Oct '21

If snd-aloop device is on playback side, the required sync mode is PLAYSHIFT. That means Loopback ctl elem "PCM Rate Shift 100000" of the corresponding capture side of the Loopback pipe must be controlled (by pitch reciprocal). ASCII name of the playback rate shift ctl elem is specified with newly added option -x/--prateshift, e.g.: -P hw:Loopback,0 -S playshift \ -x iface=PCM,name='PCM Rate Shift 100000',device=1 Signed-off-by: Pavel Hofman <pavel.hofman(a)ivitera.com> --- alsaloop/alsaloop.1 | 11 ++++-- alsaloop/alsaloop.c | 11 +++++- alsaloop/alsaloop.h | 1 + alsaloop/pcmjob.c | 83 ++++++++++++++++++++++++++++++++++----------- 4 files changed, 83 insertions(+), 23 deletions(-) diff --git a/alsaloop/alsaloop.1 b/alsaloop/alsaloop.1 index 6bacfd5..006494d 100644 --- a/alsaloop/alsaloop.1 +++ b/alsaloop/alsaloop.1 @@ -57,6 +57,11 @@ Use given CTL device for playback. Use given CTL device for capture. +.TP +\fI\-x <ctl_ascii_name>\fP | \fI\-\-prateshift=<ctl_ascii_name>\fP + +Specify ctl ascii name for playshift sync mode (see the Examples section). + .TP \fI\-l <latency>\fP | \fI\-\-latency=<frames>\fP @@ -195,10 +200,12 @@ Verbose xrun profiling. Set process wake timeout. .SH EXAMPLES - -.TP +.nf \fBalsaloop \-C hw:0,0 \-P hw:1,0 \-t 50000\fR +\fBalsaloop \-C hw:soundcard,0 \-P hw:Loopback,0 \-t 50000 \-S playshift \\ +\-x iface=PCM,name='PCM Rate Shift 100000',device=1\fR +.ne .SH BUGS None known. .SH AUTHOR diff --git a/alsaloop/alsaloop.c b/alsaloop/alsaloop.c index 06ffadf..4192712 100644 --- a/alsaloop/alsaloop.c +++ b/alsaloop/alsaloop.c @@ -175,6 +175,7 @@ void help(void) "-C,--cdevice capture device\n" "-X,--pctl playback ctl device\n" "-Y,--cctl capture ctl device\n" +"-x,--prateshift playback 'PCM Rate Shift 100000' ascii ctl name\n" "-l,--latency requested latency in frames\n" "-t,--tlatency requested latency in usec (1/1000000sec)\n" "-f,--format sample format\n" @@ -362,6 +363,7 @@ static int parse_config(int argc, char *argv[], snd_output_t *output, {"cdevice", 1, NULL, 'C'}, {"pctl", 1, NULL, 'X'}, {"cctl", 1, NULL, 'Y'}, + {"prateshift", 1, NULL, 'x'}, {"latency", 1, NULL, 'l'}, {"tlatency", 1, NULL, 't'}, {"format", 1, NULL, 'f'}, @@ -391,6 +393,7 @@ static int parse_config(int argc, char *argv[], snd_output_t *output, char *arg_cdevice = NULL; char *arg_pctl = NULL; char *arg_cctl = NULL; + char *arg_prateshift = NULL; unsigned int arg_latency_req = 0; unsigned int arg_latency_reqtime = 10000; snd_pcm_format_t arg_format = SND_PCM_FORMAT_S16_LE; @@ -420,7 +423,7 @@ static int parse_config(int argc, char *argv[], snd_output_t *output, while (1) { int c; if ((c = getopt_long(argc, argv, - "hdg:P:C:X:Y:l:t:F:f:c:r:s:benvA:S:a:m:T:O:w:UW:z", + "hdg:P:C:X:Y:x:l:t:F:f:c:r:s:benvA:S:a:m:T:O:w:UW:z", long_option, NULL)) < 0) break; switch (c) { @@ -446,6 +449,9 @@ static int parse_config(int argc, char *argv[], snd_output_t *output, case 'Y': arg_cctl = strdup(optarg); break; + case 'x': + arg_prateshift = strdup(optarg); + break; case 'l': err = atoi(optarg); arg_latency_req = err >= 4 ? err : 4; @@ -627,6 +633,9 @@ static int parse_config(int argc, char *argv[], snd_output_t *output, logit(LOG_CRIT, "Unable to add ossmixer controls.\n"); exit(EXIT_FAILURE); } + if (arg_prateshift) + play->prateshift_name = arg_prateshift; + #ifdef USE_SAMPLERATE loop->src_enable = arg_samplerate > 0; if (loop->src_enable) diff --git a/alsaloop/alsaloop.h b/alsaloop/alsaloop.h index 1dbcefe..7a98ef3 100644 --- a/alsaloop/alsaloop.h +++ b/alsaloop/alsaloop.h @@ -127,6 +127,7 @@ struct loopback_handle { snd_ctl_elem_value_t *ctl_format; snd_ctl_elem_value_t *ctl_rate; snd_ctl_elem_value_t *ctl_channels; + char *prateshift_name; /* ascii name for the playback rate shift ctl elem */ }; struct loopback { diff --git a/alsaloop/pcmjob.c b/alsaloop/pcmjob.c index 845ab82..8658fa6 100644 --- a/alsaloop/pcmjob.c +++ b/alsaloop/pcmjob.c @@ -1101,7 +1101,8 @@ void update_pitch(struct loopback *loop) #endif } else if (loop->sync == SYNC_TYPE_PLAYRATESHIFT) { - set_rate_shift(loop->play, pitch); + // pitch is capture-based, playback side requires reciprocal + set_rate_shift(loop->play, 1 / pitch); #ifdef USE_SAMPLERATE if (loop->use_samplerate) { loop->src_data.src_ratio = @@ -1172,44 +1173,86 @@ static int get_channels(struct loopback_handle *lhandle) return snd_ctl_elem_value_get_integer(lhandle->ctl_channels, 0); } -static void openctl_elem(struct loopback_handle *lhandle, - int device, int subdevice, - const char *name, - snd_ctl_elem_value_t **elem) +static int openctl_elem_id(struct loopback_handle *lhandle, snd_ctl_elem_id_t *id, + snd_ctl_elem_value_t **elem) { int err; if (snd_ctl_elem_value_malloc(elem) < 0) { *elem = NULL; + snd_ctl_elem_id_free(id); + return -ENOMEM; + } + snd_ctl_elem_value_set_id(*elem, id); + snd_ctl_elem_value_set_interface(*elem, SND_CTL_ELEM_IFACE_PCM); + err = snd_ctl_elem_read(lhandle->ctl, *elem); + if (err < 0) { + snd_ctl_elem_id_free(id); + snd_ctl_elem_value_free(*elem); + *elem = NULL; + return err; } else { - snd_ctl_elem_value_set_interface(*elem, - SND_CTL_ELEM_IFACE_PCM); - snd_ctl_elem_value_set_device(*elem, device); - snd_ctl_elem_value_set_subdevice(*elem, subdevice); - snd_ctl_elem_value_set_name(*elem, name); - err = snd_ctl_elem_read(lhandle->ctl, *elem); - if (err < 0) { - snd_ctl_elem_value_free(*elem); - *elem = NULL; - } else { - if (verbose) - snd_output_printf(lhandle->loopback->output, - "Opened PCM element %s of %s, device %d, subdevice %d\n", - name, snd_ctl_name(lhandle->ctl), device, subdevice); - } + snd_output_printf(lhandle->loopback->output, + "Opened PCM element %s of %s, device %d, subdevice %d\n", + snd_ctl_elem_id_get_name(id), snd_ctl_name(lhandle->ctl), + snd_ctl_elem_id_get_device(id), + snd_ctl_elem_id_get_subdevice(id)); + return 0; } } +static int openctl_elem(struct loopback_handle *lhandle, + int device, int subdevice, + const char *name, + snd_ctl_elem_value_t **elem) +{ + snd_ctl_elem_id_t *id; + + if (snd_ctl_elem_id_malloc(&id)) + return -ENOMEM; + snd_ctl_elem_id_set_device(id, device); + snd_ctl_elem_id_set_subdevice(id, subdevice); + snd_ctl_elem_id_set_name(id, name); + return openctl_elem_id(lhandle, id, elem); +} + +static int openctl_elem_ascii(struct loopback_handle *lhandle, char *ascii_name, + snd_ctl_elem_value_t **elem) +{ + snd_ctl_elem_id_t *id; + + if (snd_ctl_elem_id_malloc(&id)) { + return -ENOMEM; + } + if (snd_ctl_ascii_elem_id_parse(id, ascii_name)) { + snd_ctl_elem_id_free(id); + fprintf(stderr, "Wrong control identifier: %s\n", ascii_name); + return -EINVAL; + } + return openctl_elem_id(lhandle, id, elem); +} + static int openctl(struct loopback_handle *lhandle, int device, int subdevice) { int err; lhandle->ctl_rate_shift = NULL; if (lhandle->loopback->play == lhandle) { + // play only + if (lhandle->prateshift_name) { + err = openctl_elem_ascii(lhandle, lhandle->prateshift_name, + &lhandle->ctl_rate_shift); + if (err < 0) { + logit(LOG_CRIT, "Unable to open playback PCM Rate Shift elem '%s'.\n", + lhandle->prateshift_name); + exit(EXIT_FAILURE); + } + } if (lhandle->loopback->controls) goto __events; return 0; } + // capture only openctl_elem(lhandle, device, subdevice, "PCM Notify", &lhandle->ctl_notify); openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000", -- 2.25.1

2 1

Alsaloop: sync mode PLAYSHIFT + Loopback on playback side
by Pavel Hofman 13 Oct '21

13 Oct '21

Hi, I added support for "Playback Pitch 1000000" ctl elem to UAC2 gadget (not submitted to USB yet) and now I am working on alsaloop support for this ctl elem. The changes are simple (tested to work perfectly, patch to follow), but during the work I hit the following issue with playback Loopback "PCM Rate Shift 100000". If the snd-aloop device is on playback side (i.e. capture from soundcard -> Loopback), the required sync mode is PLAYSHIFT. That means Loopback ctl elem "PCM Rate Shift 100000" should be controlled (by a reciprocal). That is simple by a patch like this: diff --git a/alsaloop/pcmjob.c b/alsaloop/pcmjob.c index 845ab82..619bf35 100644 --- a/alsaloop/pcmjob.c +++ b/alsaloop/pcmjob.c @@ -1061,7 +1061,13 @@ static int set_rate_shift(struct loopback_handle *lhandle, double pitch) int err; if (lhandle->ctl_rate_shift) { - snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift, 0, pitch * 100000); + long value; + if (lhandle->loopback->play == lhandle) + // playback => reciprocal + value = 1/(pitch) * 100000; + else + value = pitch * 100000; + snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift, 0, value); err = snd_ctl_elem_write(lhandle->ctl, lhandle->ctl_rate_shift); } else if (lhandle->capt_pitch) { snd_ctl_elem_value_set_integer(lhandle->capt_pitch, 0, (1 / pitch) * 1000000); @@ -1205,15 +1211,18 @@ static int openctl(struct loopback_handle *lhandle, int device, int subdevice) int err; lhandle->ctl_rate_shift = NULL; + // both play and capture + openctl_elem(lhandle, device, subdevice, "PCM Notify", + &lhandle->ctl_notify); + openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000", + &lhandle->ctl_rate_shift); if (lhandle->loopback->play == lhandle) { + // play only if (lhandle->loopback->controls) goto __events; return 0; } - openctl_elem(lhandle, device, subdevice, "PCM Notify", - &lhandle->ctl_notify); - openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000", - &lhandle->ctl_rate_shift); + // capture only openctl_elem(lhandle, device, subdevice, "Capture Pitch 1000000", &lhandle->capt_pitch); set_rate_shift(lhandle, 1); However, IIUC how the Loopback device works, the "PCM Rate Shift 100000" ctl elem applicable to device=0 on playback side is that of the capture side, i.e. for device=1. The patch above would pick the playback-side device=0 ctl elem in pcmjob.c:openctl_elem. Hard-coding the device=0 -> device=1 is possible, but Loopback supports more devices. Please what solution for picking the correct "PCM Rate Shift 100000" ctl elem for the PLAYSHIFT sync mode would you recommend? Thanks a lot, Pavel.

2 6

Re: [PATCH] ASoC: wm8960: Fix clock configuration on slave mode
by S.j. Wang 13 Oct '21

13 Oct '21

> On Tue, Sep 07, 2021 at 05:11:09PM +0800, Shengjiu Wang wrote: > > There is a noise issue for 8kHz sample rate on slave mode. > > Compared with master mode, the difference is the DACDIV setting, after > > correcting the DACDIV, the noise is gone. > > > > There is no noise issue for 48kHz sample rate, because the default > > value of DACDIV is correct for 48kHz. > > > > So wm8960_configure_clocking() should be functional for ADC and DAC > > function even if it is slave mode. > > > > In order to be compatible for old use case, just add condition for > > checking that sysclk is zero with slave mode. > > > > Fixes: 0e50b51aa22f ("ASoC: wm8960: Let wm8960 driver configure its > > bit clock and frame clock") > > Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> > > --- > > sound/soc/codecs/wm8960.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c > > index 9e621a254392..9c6af76a60fd 100644 > > --- a/sound/soc/codecs/wm8960.c > > +++ b/sound/soc/codecs/wm8960.c > > @@ -742,7 +742,7 @@ static int wm8960_configure_clocking(struct > snd_soc_component *component) > > int i, j, k; > > int ret; > > > > - if (!(iface1 & (1<<6))) { > > + if (!(iface1 & (1 << 6)) && !wm8960->sysclk) { > > dev_dbg(component->dev, > > "Codec is slave mode, no need to configure > > clock\n"); > > Looking through the datasheet it just looks like this if statement has always > been non-sense, it looks pretty clear the clocking should still be configured in > slave mode (apart from BCLKDIV which is presumably ignored in slave mode). > > I would be slightly inclined to suggest it would be better to just fixup any > systems not setting sysclk for slave mode, but I am assuming you are talking > primarily about out of tree systems. So I think we need to at least update the > message here as well, it should probably change to a warning and state that > the we are proceeding with no clock configuration, rather than erroneously > saying it doesn't need one. > > Thanks, > Charles Sorry for missing your email. I will update it in v2. Thanks. Best regards Wang Shengjiu

1 0

[RESEND PATCH] ASoC: wm8960: Fix clock configuration on slave mode
by Shengjiu Wang 13 Oct '21

13 Oct '21

There is a noise issue for 8kHz sample rate on slave mode. Compared with master mode, the difference is the DACDIV setting, after correcting the DACDIV, the noise is gone. There is no noise issue for 48kHz sample rate, because the default value of DACDIV is correct for 48kHz. So wm8960_configure_clocking() should be functional for ADC and DAC function even if it is slave mode. In order to be compatible for old use case, just add condition for checking that sysclk is zero with slave mode. Fixes: 0e50b51aa22f ("ASoC: wm8960: Let wm8960 driver configure its bit clock and frame clock") Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> --- sound/soc/codecs/wm8960.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 9e621a254392..9c6af76a60fd 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -742,7 +742,7 @@ static int wm8960_configure_clocking(struct snd_soc_component *component) int i, j, k; int ret; - if (!(iface1 & (1<<6))) { + if (!(iface1 & (1 << 6)) && !wm8960->sysclk) { dev_dbg(component->dev, "Codec is slave mode, no need to configure clock\n"); return 0; -- 2.17.1

3 2

[PATCH] ALSA: usb-audio: Add quirk for VF0770
by Jonas Hahnfeld 12 Oct '21

12 Oct '21

The device advertises 8 formats, but only a rate of 48kHz is honored by the hardware and 24 bits give chopped audio, so only report the one working combination. This fixes out-of-the-box audio experience with PipeWire which otherwise attempts to choose S24_3LE (while PulseAudio defaulted to S16_LE). Signed-off-by: Jonas Hahnfeld <hahnjo(a)hahnjo.de> --- sound/usb/quirks-table.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h index e03043f7dad3..be5c2bc26a1d 100644 --- a/sound/usb/quirks-table.h +++ b/sound/usb/quirks-table.h @@ -77,6 +77,45 @@ /* E-Mu 0204 USB */ { USB_DEVICE_VENDOR_SPEC(0x041e, 0x3f19) }, +/* + * Creative Technology, Ltd Live! Cam Sync HD [VF0770] + * The device advertises 8 formats, but only a rate of 48kHz is honored by the + * hardware and 24 bits give chopped audio, so only report the one working + * combination. + */ +{ + USB_DEVICE(0x041e, 0x4095), + .driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) { + .ifnum = QUIRK_ANY_INTERFACE, + .type = QUIRK_COMPOSITE, + .data = &(const struct snd_usb_audio_quirk[]) { + { + .ifnum = 2, + .type = QUIRK_AUDIO_STANDARD_MIXER, + }, + { + .ifnum = 3, + .type = QUIRK_AUDIO_FIXED_ENDPOINT, + .data = &(const struct audioformat) { + .formats = SNDRV_PCM_FMTBIT_S16_LE, + .channels = 2, + .fmt_bits = 16, + .iface = 3, + .altsetting = 4, + .altset_idx = 4, + .endpoint = 0x82, + .ep_attr = 0x05, + .rates = SNDRV_PCM_RATE_48000, + .rate_min = 48000, + .rate_max = 48000, + .nr_rates = 1, + .rate_table = (unsigned int[]) { 48000 }, + }, + }, + }, + }, +}, + /* * HP Wireless Audio * When not ignored, causes instability issues for some users, forcing them to -- 2.33.0

2 5

[PATCH v2 0/3] ASoC: SOF: Intel: power optimizations with HDaudio SPIB register
by Pierre-Louis Bossart 12 Oct '21

12 Oct '21

This patchset was initially provided in a larger series that was split in two [1]. This part only provides support for the SPIB register support, added on Intel platforms since Skylake (2015). The use of the SPIB register helps reduce power consumption - though to a smaller degree than DMI_L1. This hardware capability is however incompatible with userspace-initiated rewinds typically used by PulseAudio. In the past (2015..2017) Intel suggested an API extension to let applications disable rewinds. At the time the feedback was that such a capability was too Intel-specific and SPIB remained unused except for loading DSP code. We now see devices with smaller batteries being released, and it's time to revisit Linux support for SPIB to extend battery life. In this update the rewinds are disabled via an opt-in kernel parameter. In the previous reviews, there was consensus that a Kconfig option was too complicated for distributions to set, and we are missing a TBD API to expose such capabilities to user-space. The debate on whether or not to use rewinds, and the impact of disabling rewinds, will likely be closed when Intel releases the 'deep-buffer' support, currently under development [2][3]. With this solution, rewinds will not be needed, ever. When an application deals with content that is not latency-sensitive (e.g. music playback), it will be able to reduce power consumption by selecting a different PCM device with increased buffering capabilities. Low-latency streams will be handled by the 'regular' path. In other words, the impossible compromise between power and latency will be handled with different PCM devices/profiles for the same endpoint, and we can push the design of capability negotiation to a later time when all the building blocks (firmware topology, kernel, userspace) are ready - we still have firmware xruns, DPCM race conditions to solve, and a need to describe these alternate PCM devices with UCM using 'modifiers'. [1] https://lore.kernel.org/alsa-devel/20210610205326.1176400-1-pierre-louis.bo… [2] https://github.com/thesofproject/linux/pull/3146 [3] https://github.com/thesofproject/sof/pull/4611 Pierre-Louis Bossart (1): ALSA: pcm: introduce INFO_NO_REWINDS flag Ranjani Sridharan (2): ASOC: SOF: pcm: add .ack callback support ASoC: SOF: Intel: add .ack support for HDaudio platforms include/uapi/sound/asound.h | 2 +- sound/core/pcm_native.c | 2 ++ sound/soc/sof/intel/apl.c | 1 + sound/soc/sof/intel/cnl.c | 1 + sound/soc/sof/intel/hda-pcm.c | 41 ++++++++++++++++++++++++++++++-- sound/soc/sof/intel/hda-stream.c | 2 ++ sound/soc/sof/intel/hda.h | 1 + sound/soc/sof/intel/tgl.c | 1 + sound/soc/sof/ops.h | 10 ++++++++ sound/soc/sof/pcm.c | 9 +++++++ sound/soc/sof/sof-priv.h | 3 +++ 11 files changed, 70 insertions(+), 3 deletions(-) -- 2.25.1

2 14

[PATCH] ALSA: hda: avoid write to STATESTS if controller is in reset
by Kai Vehmanen 12 Oct '21

12 Oct '21

The snd_hdac_bus_reset_link() contains logic to clear STATESTS register before performing controller reset. This code dates back to an old bugfix in commit e8a7f136f5ed ("[ALSA] hda-intel - Improve HD-audio codec probing robustness"). Originally the code was added to azx_reset(). The code was moved around in commit a41d122449be ("ALSA: hda - Embed bus into controller object") and ended up to snd_hdac_bus_reset_link() and called primarily via snd_hdac_bus_init_chip(). The logic to clear STATESTS is correct when snd_hdac_bus_init_chip() is called when controller is not in reset. In this case, STATESTS can be cleared. This can be useful e.g. when forcing a controller reset to retry codec probe. A normal non-power-on reset will not clear the bits. However, this old logic is problematic when controller is already in reset. The HDA specification states that controller must be taken out of reset before writing to registers other than GCTL.CRST (1.0a spec, 3.3.7). The write to STATESTS in snd_hdac_bus_reset_link() will be lost if the controller is already in reset per the HDA specification mentioned. This has been harmless on older hardware. On newer generation of Intel PCIe based HDA controllers, if configured to report issues, this write will emit an unsupported request error. If ACPI Platform Error Interface (APEI) is enabled in kernel, this will end up to kernel log. Fix the code in snd_hdac_bus_reset_link() to only clear the STATESTS if the function is called when controller is not in reset. Otherwise clearing the bits is not possible and should be skipped. Signed-off-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> --- sound/hda/hdac_controller.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c index 062da7a7a586..f7bd6e2db085 100644 --- a/sound/hda/hdac_controller.c +++ b/sound/hda/hdac_controller.c @@ -421,8 +421,9 @@ int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset) if (!full_reset) goto skip_reset; - /* clear STATESTS */ - snd_hdac_chip_writew(bus, STATESTS, STATESTS_INT_MASK); + /* clear STATESTS if not in reset */ + if (snd_hdac_chip_readb(bus, GCTL) & AZX_GCTL_RESET) + snd_hdac_chip_writew(bus, STATESTS, STATESTS_INT_MASK); /* reset controller */ snd_hdac_bus_enter_link_reset(bus); base-commit: 0fc27129ed268b619135b99d365ce44daaf85602 -- 2.33.0

2 3

[RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE
by Pierre-Louis Bossart 12 Oct '21

12 Oct '21

We've been adding a 'deep buffer' PCM device to several SOF topologies in order to reduce power consumption. The typical use-case would be music playback over a headset: this additional PCM device provides more buffering and longer latencies, leaving the rest of the system sleep for longer periods. Notifications and 'regular' low-latency audio playback would still use the 'normal' PCM device and be mixed with the 'deep buffer' before rendering on the headphone endpoint. The tentative direction would be to expose this alternate device to PulseAudio/PipeWire/CRAS via the UCM SectionModifier definitions. That seemed a straightforward topology change until our automated validation stress tests started reporting issues on SoundWire platforms, when e.g. two START triggers might be send and conversely the STOP trigger is never sent. The SoundWire stream state management flagged inconsistent states when the two 'normal' and 'deep buffer' devices are used concurrently with rapid play/stop/pause monkey testing. Looking at the soc-pcm.c code, it seems that the BE state management needs a lot of love. a) there is no consistent protection for the BE state. In some parts of the code, the state updates are protected by a spinlock but in the trigger they are not. When we open/play/close the two PCM devices in stress tests, we end-up testing a state that is being modified. That can't be good. b) there is a conceptual deadlock: on stop we check the FE states to see if a shared BE can be stopped, but since we trigger the BE first the FE states have not been modified yet, so the TRIGGER_STOP is never sent. This patchset suggests a transition from a spinlock to a mutex, an extended protection when walking through the BE list, and the use of a refcount to decide when to trigger the BE. With these patches I am able to run our entire validation suite without any issues with this new 'deep buffer' topology, and no regressions on existing solutions [1] One might ask 'how come we didn't see this earlier'? The answer is probably that the .trigger callbacks in most implementations seems to perform DAPM operations, and sending the triggers multiple times is not an issue. In the case of SoundWire, we do use the .trigger callback to reconfigure the bus using the 'bank switch' mechanism. It could be acceptable to tolerate a trigger multiple times, but the deadlock on stop cannot be fixed at the SoundWire layer alone. I chose to send this patchset as an RFCv2 to gather more feedback and make use others know about DPCM issues. We're going to spend more time on this but if others can provide feedback/test results it would be greatly appreciated. Opens: 1) is this the right solution? The DPCM code is far from simple, has notions such as SND_SOC_DPCM_UPDATE_NO and 'trigger_pending' that I have no background on. 2) There are other reports of kernel oopses [2] that seem related to the lack of protection. I'd be good to confirm if this patchset solve these problems as well. [1] https://github.com/thesofproject/linux/pull/3146 [2] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.… changes since RFC v1: Removed unused function Removed exported symbols only used in soc-pcm.c, used static instead Use a mutex instead of a spinlock Protect all for_each_dpcm_be() loops Fix bugs introduced in the refcount Pierre-Louis Bossart (5): ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() ASoC: soc-pcm: don't export local functions, use static ASoC: soc-pcm: replace dpcm_lock with dpcm_mutex ASoC: soc-pcm: protect for_each_dpcm_be() loops with dpcm_mutex ASoC: soc-pcm: test refcount before triggering include/sound/soc-dpcm.h | 17 +---- include/sound/soc.h | 2 +- sound/soc/soc-core.c | 2 +- sound/soc/soc-pcm.c | 153 ++++++++++++++++++++++++++------------- 4 files changed, 108 insertions(+), 66 deletions(-) -- 2.25.1

3 27