Re: [PATCH] ASoC: Intel: avs: Update stream status in a separate thread

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

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@xxxxxxxxxxxxxxx>


Reviewed-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>

> ---
>   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@xxxxxxxxx>
> + *          Amadeusz Slawinski <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> + */
> +
> +#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