March 2022 - Alsa-devel - mailman.alsa-project.org

[PATCH v2 00/18] Clean ups and preparation for IPC abstraction in the SOF driver
by Ranjani Sridharan 10 Mar '22

10 Mar '22

In preparation for adding support for the new IPC version that has been introduced in the SOF firmware, this patch set includes some clean ups and necessary modifications to commonly used functions that will be re-used across different IPC-specific code. Changes in v2: - Use guid_t instead of u8 array for uuid and the helper guid_is_null() Pierre-Louis Bossart (1): ASoC: SOF: sof-audio: removed unused function Rander Wang (1): ASoC: SOF: move definition of snd_sof_ipc to header file Ranjani Sridharan (16): ASoC: SOF: remove snd_sof_pipeline_find() ASoC: SOF: simplify snd_sof_device_remove() ASoC: SOF: set swidget's core for scheduler widget ASoC: SOF: topology: remove redundant code ASoC: SOF: topology: remove redundant code in sof_link_afe_load() ASoC: SOF: topology: Drop the size parameter from struct sof_topology_token ASoC: SOF: topology: Modify the get_token op for string tokens ASoC: SOF: topology: expose some get_token ops ASoC: SOF: change comp_dai to a pointer in struct snd_sof_dai ASoC: SOF: make struct snd_sof_widget IPC agnostic ASoC: SOF: topology: make sof_route_load() IPC agnostic ASoC: SOF: Add a tuples array to struct snd_sof_widget ASoC: SOF: topology: Modify signature for token parsing functions ASoC: SOF: topology: Rename arguments in sof_parse_token_sets() ASoC: SOF: topology: Rename arguments in sof_parse_tokens() ASoC: SOF: make struct snd_sof_dai IPC agnostic include/sound/sof/dai.h | 5 + include/sound/sof/topology.h | 8 - sound/soc/sof/core.c | 10 +- sound/soc/sof/intel/hda-dai.c | 27 ++- sound/soc/sof/intel/hda.c | 39 +++- sound/soc/sof/ipc.c | 12 -- sound/soc/sof/pcm.c | 77 +++---- sound/soc/sof/sof-audio.c | 108 +++------- sound/soc/sof/sof-audio.h | 30 ++- sound/soc/sof/sof-priv.h | 12 ++ sound/soc/sof/topology.c | 392 +++++++++++++++++----------------- 11 files changed, 354 insertions(+), 366 deletions(-) -- 2.25.1

2 22

[PATCH] ASoC: Intel: boards: fix randconfig issue
by Pierre-Louis Bossart 10 Mar '22

10 Mar '22

on x86_64: ERROR: modpost: "sof_dai_get_bclk" [sound/soc/intel/boards/snd-soc-intel-sof-cirrus-common.ko] undefined! ERROR: modpost: "sof_dai_get_mclk" [sound/soc/intel/boards/snd-soc-intel-sof-realtek-common.ko] undefined! This comes from a missing dependency on at least ONE SOF platform being selected. This dependency exists for all other machine drivers, this was missed in the earlier reviews. Fixes: 2fe14ff61bd6 ("ASoC: Intel: sof_ssp_amp: rename driver and support cs35l41 amplifier") Reported-by: Randy Dunlap <rdunlap(a)infradead.org> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> --- sound/soc/intel/boards/Kconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig index a62785893bec..f3873b5bea87 100644 --- a/sound/soc/intel/boards/Kconfig +++ b/sound/soc/intel/boards/Kconfig @@ -616,6 +616,8 @@ config SND_SOC_INTEL_SOF_DA7219_MAX98373_MACH endif ## SND_SOC_SOF_JASPERLAKE +if SND_SOC_SOF_HDA_LINK + config SND_SOC_INTEL_SOF_SSP_AMP_MACH tristate "SOF with amplifiers in I2S Mode" depends on I2C && ACPI @@ -632,6 +634,7 @@ config SND_SOC_INTEL_SOF_SSP_AMP_MACH with RT1308/CS35L41 I2S audio codec. Say Y if you have such a device. If unsure select "N". +endif ## SND_SOC_SOF_HDA_LINK if SND_SOC_SOF_ELKHARTLAKE -- 2.30.2

4 3

[PATCH] ASoC: wm_adsp: Compressed stream DSP memory structs should be __packed
by Simon Trimmer 10 Mar '22

10 Mar '22

The compressed stream code has a bunch of structs that are used to represent DSP memory but have not been marked __packed. This isn't safe, they could get padded on a 64-bit build. Signed-off-by: Simon Trimmer <simont(a)opensource.cirrus.com> --- sound/soc/codecs/wm_adsp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 0582585236a2..5a57bb04a0ae 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -97,13 +97,13 @@ struct wm_adsp_system_config_xm_hdr { __be32 wdma[8]; __be32 build_job_name[3]; __be32 build_job_number; -}; +} __packed; struct wm_halo_system_config_xm_hdr { __be32 halo_heartbeat; __be32 build_job_name[3]; __be32 build_job_number; -}; +} __packed; struct wm_adsp_alg_xm_struct { __be32 magic; @@ -114,13 +114,13 @@ struct wm_adsp_alg_xm_struct { __be32 high_water_mark; __be32 low_water_mark; __be64 smoothed_power; -}; +} __packed; struct wm_adsp_host_buf_coeff_v1 { __be32 host_buf_ptr; /* Host buffer pointer */ __be32 versions; /* Version numbers */ __be32 name[4]; /* The buffer name */ -}; +} __packed; struct wm_adsp_buffer { __be32 buf1_base; /* Base addr of first buffer area */ @@ -141,7 +141,7 @@ struct wm_adsp_buffer { __be32 min_free; /* min free space since stream start */ __be32 blocks_written[2]; /* total blocks written (64 bit) */ __be32 words_written[2]; /* total words written (64 bit) */ -}; +} __packed; struct wm_adsp_compr; -- 2.34.1

3 2

RE: [PATCH alsa-lib v2 3/3] pcm: direct: Check xrun/suspend before the slave hwptr update
by S.J. Wang 10 Mar '22

10 Mar '22

Hi > > The xrun/suspend may happen at any time and we should check it right after > the slave hwptr update (but before the actual sync_ptr update in direct pcm > side). Otherwise the hwptr value may be screwed and get unexpected large > read/write. > > Reported-by: S.J. Wang <shengjiu.wang(a)nxp.com> > Signed-off-by: Takashi Iwai <tiwai(a)suse.de> Acked-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> > --- > src/pcm/pcm_dmix.c | 8 +++++--- > src/pcm/pcm_dshare.c | 8 +++++--- > src/pcm/pcm_dsnoop.c | 6 +++--- > 3 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index > d00d53bef604..c6cb47f0f840 100644 > --- a/src/pcm/pcm_dmix.c > +++ b/src/pcm/pcm_dmix.c > @@ -424,15 +424,17 @@ static int snd_pcm_dmix_sync_ptr0(snd_pcm_t > *pcm, snd_pcm_uframes_t slave_hw_ptr static int > snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm) { > snd_pcm_direct_t *dmix = pcm->private_data; > + snd_pcm_uframes_t slave_hw_ptr; > int err; > > + if (dmix->slowptr) > + snd_pcm_hwsync(dmix->spcm); > + slave_hw_ptr = *dmix->spcm->hw.ptr; > err = snd_pcm_direct_check_xrun(dmix, pcm); > if (err < 0) > return err; > - if (dmix->slowptr) > - snd_pcm_hwsync(dmix->spcm); > > - return snd_pcm_dmix_sync_ptr0(pcm, *dmix->spcm->hw.ptr); > + return snd_pcm_dmix_sync_ptr0(pcm, slave_hw_ptr); > } > > /* > diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index > 0ff43a90d270..461adafc77f8 100644 > --- a/src/pcm/pcm_dshare.c > +++ b/src/pcm/pcm_dshare.c > @@ -199,15 +199,17 @@ static int snd_pcm_dshare_sync_ptr0(snd_pcm_t > *pcm, snd_pcm_uframes_t slave_hw_p static int > snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm) { > snd_pcm_direct_t *dshare = pcm->private_data; > + snd_pcm_uframes_t slave_hw_ptr; > int err; > > + if (dshare->slowptr) > + snd_pcm_hwsync(dshare->spcm); > + slave_hw_ptr = *dshare->spcm->hw.ptr; > err = snd_pcm_direct_check_xrun(dshare, pcm); > if (err < 0) > return err; > - if (dshare->slowptr) > - snd_pcm_hwsync(dshare->spcm); > > - return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr); > + return snd_pcm_dshare_sync_ptr0(pcm, slave_hw_ptr); > } > > /* > diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index > 729ff447b41f..9abbbef2c1b6 100644 > --- a/src/pcm/pcm_dsnoop.c > +++ b/src/pcm/pcm_dsnoop.c > @@ -134,14 +134,14 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t > *pcm) > snd_pcm_sframes_t diff; > int err; > > - err = snd_pcm_direct_check_xrun(dsnoop, pcm); > - if (err < 0) > - return err; > if (dsnoop->slowptr) > snd_pcm_hwsync(dsnoop->spcm); > old_slave_hw_ptr = dsnoop->slave_hw_ptr; > snoop_timestamp(pcm); > slave_hw_ptr = dsnoop->slave_hw_ptr; > + err = snd_pcm_direct_check_xrun(dsnoop, pcm); > + if (err < 0) > + return err; > diff = pcm_frame_diff(slave_hw_ptr, old_slave_hw_ptr, dsnoop- > >slave_boundary); > if (diff == 0) /* fast path */ > return 0; > -- > 2.34.1

1 0

[PATCH alsa-lib v2 1/3] pcm: direct: Improved suspend/resume support
by Takashi Iwai 10 Mar '22

10 Mar '22

The current resume handling in PCM direct plugins don't treat multiple clients properly: once after the slave PCM gets resumed by one client, the access from others at a later point is seen as already running although the internal state isn't updated and becomes inconsistent. This may end up a negative size, which eventually hangs up. This patch is an attempt to improve the handling for resume. Now the suspended state is treated similarly like XRUN; namely, we keep the slave PCM "recoveries" count that is modified at each time the slave PCM XRUN happens, so that we can check the inconsistency against the client's state. As a differentiation to XRUN, we set the highest bit of recoveries count when the slave stream hits SUSPENDED state. This bit is referred at comparing with clients, and the client's state is updated to either XRUN or SUSPENDED depending on this bit. Along with this change, the actual resume is done in snd_pcm_direct_slave_recover(), and snd_pcm_direct_resume() rather calls this internally. Signed-off-by: Takashi Iwai <tiwai(a)suse.de> --- src/pcm/pcm_direct.c | 72 ++++++++++++++++++++++++++------------------ src/pcm/pcm_dmix.c | 6 ++-- src/pcm/pcm_dshare.c | 6 ++-- src/pcm/pcm_dsnoop.c | 6 ++-- 4 files changed, 52 insertions(+), 38 deletions(-) diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 193dc3e76d49..1ca3b76306b1 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -560,8 +560,11 @@ int snd_pcm_direct_timer_stop(snd_pcm_direct_t *dmix) return 0; } +#define RECOVERIES_FLAG_SUSPENDED (1U << 31) +#define RECOVERIES_MASK ((1U << 31) - 1) + /* - * Recover slave on XRUN. + * Recover slave on XRUN or SUSPENDED. * Even if direct plugins disable xrun detection, there might be an xrun * raised directly by some drivers. * The first client recovers slave pcm. @@ -569,6 +572,8 @@ int snd_pcm_direct_timer_stop(snd_pcm_direct_t *dmix) */ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) { + unsigned int recoveries; + int state; int ret; int semerr; @@ -579,7 +584,8 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) return semerr; } - if (snd_pcm_state(direct->spcm) != SND_PCM_STATE_XRUN) { + state = snd_pcm_state(direct->spcm); + if (state != SND_PCM_STATE_XRUN && state != SND_PCM_STATE_SUSPENDED) { /* ignore... someone else already did recovery */ semerr = snd_pcm_direct_semaphore_up(direct, DIRECT_IPC_SEM_CLIENT); @@ -590,6 +596,24 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) return 0; } + recoveries = direct->shmptr->s.recoveries; + recoveries = (recoveries + 1) & RECOVERIES_MASK; + if (state == SND_PCM_STATE_SUSPENDED) + recoveries |= RECOVERIES_FLAG_SUSPENDED; + direct->shmptr->s.recoveries = recoveries; + + /* some buggy drivers require the device resumed before prepared; + * when a device has RESUME flag and is in SUSPENDED state, resume + * here but immediately drop to bring it to a sane active state. + */ + if (state == SND_PCM_STATE_SUSPENDED && + (direct->spcm->info & SND_PCM_INFO_RESUME)) { + snd_pcm_resume(direct->spcm); + snd_pcm_drop(direct->spcm); + snd_pcm_direct_timer_stop(direct); + snd_pcm_direct_clear_timer_queue(direct); + } + ret = snd_pcm_prepare(direct->spcm); if (ret < 0) { SNDERR("recover: unable to prepare slave"); @@ -621,7 +645,6 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) } return ret; } - direct->shmptr->s.recoveries++; semerr = snd_pcm_direct_semaphore_up(direct, DIRECT_IPC_SEM_CLIENT); if (semerr < 0) { @@ -632,11 +655,15 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) } /* - * enter xrun state, if slave xrun occurred - * @return: 0 for no xrun or a negative error code for xrun + * enter xrun or suspended state, if slave xrun occurred or suspended + * @return: 0 for no xrun/suspend or a negative error code for xrun/suspend */ int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm) { + if (direct->state == SND_PCM_STATE_XRUN) + return -EPIPE; + else if (direct->state == SND_PCM_STATE_SUSPENDED) + return -ESTRPIPE; if (direct->shmptr->s.recoveries != direct->recoveries) { /* no matter how many xruns we missed - * so don't increment but just update to actual counter @@ -649,10 +676,14 @@ int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm) * if slave already entered xrun again the event is lost. * snd_pcm_direct_clear_timer_queue(direct); */ - direct->state = SND_PCM_STATE_XRUN; + if (direct->recoveries & RECOVERIES_FLAG_SUSPENDED) { + direct->state = SND_PCM_STATE_SUSPENDED; + return -ESTRPIPE; + } else { + direct->state = SND_PCM_STATE_XRUN; + return -EPIPE; + } } - if (direct->state == SND_PCM_STATE_XRUN) - return -EPIPE; return 0; } @@ -721,13 +752,13 @@ timer_changed: } switch (snd_pcm_state(dmix->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: /* recover slave and update client state to xrun * before returning POLLERR */ snd_pcm_direct_slave_recover(dmix); snd_pcm_direct_client_chk_xrun(dmix, pcm); /* fallthrough */ - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_SETUP: events |= POLLERR; break; @@ -1074,27 +1105,10 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm) int snd_pcm_direct_resume(snd_pcm_t *pcm) { snd_pcm_direct_t *dmix = pcm->private_data; - snd_pcm_t *spcm = dmix->spcm; + int err; - snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT); - /* some buggy drivers require the device resumed before prepared; - * when a device has RESUME flag and is in SUSPENDED state, resume - * here but immediately drop to bring it to a sane active state. - */ - if ((spcm->info & SND_PCM_INFO_RESUME) && - snd_pcm_state(spcm) == SND_PCM_STATE_SUSPENDED) { - snd_pcm_resume(spcm); - snd_pcm_drop(spcm); - snd_pcm_direct_timer_stop(dmix); - snd_pcm_direct_clear_timer_queue(dmix); - snd_pcm_areas_silence(snd_pcm_mmap_areas(spcm), 0, - spcm->channels, spcm->buffer_size, - spcm->format); - snd_pcm_prepare(spcm); - snd_pcm_start(spcm); - } - snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); - return -ENOSYS; + err = snd_pcm_direct_slave_recover(dmix); + return err < 0 ? err : -ENOSYS; } #define COPY_SLAVE(field) (dmix->shmptr->s.field = spcm->field) diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index dfff18b992c5..e3bf49269f0c 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -431,6 +431,7 @@ static int snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm) dmix->state = SND_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; @@ -457,11 +458,11 @@ static snd_pcm_state_t snd_pcm_dmix_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dmix->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dmix->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; @@ -833,11 +834,10 @@ static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dmix->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index d63c86bd4044..5a52ae9173be 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -206,6 +206,7 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm) dshare->state = SNDRV_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; @@ -260,11 +261,11 @@ static snd_pcm_state_t snd_pcm_dshare_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dshare->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dshare->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; @@ -532,11 +533,10 @@ static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dshare->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 77ffbada931f..1653f6fba86c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -139,6 +139,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm) dsnoop->state = SNDRV_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; @@ -211,11 +212,11 @@ static snd_pcm_state_t snd_pcm_dsnoop_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dsnoop->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dsnoop->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; @@ -423,11 +424,10 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dsnoop->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } -- 2.34.1

1 2

RE: [PATCH alsa-lib 3/3] pcm: direct: Check xrun/suspend before the slave hwptr update
by S.J. Wang 10 Mar '22

10 Mar '22

Thanks. > > The xrun/suspend may happen at any time and we should check it right > before the slave hwptr update. Otherwise the hwptr value may be screwed I think should be "after the slave hwptr update". If hwptr is screwed, means Suspend happened then check_xrun() can return. > and get unexpected large read/write. > > Reported-by: S.J. Wang <shengjiu.wang(a)nxp.com> > Signed-off-by: Takashi Iwai <tiwai(a)suse.de> > --- > src/pcm/pcm_dmix.c | 4 ++-- > src/pcm/pcm_dshare.c | 4 ++-- > src/pcm/pcm_dsnoop.c | 6 +++--- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index > d00d53bef604..111fea157228 100644 > --- a/src/pcm/pcm_dmix.c > +++ b/src/pcm/pcm_dmix.c > @@ -426,11 +426,11 @@ static int snd_pcm_dmix_sync_ptr(snd_pcm_t > *pcm) > snd_pcm_direct_t *dmix = pcm->private_data; > int err; > > + if (dmix->slowptr) > + snd_pcm_hwsync(dmix->spcm); > err = snd_pcm_direct_check_xrun(dmix, pcm); > if (err < 0) > return err; > - if (dmix->slowptr) > - snd_pcm_hwsync(dmix->spcm); > > return snd_pcm_dmix_sync_ptr0(pcm, *dmix->spcm->hw.ptr); } diff -- Better to get slave_hw_ptr before check_xrun(), like this: --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -424,15 +424,17 @@ static int snd_pcm_dmix_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr static int snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm) { snd_pcm_direct_t *dmix = pcm->private_data; + snd_pcm_uframes_t slave_hw_ptr; int err; if (dmix->slowptr) snd_pcm_hwsync(dmix->spcm); + slave_hw_ptr = *dmix->spcm->hw.ptr; err = snd_pcm_direct_check_xrun(dmix, pcm); if (err < 0) return err; - return snd_pcm_dmix_sync_ptr0(pcm, *dmix->spcm->hw.ptr); + return snd_pcm_dmix_sync_ptr0(pcm, slave_hw_ptr); } > git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index > 0ff43a90d270..c46e8d6a53da 100644 > --- a/src/pcm/pcm_dshare.c > +++ b/src/pcm/pcm_dshare.c > @@ -201,11 +201,11 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t > *pcm) > snd_pcm_direct_t *dshare = pcm->private_data; > int err; > > + if (dshare->slowptr) > + snd_pcm_hwsync(dshare->spcm); > err = snd_pcm_direct_check_xrun(dshare, pcm); > if (err < 0) > return err; > - if (dshare->slowptr) > - snd_pcm_hwsync(dshare->spcm); > > return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr); } diff --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -199,15 +199,17 @@ static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_p static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm) { snd_pcm_direct_t *dshare = pcm->private_data; + snd_pcm_uframes_t slave_hw_ptr; int err; if (dshare->slowptr) snd_pcm_hwsync(dshare->spcm); + slave_hw_ptr = *dshare->spcm->hw.ptr; err = snd_pcm_direct_check_xrun(dshare, pcm); if (err < 0) return err; - return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr); + return snd_pcm_dshare_sync_ptr0(pcm, slave_hw_ptr); > --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index > 729ff447b41f..9abbbef2c1b6 100644 > --- a/src/pcm/pcm_dsnoop.c > +++ b/src/pcm/pcm_dsnoop.c > @@ -134,14 +134,14 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t > *pcm) > snd_pcm_sframes_t diff; > int err; > > - err = snd_pcm_direct_check_xrun(dsnoop, pcm); > - if (err < 0) > - return err; > if (dsnoop->slowptr) > snd_pcm_hwsync(dsnoop->spcm); > old_slave_hw_ptr = dsnoop->slave_hw_ptr; > snoop_timestamp(pcm); > slave_hw_ptr = dsnoop->slave_hw_ptr; > + err = snd_pcm_direct_check_xrun(dsnoop, pcm); > + if (err < 0) > + return err; > diff = pcm_frame_diff(slave_hw_ptr, old_slave_hw_ptr, dsnoop- > >slave_boundary); > if (diff == 0) /* fast path */ > return 0; > -- > 2.34.1

2 1

[PATCH alsa-lib 1/3] pcm: direct: Improved suspend/resume support
by Takashi Iwai 10 Mar '22

10 Mar '22

The current resume handling in PCM direct plugins don't treat multiple clients properly: once after the slave PCM gets resumed by one client, the access from others at a later point is seen as already running although the internal state isn't updated and becomes inconsistent. This may end up a negative size, which eventually hangs up. This patch is an attempt to improve the handling for resume. Now the suspended state is treated similarly like XRUN; namely, we keep the slave PCM "recoveries" count that is modified at each time the slave PCM XRUN happens, so that we can check the inconsistency against the client's state. As a differentiation to XRUN, we set the highest bit of recoveries count when the slave stream hits SUSPENDED state. This bit is referred at comparing with clients, and the client's state is updated to either XRUN or SUSPENDED depending on this bit. Along with this change, the actual resume is done in snd_pcm_direct_slave_recover(), and snd_pcm_direct_resume() rather calls this internally. Signed-off-by: Takashi Iwai <tiwai(a)suse.de> --- src/pcm/pcm_direct.c | 72 ++++++++++++++++++++++++++------------------ src/pcm/pcm_dmix.c | 6 ++-- src/pcm/pcm_dshare.c | 6 ++-- src/pcm/pcm_dsnoop.c | 6 ++-- 4 files changed, 52 insertions(+), 38 deletions(-) diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 193dc3e76d49..1ca3b76306b1 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -560,8 +560,11 @@ int snd_pcm_direct_timer_stop(snd_pcm_direct_t *dmix) return 0; } +#define RECOVERIES_FLAG_SUSPENDED (1U << 31) +#define RECOVERIES_MASK ((1U << 31) - 1) + /* - * Recover slave on XRUN. + * Recover slave on XRUN or SUSPENDED. * Even if direct plugins disable xrun detection, there might be an xrun * raised directly by some drivers. * The first client recovers slave pcm. @@ -569,6 +572,8 @@ int snd_pcm_direct_timer_stop(snd_pcm_direct_t *dmix) */ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) { + unsigned int recoveries; + int state; int ret; int semerr; @@ -579,7 +584,8 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) return semerr; } - if (snd_pcm_state(direct->spcm) != SND_PCM_STATE_XRUN) { + state = snd_pcm_state(direct->spcm); + if (state != SND_PCM_STATE_XRUN && state != SND_PCM_STATE_SUSPENDED) { /* ignore... someone else already did recovery */ semerr = snd_pcm_direct_semaphore_up(direct, DIRECT_IPC_SEM_CLIENT); @@ -590,6 +596,24 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) return 0; } + recoveries = direct->shmptr->s.recoveries; + recoveries = (recoveries + 1) & RECOVERIES_MASK; + if (state == SND_PCM_STATE_SUSPENDED) + recoveries |= RECOVERIES_FLAG_SUSPENDED; + direct->shmptr->s.recoveries = recoveries; + + /* some buggy drivers require the device resumed before prepared; + * when a device has RESUME flag and is in SUSPENDED state, resume + * here but immediately drop to bring it to a sane active state. + */ + if (state == SND_PCM_STATE_SUSPENDED && + (direct->spcm->info & SND_PCM_INFO_RESUME)) { + snd_pcm_resume(direct->spcm); + snd_pcm_drop(direct->spcm); + snd_pcm_direct_timer_stop(direct); + snd_pcm_direct_clear_timer_queue(direct); + } + ret = snd_pcm_prepare(direct->spcm); if (ret < 0) { SNDERR("recover: unable to prepare slave"); @@ -621,7 +645,6 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) } return ret; } - direct->shmptr->s.recoveries++; semerr = snd_pcm_direct_semaphore_up(direct, DIRECT_IPC_SEM_CLIENT); if (semerr < 0) { @@ -632,11 +655,15 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct) } /* - * enter xrun state, if slave xrun occurred - * @return: 0 for no xrun or a negative error code for xrun + * enter xrun or suspended state, if slave xrun occurred or suspended + * @return: 0 for no xrun/suspend or a negative error code for xrun/suspend */ int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm) { + if (direct->state == SND_PCM_STATE_XRUN) + return -EPIPE; + else if (direct->state == SND_PCM_STATE_SUSPENDED) + return -ESTRPIPE; if (direct->shmptr->s.recoveries != direct->recoveries) { /* no matter how many xruns we missed - * so don't increment but just update to actual counter @@ -649,10 +676,14 @@ int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm) * if slave already entered xrun again the event is lost. * snd_pcm_direct_clear_timer_queue(direct); */ - direct->state = SND_PCM_STATE_XRUN; + if (direct->recoveries & RECOVERIES_FLAG_SUSPENDED) { + direct->state = SND_PCM_STATE_SUSPENDED; + return -ESTRPIPE; + } else { + direct->state = SND_PCM_STATE_XRUN; + return -EPIPE; + } } - if (direct->state == SND_PCM_STATE_XRUN) - return -EPIPE; return 0; } @@ -721,13 +752,13 @@ timer_changed: } switch (snd_pcm_state(dmix->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: /* recover slave and update client state to xrun * before returning POLLERR */ snd_pcm_direct_slave_recover(dmix); snd_pcm_direct_client_chk_xrun(dmix, pcm); /* fallthrough */ - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_SETUP: events |= POLLERR; break; @@ -1074,27 +1105,10 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm) int snd_pcm_direct_resume(snd_pcm_t *pcm) { snd_pcm_direct_t *dmix = pcm->private_data; - snd_pcm_t *spcm = dmix->spcm; + int err; - snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT); - /* some buggy drivers require the device resumed before prepared; - * when a device has RESUME flag and is in SUSPENDED state, resume - * here but immediately drop to bring it to a sane active state. - */ - if ((spcm->info & SND_PCM_INFO_RESUME) && - snd_pcm_state(spcm) == SND_PCM_STATE_SUSPENDED) { - snd_pcm_resume(spcm); - snd_pcm_drop(spcm); - snd_pcm_direct_timer_stop(dmix); - snd_pcm_direct_clear_timer_queue(dmix); - snd_pcm_areas_silence(snd_pcm_mmap_areas(spcm), 0, - spcm->channels, spcm->buffer_size, - spcm->format); - snd_pcm_prepare(spcm); - snd_pcm_start(spcm); - } - snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); - return -ENOSYS; + err = snd_pcm_direct_slave_recover(dmix); + return err < 0 ? err : -ENOSYS; } #define COPY_SLAVE(field) (dmix->shmptr->s.field = spcm->field) diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index dfff18b992c5..e3bf49269f0c 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -431,6 +431,7 @@ static int snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm) dmix->state = SND_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; @@ -457,11 +458,11 @@ static snd_pcm_state_t snd_pcm_dmix_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dmix->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dmix->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; @@ -833,11 +834,10 @@ static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dmix->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dmix)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index d63c86bd4044..5a52ae9173be 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -206,6 +206,7 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm) dshare->state = SNDRV_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; @@ -260,11 +261,11 @@ static snd_pcm_state_t snd_pcm_dshare_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dshare->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dshare->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; @@ -532,11 +533,10 @@ static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dshare->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dshare)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 77ffbada931f..1653f6fba86c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -139,6 +139,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm) dsnoop->state = SNDRV_PCM_STATE_DISCONNECTED; return -ENODEV; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; @@ -211,11 +212,11 @@ static snd_pcm_state_t snd_pcm_dsnoop_state(snd_pcm_t *pcm) snd_pcm_state_t state; state = snd_pcm_state(dsnoop->spcm); switch (state) { - case SND_PCM_STATE_SUSPENDED: case SND_PCM_STATE_DISCONNECTED: dsnoop->state = state; return state; case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; @@ -423,11 +424,10 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_mmap_commit(snd_pcm_t *pcm, switch (snd_pcm_state(dsnoop->spcm)) { case SND_PCM_STATE_XRUN: + case SND_PCM_STATE_SUSPENDED: if ((err = snd_pcm_direct_slave_recover(dsnoop)) < 0) return err; break; - case SND_PCM_STATE_SUSPENDED: - return -ESTRPIPE; default: break; } -- 2.34.1

1 2

Re: Issue in pcm_dsnoop.c in alsa-lib
by S.J. Wang 10 Mar '22

10 Mar '22

Hi > > > > > > > > > > Hi Takashi Iwai, Jaroslav Kysela > > > > > > > > We encountered an issue in the pcm_dsnoop use case, could you > > > > please help to have a look? > > > > > > > > *Issue description:* > > > > With two instances for dsnoop type device running in parallel, > > > > after suspend/resume, one of the instances will be hung in memcpy > > > > because the very large copy size is obtained. > > > > > > > > #3 0x0000ffffa78d5098 in snd_pcm_dsnoop_sync_ptr > > > (pcm=0xaaab06563da0) > > > > at pcm_dsnoop.c:158 dsnoop = 0xaaab06563c20 slave_hw_ptr = 64 > > > > old_slave_hw_ptr = 533120 avail = *187651522444320* > > > > > > > > * Reason analysis: * > > > > The root cause that I analysis is that after suspend/resume, > > > > one instance will get the SND_PCM_STATE_SUSPENDED state from slave > > > > pcm > > > device, > > > > then it will do snd_pcm_prepare() and snd_pcm_start(), which > > > > will reset the dsnoop->slave_hw_ptr and the hw_ptr of slave pcm > > > > device, then the state of this instance is correct. But another > > > > instance may not get the SND_PCM_STATE_SUSPENDED state from > slave > > > > pcm device because slave device may have been recovered by first > > > > instance, so the dsnoop->slave_hw_ptr is not reset. but because > > > > hw_ptr of slave pcm device has been reset, so there will be a very large > "avail" size. > > > > > > > > *Solution:* > > > > I didn't come up with a fix for this issue, seems there is no > > > > easy way to let another instance know this case and reset the > > > > dsnoop->slave_hw_ptr, could you please help? > > > > > > Could you try topic/pcm-direct-resume branch on > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > thub > > > .com%2Ftiwai%2Falsa- > > > > lib&data=04%7C01%7Cshengjiu.wang%40nxp.com%7C95f97de3f2c840d > > > > 9853508d9fd2e79ea%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C > > > > 637819198319430045%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM > > > > DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdat > > > > a=WWX1ZlcQhJF3pHJdHPIH%2B0xG9o%2FjQnHG5fHDbKXwQwE%3D&r > > > eserved=0 > > > > > > > Thanks, I push my test result in > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > > ub.com%2Falsa-project%2Falsa- > lib%2Fissues%2F213&data=04%7C01%7Cshe > > > ngjiu.wang%40nxp.com%7Cf71e70640d1b40b66be508d9fdbb2ac2%7C686ea > 1d3bc2b > > > 4c6fa92cd99c5c301635%7C0%7C0%7C637819802581943763%7CUnknown%7 > CTWFpbGZs > > > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn > 0%3D > > %7C3000&sdata=fZ2ogNj2RDTv4DV8vgB71M2m0XtU8UhMiXEV1%2Bl > wUrQ%3D&amp > > ;reserved=0 > > Could you please review? > > Please keep the discussion on ML. > I saw you have update the origin/topic/pcm-direct-resume branch, I test your latest change, it is more stable than before, but still meet once of the issue after overnight test, it it very very low possibility. So I suggest if we need to do below change, shall we? diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 729ff447b41f..cc333b3f4384 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -134,14 +134,21 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm) snd_pcm_sframes_t diff; int err; - err = snd_pcm_direct_check_xrun(dsnoop, pcm); - if (err < 0) - return err; if (dsnoop->slowptr) snd_pcm_hwsync(dsnoop->spcm); old_slave_hw_ptr = dsnoop->slave_hw_ptr; snoop_timestamp(pcm); slave_hw_ptr = dsnoop->slave_hw_ptr; + /* + * FIXME: Move snd_pcm_direct_client_chk_xrun after getting the + * dsnoop->spcm->hw.ptr. If the snd_pcm_direct_slave_recover() + * of another instance happening before dsnoop->spcm->hw.ptr + * is got, then a wrong spcm->hw.ptr is got which cause a wrong + * 'diff' data later. + */ + err = snd_pcm_direct_check_xrun(dsnoop, pcm); + if (err < 0) + return err; diff = pcm_frame_diff(slave_hw_ptr, old_slave_hw_ptr, dsnoop->slave_boundary); best regards wang shengjiu

2 1

[PATCH] ASoC: atmel: mchp-pdmc: Remove unnecessary print function dev_err()
by Jiapeng Chong 10 Mar '22

10 Mar '22

The print function dev_err() is redundant because platform_get_irq() already prints an error. Eliminate the follow coccicheck warning: ./sound/soc/atmel/mchp-pdmc.c:991:2-9: line 991 is redundant because platform_get_irq() already prints an error. Reported-by: Abaci Robot <abaci(a)linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com> --- sound/soc/atmel/mchp-pdmc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c index c44636f6207d..2b48bd3434c7 100644 --- a/sound/soc/atmel/mchp-pdmc.c +++ b/sound/soc/atmel/mchp-pdmc.c @@ -988,7 +988,6 @@ static int mchp_pdmc_probe(struct platform_device *pdev) irq = platform_get_irq(pdev, 0); if (irq < 0) { - dev_err(dev, "failed to get irq: %d\n", irq); return irq; } -- 2.20.1.7.g153144c

2 1

[PATCH V0 0/1] ASoC: msm: fix integer overflow for long duration compress offload playback
by Raghu Bankapur 09 Mar '22

09 Mar '22

32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback duration goes beyond 24 hours. Change data type for this variable to uint64_t to prevent overflow and related playback anomaly. below are the steps used to reproduce Steps:- 1. play all clips from music app with Repeat All Songs enabled 2. one of clip is 1hr duration - 5.1_16bit_192khz_1hr.m4a 3. ACTUAL BEHAVIOUR:- there is no audio playback & progress bar was at end of stream, but time stamp continues to update till 31hrs for 1hr clip Raghu Bankapur (1): ASoC: msm: fix integer overflow for long duration offload playback include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.17.1

4 5