Message ID | 20241008083758.756578-1-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 0dbb186c3510cad4e9f443e801bf2e6ab5770c00 |
Headers | show |
Series | ASoC: Intel: avs: Update stream status in a separate thread | expand |
On 2024-10-08 10:37 AM, Amadeusz Sławiński wrote: > Function snd_pcm_period_elapsed() is part of sequence servicing HDAudio > stream IRQs. It's called under Global Interrupt Enable (GIE) disabled - > no HDAudio interrupts will be raised. At the same time, the function may > end up calling __snd_pcm_xrun() or snd_pcm_drain_done(). On the > avs-driver side, this translates to IPCs and as GIE is disabled, these > will never complete successfully. > > Improve system stability by scheduling stream-IRQ handling in a separate > thread. > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/core.c | 3 ++- > sound/soc/intel/avs/pcm.c | 19 +++++++++++++++++++ > sound/soc/intel/avs/pcm.h | 16 ++++++++++++++++ > 3 files changed, 37 insertions(+), 1 deletion(-) > create mode 100644 sound/soc/intel/avs/pcm.h > > diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c > index da7bac09acb4e..73d4bde9b2f78 100644 > --- a/sound/soc/intel/avs/core.c > +++ b/sound/soc/intel/avs/core.c > @@ -28,6 +28,7 @@ > #include "avs.h" > #include "cldma.h" > #include "messages.h" > +#include "pcm.h" > > static u32 pgctl_mask = AZX_PGCTL_LSRMD_MASK; > module_param(pgctl_mask, uint, 0444); > @@ -247,7 +248,7 @@ static void hdac_stream_update_pos(struct hdac_stream *stream, u64 buffer_size) > static void hdac_update_stream(struct hdac_bus *bus, struct hdac_stream *stream) > { > if (stream->substream) { > - snd_pcm_period_elapsed(stream->substream); > + avs_period_elapsed(stream->substream); > } else if (stream->cstream) { > u64 buffer_size = stream->cstream->runtime->buffer_size; > > diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c > index afc0fc74cf941..4af8115803568 100644 > --- a/sound/soc/intel/avs/pcm.c > +++ b/sound/soc/intel/avs/pcm.c > @@ -16,6 +16,7 @@ > #include <sound/soc-component.h> > #include "avs.h" > #include "path.h" > +#include "pcm.h" > #include "topology.h" > #include "../../codecs/hda.h" > > @@ -30,6 +31,7 @@ struct avs_dma_data { > struct hdac_ext_stream *host_stream; > }; > > + struct work_struct period_elapsed_work; > struct snd_pcm_substream *substream; > }; > > @@ -56,6 +58,22 @@ avs_dai_find_path_template(struct snd_soc_dai *dai, bool is_fe, int direction) > return dw->priv; > } > > +static void avs_period_elapsed_work(struct work_struct *work) > +{ > + struct avs_dma_data *data = container_of(work, struct avs_dma_data, period_elapsed_work); > + > + snd_pcm_period_elapsed(data->substream); > +} > + > +void avs_period_elapsed(struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct snd_soc_dai *dai = snd_soc_rtd_to_cpu(rtd, 0); > + struct avs_dma_data *data = snd_soc_dai_get_dma_data(dai, substream); > + > + schedule_work(&data->period_elapsed_work); > +} > + > static int avs_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) > { > struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > @@ -77,6 +95,7 @@ static int avs_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_d > data->substream = substream; > data->template = template; > data->adev = adev; > + INIT_WORK(&data->period_elapsed_work, avs_period_elapsed_work); > snd_soc_dai_set_dma_data(dai, substream, data); > > if (rtd->dai_link->ignore_suspend) > diff --git a/sound/soc/intel/avs/pcm.h b/sound/soc/intel/avs/pcm.h > new file mode 100644 > index 0000000000000..0f3615c903982 > --- /dev/null > +++ b/sound/soc/intel/avs/pcm.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright(c) 2024 Intel Corporation > + * > + * Authors: Cezary Rojewski <cezary.rojewski@intel.com> > + * Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> > + */ > + > +#ifndef __SOUND_SOC_INTEL_AVS_PCM_H > +#define __SOUND_SOC_INTEL_AVS_PCM_H > + > +#include <sound/pcm.h> > + > +void avs_period_elapsed(struct snd_pcm_substream *substream); > + > +#endif For transparency, this short pcm.h header is not a mistake. The team intends to expand it with future changes so that it matches its internal equivalent. To avoid conflicts/code shuffling when pushing those changes, it is easier for us to define it up-front. Czarek
On Tue, 08 Oct 2024 10:37:58 +0200, Amadeusz Sławiński wrote: > Function snd_pcm_period_elapsed() is part of sequence servicing HDAudio > stream IRQs. It's called under Global Interrupt Enable (GIE) disabled - > no HDAudio interrupts will be raised. At the same time, the function may > end up calling __snd_pcm_xrun() or snd_pcm_drain_done(). On the > avs-driver side, this translates to IPCs and as GIE is disabled, these > will never complete successfully. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: Intel: avs: Update stream status in a separate thread commit: 0dbb186c3510cad4e9f443e801bf2e6ab5770c00 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index da7bac09acb4e..73d4bde9b2f78 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -28,6 +28,7 @@ #include "avs.h" #include "cldma.h" #include "messages.h" +#include "pcm.h" static u32 pgctl_mask = AZX_PGCTL_LSRMD_MASK; module_param(pgctl_mask, uint, 0444); @@ -247,7 +248,7 @@ static void hdac_stream_update_pos(struct hdac_stream *stream, u64 buffer_size) static void hdac_update_stream(struct hdac_bus *bus, struct hdac_stream *stream) { if (stream->substream) { - snd_pcm_period_elapsed(stream->substream); + avs_period_elapsed(stream->substream); } else if (stream->cstream) { u64 buffer_size = stream->cstream->runtime->buffer_size; diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index afc0fc74cf941..4af8115803568 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -16,6 +16,7 @@ #include <sound/soc-component.h> #include "avs.h" #include "path.h" +#include "pcm.h" #include "topology.h" #include "../../codecs/hda.h" @@ -30,6 +31,7 @@ struct avs_dma_data { struct hdac_ext_stream *host_stream; }; + struct work_struct period_elapsed_work; struct snd_pcm_substream *substream; }; @@ -56,6 +58,22 @@ avs_dai_find_path_template(struct snd_soc_dai *dai, bool is_fe, int direction) return dw->priv; } +static void avs_period_elapsed_work(struct work_struct *work) +{ + struct avs_dma_data *data = container_of(work, struct avs_dma_data, period_elapsed_work); + + snd_pcm_period_elapsed(data->substream); +} + +void avs_period_elapsed(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); + struct snd_soc_dai *dai = snd_soc_rtd_to_cpu(rtd, 0); + struct avs_dma_data *data = snd_soc_dai_get_dma_data(dai, substream); + + schedule_work(&data->period_elapsed_work); +} + static int avs_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); @@ -77,6 +95,7 @@ static int avs_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_d data->substream = substream; data->template = template; data->adev = adev; + INIT_WORK(&data->period_elapsed_work, avs_period_elapsed_work); snd_soc_dai_set_dma_data(dai, substream, data); if (rtd->dai_link->ignore_suspend) diff --git a/sound/soc/intel/avs/pcm.h b/sound/soc/intel/avs/pcm.h new file mode 100644 index 0000000000000..0f3615c903982 --- /dev/null +++ b/sound/soc/intel/avs/pcm.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright(c) 2024 Intel Corporation + * + * Authors: Cezary Rojewski <cezary.rojewski@intel.com> + * Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> + */ + +#ifndef __SOUND_SOC_INTEL_AVS_PCM_H +#define __SOUND_SOC_INTEL_AVS_PCM_H + +#include <sound/pcm.h> + +void avs_period_elapsed(struct snd_pcm_substream *substream); + +#endif
Function snd_pcm_period_elapsed() is part of sequence servicing HDAudio stream IRQs. It's called under Global Interrupt Enable (GIE) disabled - no HDAudio interrupts will be raised. At the same time, the function may end up calling __snd_pcm_xrun() or snd_pcm_drain_done(). On the avs-driver side, this translates to IPCs and as GIE is disabled, these will never complete successfully. Improve system stability by scheduling stream-IRQ handling in a separate thread. Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/intel/avs/core.c | 3 ++- sound/soc/intel/avs/pcm.c | 19 +++++++++++++++++++ sound/soc/intel/avs/pcm.h | 16 ++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 sound/soc/intel/avs/pcm.h