diff mbox series

ASoC: Intel: avs: Update stream status in a separate thread

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

Commit Message

Amadeusz Sławiński Oct. 8, 2024, 8:37 a.m. UTC 
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

Comments

Cezary Rojewski Oct. 8, 2024, 9:19 a.m. UTC | #1 
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
Mark Brown Oct. 8, 2024, 12:51 p.m. UTC | #2 
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 mbox series

Patch

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